Closed Bug 1436253 Opened 2 years ago Closed 2 years ago

FunctionBroker::PostToDispatchThread is open to spurious wakes

Categories

(Core :: Security: Process Sandboxing, defect, P1)

Unspecified
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: handyman, Assigned: handyman)

References

Details

Attachments

(1 file)

This is the code:

>  FunctionBrokerChild::GetInstance()->PostToDispatchThread(
>    NewRunnableFunction("FunctionDispatchThreadRunnable", &PostToDispatchHelper,
>                        this, &monitor, &success, &aWinError, &aRet,
>                        &aParameters...));
>  monitor.Wait();
>  return success;

The Wait() clearly has no guard for if the condition variable "has a spurious wake" (or is it "spuriously wakes"?).  I think the result is mysterious crashes like those in bug 1433855.  The code just needs a guard to make sure that any wake was not spurious.  Spurious wakes are explained here: https://en.wikipedia.org/wiki/Spurious_wakeup
This code fixes the issue as well as two related ones -- all in the same spot in PostToDispatchHelper [1].

>  static void
>  PostToDispatchHelper(const SelfType* bmhi, Monitor* monitor, bool* ok, uint32_t* winErr, ResultType* r, ParamTypes*... p)
>  {
>    // Note: p is also non-null... its just hard to assert that.
>    MOZ_ASSERT(bmhi && monitor && ok && winErr && r);
>    *ok = bmhi->BrokerCallClient(*winErr, *r, *p...);
>    {
>      // By grabbing (and freeing) the lock, we make sure that Wait() has been
>      // called in PostToDispatchThread.  We need that since we wake it with
>      // Notify().
>      MonitorAutoLock lock(*monitor);
>    }
>    *ok &= NS_SUCCEEDED(monitor->Notify());
>  };

First:
>    *ok &= NS_SUCCEEDED(monitor->Notify());
ok is assigned after Notify but Notify may have woken the other thread, which would delete ok.

Second, we need to hold the lock until _after_ Notify since a spurious wake before Notify was called would free the Monitor and _then_ call Notify on it (even with the new 'notified' boolean).

This patch fixes all of these issues.

Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fd30c437641002d97cf3d7218c0204b7d4a4d11

[1] https://searchfox.org/mozilla-central/rev/84cea84b12145d752e50ddca6be5462c38510e35/dom/plugins/ipc/FunctionBroker.h#1266
Attachment #8949110 - Flags: review?(jmathies)
Comment on attachment 8949110 [details] [diff] [review]
Fix NPAPI FunctionBroker condition variable concurrency issues

Review of attachment 8949110 [details] [diff] [review]:
-----------------------------------------------------------------

Ay caramba!
Attachment #8949110 - Flags: review?(jmathies) → review+
Yeah, the real fix for concurrency issues is liquor.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a8f3128efea
Fix NPAPI FunctionBroker condition variable concurrency issues. r=aklotz
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9a8f3128efea
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1450708
You need to log in before you can comment on or make changes to this bug.