[haiku-commits] Re: haiku: hrev53104 - src/apps/debugger/user_interface/gui/team_window src/system/kernel/locks headers/private/kernel src/servers/debug

  • From: Jessica Hamilton <jessica.l.hamilton@xxxxxxxxx>
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Fri, 3 May 2019 09:19:46 +1200

On Fri, 3 May 2019 at 09:05, waddlesplash <waddlesplash@xxxxxxxxx> wrote:


Commit:      c2cbf95810ba5aeaae3ec3f9bc276f27593c69e8
URL:         https://git.haiku-os.org/haiku/commit/?id=c2cbf95810ba
Author:      Augustin Cavalier <waddlesplash@xxxxxxxxx>
Date:        Thu May  2 20:07:39 2019 UTC

Ticket:      https://dev.haiku-os.org/ticket/15015

kernel: Add and fix ownership checks in mutex_destroy and mutex_transfer.

 * mutex_destroy() only checked wether or not there were waiters,
   not if the lock itself was presently held by another thread.
   Now we do, which should make #15015 panic much earlier instead
   of trying to use freed memory.
 * mutex_transfer_lock() and recursive_lock_transfer_lock() did
   not check that the calling thread actually owned the lock.
   Now it does, which should trigger asserts if anyone tries
   to do this.

----------------------------------------------------------------------------

diff --git a/headers/private/kernel/lock.h b/headers/private/kernel/lock.h
index 46d1585239..004a0b5fb0 100644
--- a/headers/private/kernel/lock.h
+++ b/headers/private/kernel/lock.h
@@ -138,6 +138,7 @@ extern void mutex_init(mutex* lock, const char* name);
 extern void mutex_init_etc(mutex* lock, const char* name, uint32 flags);
 extern void mutex_destroy(mutex* lock);
 extern status_t mutex_switch_lock(mutex* from, mutex* to);
+extern void mutex_transfer_lock(mutex* lock, thread_id thread);
        // Unlocks "from" and locks "to" such that unlocking and starting to 
wait
        // for the lock is atomically. I.e. if "from" guards the object "to" 
belongs
        // to, the operation is safe as long as "from" is held while 
destroying
@@ -260,15 +261,6 @@ mutex_unlock(mutex* lock)
 }

diff --git a/src/system/kernel/locks/lock.cpp 
b/src/system/kernel/locks/lock.cpp
index ab62a1ac49..db0f0dc1e0 100644
--- a/src/system/kernel/locks/lock.cpp
+++ b/src/system/kernel/locks/lock.cpp
@@ -628,10 +628,9 @@ mutex_destroy(mutex* lock)
        InterruptsSpinLocker locker(lock->lock);

 #if KDEBUG
-       if (lock->waiters != NULL && thread_get_current_thread_id()
-                       != lock->holder) {
-               panic("mutex_destroy(): there are blocking threads, but 
caller doesn't "
-                       "hold the lock (%p)", lock);

I don't understand why you removed the check for blocking threads.
Shouldn't we instead have both panics now?

+       if (lock->holder != -1 && thread_get_current_thread_id() != 
lock->holder) {
+               panic("mutex_destroy(): the lock (%p) is held by %" B_PRId32 
", not "
+                       "by the caller", lock, lock->holder);
                if (_mutex_lock(lock, &locker) != B_OK)
                        return;
                locker.Lock();
@@ -691,6 +690,17 @@ mutex_switch_lock(mutex* from, mutex* to)
 }

Other related posts: