From c2cbf95810ba5aeaae3ec3f9bc276f27593c69e8 Mon Sep 17 00:00:00 2001 From: Augustin Cavalier Date: Thu, 2 May 2019 16:07:39 -0400 Subject: [PATCH] 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. --- headers/private/kernel/lock.h | 12 ++---------- src/system/kernel/locks/lock.cpp | 18 ++++++++++++++---- 2 files changed, 16 insertions(+), 14 deletions(-) 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) } -static inline void -mutex_transfer_lock(mutex* lock, thread_id thread) -{ -#if KDEBUG - lock->holder = thread; -#endif -} - - static inline void recursive_lock_transfer_lock(recursive_lock* lock, thread_id thread) { @@ -276,7 +268,7 @@ recursive_lock_transfer_lock(recursive_lock* lock, thread_id thread) panic("invalid recursion level for lock transfer!"); #if KDEBUG - lock->lock.holder = thread; + mutex_transfer_lock(&lock->lock, thread); #else lock->holder = thread; #endif 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); + 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) } +void +mutex_transfer_lock(mutex* lock, thread_id thread) +{ +#if KDEBUG + if (thread_get_current_thread_id() != lock->holder) + panic("mutex_transfer_lock(): current thread is not the lock holder!"); + lock->holder = thread; +#endif +} + + status_t mutex_switch_from_read_lock(rw_lock* from, mutex* to) {