[haiku-commits] Re: haiku: hrev53337 - src/system/kernel

  • From: Michael Lotz <mmlr@xxxxxxxx>
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Sun, 4 Aug 2019 09:17:19 +0200

On 04.08.19 02:31, waddlesplash wrote:

hrev53337 adds 1 changeset to branch 'master'
old head: 2f1e2ae469ccb1657cc506db06de8de33b89b874
new head: 287428271dd5329c9d84758c43fa350033818a3e
overview: 
https://git.haiku-os.org/haiku/log/?qt=range&q=287428271dd5+%5E2f1e2ae469cc

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

287428271dd5: kernel/condition_variable: Acquire and release locks in the right 
order.

The SpinLocker has an Unlock method, so you wouldn't need to fall back to call release_spinlock() manually.

@@ -325,11 +325,12 @@ ConditionVariable::_NotifyLocked(bool all, status_t 
result)
        // entering Wait() and acquiring its own lock, and then acquiring ours.
        while (ConditionVariableEntry* entry = fEntries.RemoveHead()) {
                release_spinlock(&fLock);
+               acquire_spinlock(&entry->fLock);

I only managed to look at the change more closely just now, but I think there's a problem in this situation (coming from the overall change, not this commit).

Between these two lines both locks are unlocked, the one from the condition variable and the one from the entry. At this point anything may happen, including the ConditionVariableEntry::Wait() timing out. The entry will then check if it is still part of the variable and remove itself if it isn't (this will work fine, although it really looks like a workaround). After that however, the Wait() of the entry will return and the entry will usually become invalid (as these are usually stack allocated). When ConditionVariable::_NotifyLocked() then continues operating on the the entry to acquire its spinlock and evaluate its wait status, it will access invalid memory.

Regards,
Michael

Other related posts: