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

Status: RESOLVED FIXED

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

Description

[Byron Campen [:bwc]]

Attachment: tsan output

This looks pretty scary.

Comment 1

[Byron Campen [:bwc]]

Attachment: Another similar tsan failure

Comment 2

[Dan Minor [:dminor]]

From the log: Mutex M180560751542984000 is already destroyed. so I guess a UAF. It is happening here at [1], the corresponding lock is at [2].
Upstream is substantially different: [3].

[1] https://searchfox.org/mozilla-central/rev/89814940895946b48b4c04c702efd2c676ec8e7e/media/webrtc/trunk/webrtc/modules/desktop_capture/linux/x_error_trap.cc#54
[2] https://searchfox.org/mozilla-central/rev/89814940895946b48b4c04c702efd2c676ec8e7e/media/webrtc/trunk/webrtc/modules/desktop_capture/linux/x_error_trap.cc#48
[3] https://source.chromium.org/chromium/chromium/src/+/master:third_party/webrtc/modules/desktop_capture/linux/x_error_trap.cc;l=40

Comment 3

[Byron Campen [:bwc]]

Attachment: Yet another similar failure

Comment 4

[Dan Minor [:dminor]]

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.

Comment 5

[Byron Campen [:bwc]]

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

Comment 6

[Byron Campen [:bwc]]

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?

Comment 7

[Dan Minor [:dminor]]

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

Comment 8

[Christian Holler (:decoder)]

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.

Comment 9

[Byron Campen [:bwc]]

(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.

Comment 10

[Byron Campen [:bwc]]

(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?

Comment 11

[Dan Minor [:dminor]]

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

Comment 12

[Christian Holler (:decoder)]

(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

Comment 13

[Byron Campen [:bwc]]

Attachment: Bug 1652530: Suppress error due to libX11 not being instrumented. r?decoder

Depends on D84391

Comment 14

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d7284cc92d807848a1398be59a16514c18cac8f

Try seems to be about as expected.

Comment 15

[Byron Campen [:bwc]]

Can we unmark this as a sec bug?

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

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

For more information, please visit auto_nag documentation.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1aa3ff231258
Suppress error due to libX11 not being instrumented. r=decoder

Comment 18

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/1aa3ff231258