Closed Bug 1652530 Opened 1 year ago Closed 1 year ago

ThreadSanitizer: unlock of an unlocked mutex [@webrtc::GetWindowList]

Categories

(Core :: WebRTC: Audio/Video, defect)

Unspecified
Linux
defect

Tracking

()

RESOLVED FIXED
81 Branch
Tracking Status
firefox81 --- fixed

People

(Reporter: bwc, Assigned: bwc, NeedInfo)

References

Details

Attachments

(4 files)

Attached file tsan output

This looks pretty scary.

Group: core-security → media-core-security

I noticed we are still running enumeration code for application capture. I'm removing it in Bug 1652552. It runs after window enumeration, so I doubt it is causing any problems here, unless it ends up leaving things in a bad state for the next time window enumeration runs.

Just tested current m-c, it does not look like the landing of bug 1652552 fixed this problem. Looking closer...

I am not seeing any obvious way we could be UAFing here, but I do see that a third_party rust library (x11) is being used by webrtc.org here. Maybe webrtc.org's rust dependencies are not being instrumented properly?

Flags: needinfo?(dminor)
Flags: needinfo?(choller)

There's no way that webrtc.org should be using rust. Are we linking against the wrong library?

Flags: needinfo?(dminor)

I don't see any Rust code in the stacks, so I suspect that's not the issue. However, I do see libX11.so in the stacks, and that is of course not instrumented. For this particular error I believe that should not matter, however if libX11 does not use an intercepted function call (or that interception failed for some reason) to lock the mutex in the first place, then TSan will believe that it is unlocked despite it being locked.

We already suppress several stacks with third party libraries and errors about mutex usage.

If Dan thinks this could be a real uaf, then we should look closer, otherwise I'll be fine with mutex:libX11.so in the suppression list.

(In reply to Dan Minor [:dminor] from comment #7)

There's no way that webrtc.org should be using rust. Are we linking against the wrong library?

I misread something, you're right I think.

(In reply to Christian Holler (:decoder) from comment #8)

I don't see any Rust code in the stacks, so I suspect that's not the issue. However, I do see libX11.so in the stacks, and that is of course not instrumented. For this particular error I believe that should not matter, however if libX11 does not use an intercepted function call (or that interception failed for some reason) to lock the mutex in the first place, then TSan will believe that it is unlocked despite it being locked.

We already suppress several stacks with third party libraries and errors about mutex usage.

If Dan thinks this could be a real uaf, then we should look closer, otherwise I'll be fine with mutex:libX11.so in the suppression list.

That suppression does not seem to work; maybe because the only place libX11 shows up in the output is in the context information describing what created the mutex?

So if LockDisplay() has already succeeded, the only way I can think of that we'd UAF in UnlockDisplay() is if another thread was concurrently freeing the display. I thought all of the window enumeration stuff ran in the same thread. If that's the case, then this sounds like something we can just suppress. Maybe a couple of quick threading assertions in the SharedXDisplay destructor and in the XErrorTrap constructor would confirm this.

[1] https://searchfox.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/desktop_capture/linux/shared_x_display.h#31

(In reply to Byron Campen [:bwc] from comment #10)

That suppression does not seem to work; maybe because the only place libX11 shows up in the output is in the context information describing what created the mutex?

Hm yes, that is possible. In that case, I guess we have to go with mutex:XErrorTrap

Flags: needinfo?(choller)
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED

Can we unmark this as a sec bug?

Flags: needinfo?(dveditz)
Group: media-core-security
Flags: needinfo?(dveditz)

The severity field is not set for this bug.
:jib, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(jib)
Pushed by bcampen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1aa3ff231258
Suppress error due to libX11 not being instrumented. r=decoder
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch
Duplicate of this bug: 1656268
You need to log in before you can comment on or make changes to this bug.