2004652 - Crash in [@ pthread_mutex_lock | std::sys::pal::unix::sync::mutex::Mutex::lock]

Status: RESOLVED FIXED

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

Description

[Gabriele Svelto [:gsvelto]]

Crash report: https://crash-stats.mozilla.org/report/index/1291cf85-2967-423d-97a4-92e6c0251207

Reason:

EXC_BAD_ACCESS / KERN_INVALID_ADDRESS

Top 10 frames:

0  libsystem_pthread.dylib  pthread_mutex_lock
1  XUL  std::sys::pal::unix::sync::mutex::Mutex::lock  library/std/src/sys/pal/unix/sync/mutex.rs:72
2  XUL  std::sys::sync::mutex::pthread::Mutex::lock  /builds/worker/fetches/rustc/lib/rustlib/src/rust/library/std/src/sys/sync/mutex/pthread.rs:34
2  XUL  std::sync::poison::mutex::Mutex<T>::lock  /builds/worker/fetches/rustc/lib/rustlib/src/rust/library/std/src/sync/poison/mutex.rs:437
3  XUL  cubeb_coreaudio::backend::set_buffer_size_sync::buffer_size_changed_callback  third_party/rust/cubeb-coreaudio/src/backend/mod.rs:1595
4  AudioDSP  AudioDSP@0x3d5e88
5  libdispatch.dylib  _dispatch_call_block_and_release
6  libdispatch.dylib  _dispatch_client_callout
7  libdispatch.dylib  _dispatch_lane_serial_drain
8  libdispatch.dylib  _dispatch_lane_invoke

I found this during nightly crash triage but this problem has been around for months given the crash reports date back almost six months. The crash signatures here aren't great which is why this must have flown under the radar, the crashes all originate from cubeb_coreaudio::backend::set_buffer_size_sync::buffer_size_changed_callback(). That function seems to be accessing a dead object hence the sec flags.

Comment 1

[BugBot [:suhaib / :marco]]

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

For more information, please visit BugBot documentation.

Comment 2

[Karl Tomlinson (:karlt)]

Chun-min would know this code best

Comment 3

[C.M.Chang[:chunmin]]

I'll look into this

Comment 4

[C.M.Chang[:chunmin]]

The callback notified for buffersize's setting result hold an address from memory on stack. When timeout occurs but callback doesn't fire in time, the function is returned and the memory is invalidated. Thus, when callback fires later, it tries to use a memory that is no longer valid. Although the function tried to unregister the callback,I guess this call is queued after the callback or (racing with the callback).

It looks like we can move the info indicating buffersize's setting result to heap by Box. I will add a (stress) test to verify this hypothesis and work on the fix.

Comment 5

[C.M.Chang[:chunmin]]

I found this pattern is used in two places for aggregate device as well: https://searchfox.org/firefox-main/rev/3eaf7e2acf8186eb7aa579561eaa1312cb89132b/third_party/rust/cubeb-coreaudio/src/backend/aggregate_device.rs#157,319.

Comment 6

[C.M.Chang[:chunmin]]

The simplest solution may be putting a table, held by a mutex, and in static memory, so the callback function and the callback unregistration can check the values there to determine how they should perform. This should work for both callback-after-function-return and callback-in-flight-during-unregistration cases.

Comment 7

[C.M.Chang[:chunmin]]

Attachment: (secure)

Comment 8

[C.M.Chang[:chunmin]]

Attachment: (secure)

Comment 9

[C.M.Chang[:chunmin]]

Attachment: (secure)

Comment 10

[C.M.Chang[:chunmin]]

Attachment: (secure)

Comment 11

[C.M.Chang[:chunmin]]

Attachment: 0001-Bug-2004652-Add-testcases.patch

Comment 12

[C.M.Chang[:chunmin]]

Comment on attachment 9540142 [details]
(secure)

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easy. The occurrence of UAF depends on when the system callback is scheduled. This rarely happens in usual cases, given that we don't have many crash reports but this issue exists for ~6 yrs. To exploit this UAF, the attackers would need to do two things: (1) Interfere scheduling of (specific, domain-specific, or all) system callbacks, .e.g, increase CPU loading (2) manipulate a memory in stack, since the freed memory is on stack (comment #4). From the code path, it's not likely to achieve (2).
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Based on my guess for the first question, I don't think it's easy for attackers to exploit this UAF.
• How likely is this patch to cause regressions; how much testing does it need?: Not likely. This issue exists for ~6 years.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: No

Comment 13

[Daniel Veditz [:dveditz]]

I'll mark ESR-115 affected but it's probably safer to wontfix it. Updating to the fixed version of the library might depend on a whole bunch of other changes, and even cherry-picking this fix might not do what we expect depending on what other changes have been made to Audio in the last three years.

For ESR-140 (where we definitely have crash reports) would you update the whole library to the same revision? Or would you just land this patch (minus the packaging stuff) on what we've already vendored? We don't have any kind of nightly or beta builds of ESR-140.x to catch regressions.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 9540142 [details]
(secure)

sec-approval+, a=dveditz
Don't land the separate testcase until after we've shipped the fix. The testcase in the patch itself looks like it's already upstream so I guess that one is already public. I'll set a reminder for 2026-04-07 in case we do rush this fix into 148 beta. If the fix doesn't hit release until v149 then wait another couple of weeks after that (you can set another whiteboard reminder after this one fires if you need to)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/e99a6260dbfe
https://hg.mozilla.org/integration/autoland/rev/1784a09f10c4
r=cubeb-reviewers,pehrsons

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/d8d7f4bd1f4d
https://hg.mozilla.org/integration/autoland/rev/4cc00db297df
Revert "Bug 2004652 r=cubeb-reviewers,pehrsons" for causing rust failures.

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

• Push with failures
• Failure log

Errors occurred; new rust crates were not vendored.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/6c5b18cb7d2b
https://hg.mozilla.org/integration/autoland/rev/7d33197b8d1b
r=cubeb-reviewers,pehrsons

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7d33197b8d1b

Comment 20

[Donal Meehan [:dmeehan]]

There is already a pending need-info, but now that this has landed - :chunmin, please see Comment 13 about uplift requests.
There are only two beta builds left in the Fx148 cycle.

Comment 21

[C.M.Chang[:chunmin]]

(In reply to Donal Meehan [:dmeehan] from comment #20)

There is already a pending need-info, but now that this has landed - :chunmin, please see Comment 13 about uplift requests.
There are only two beta builds left in the Fx148 cycle.

Since cubeb-coreaudio is a crate vendored via ./mach vendor rust, the .cargo-checksum.json and .cargo/config.toml.in may need to be re-generated on each branch. Uplifting a single patch is probably no easy if checksum or .lock is not re-generated. Claude helps me to bypass this issue with a hacky way when I vendored the code changes from my machine-local repo here to review, and make it buildable in try server to test. However, I am not sure if doing the same is okay for updating the library in production code. I'll explore what I can do.

updates:

---

The easiest way is probably create a backport branches for beta, release, and esr 140. I'll check what cubeb-coreaudio commits each branch is using.

Comment 22

[C.M.Chang[:chunmin]]

Attachment: 0001-Bug-2004652-esr-140.patch

This is a trimmed version of the regular crate update patch that can be applied to esr 140.

The build for this patch on try server is here: https://treeherder.mozilla.org/jobs?repo=try&revision=a73bdcfc3e044d5589bc625a8c874a9485d05c06. There are some build error with non-unified build for DecoderDoctorLogger.cpp, but there are unrelated to this change. I suspect they exists before importing this change to the tree. On my local machine, patching the changes on top of esr140 branch builds normally.

There are are some cargo-vet errors when ./mach vendor rust the rust crate in tree, after cargo update -p cubeb-coreaudio, so this patch imports the changes via ./mach vendor rust --force instead, then cleanup/restore unrelated changes. More specifically, this patch excludes all changes except the one in cubeb-coreaudio and checksum. This means we skip adding the [policy.swgl] and [policy.wr_glyph_rasterizer] in supply-chain/config.toml as well. These two crates were recently published to crates.io, after ESR 140, so now require audit-as-crates-io configuration.

Comment 23

[C.M.Chang[:chunmin]]

Comment on attachment 9544396 [details] [diff] [review]
0001-Bug-2004652-esr-140.patch

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Firefox may crash occasionally due to a UAF issue.
• Fix Landed on Version: 149
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This should not cause any regression since it just enforce the synchronous mechanism in several functions.

Note: This patch is a trimmed version of regular updates we would have in nightly. See comment #22 for more details.

Comment 24

[C.M.Chang[:chunmin]]

(In reply to Donal Meehan [:dmeehan] from comment #20)

There is already a pending need-info, but now that this has landed - :chunmin, please see Comment 13 about uplift requests.
There are only two beta builds left in the Fx148 cycle.

Cherry-pick the change for an crate can be a bit tricky (see comment #22) and we might have to create backport branch for each version. Since this rarely happens, and exploiting from this UAF seems not that easy to me, I'd rather let the update ride the train. However, since we have a crash report in ESR 140, so I filed a uplift-esr140- request t.

Comment 25

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:chunmin, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox148 to wontfix.

For more information, please visit BugBot documentation.

Comment 26

[Donal Meehan [:dmeehan]]

:dvedtiz, see Comment 24, any concerns with this shipping in Fx149 and 140.9esr, instead of Fx148 and 1408esr?

Comment 27

[Daniel Veditz [:dveditz]]

Let's wontfix ESR-115 in any case. None of the related crashes happen on any version earlier than ESR-140. That might be related to user counts on older versions but it's still pretty striking. Another interesting thing is that I only see these related signatures on macos 26.x, nothing earlier than that.

It should be safe enough to let this ride the trains to 149 and esr-140.9

Comment 28

[Andreas Pehrson [:pehrsons]]

Attachment: (secure)

Comment 29

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined: sec-high; Firefox may crash occasionally due to a UAF issue.
• Code covered by automated testing: yes
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: This should not cause any regression since it just enforce the synchronous mechanism in several functions.
• String changes made/needed: None
• Is Android affected?: yes

Comment 30

[Andreas Pehrson [:pehrsons]]

Comment on attachment 9544396 [details] [diff] [review]
0001-Bug-2004652-esr-140.patch

Moved to phabricator to get the new flow.

Comment 31

[Andreas Pehrson [:pehrsons]]

Attachment: (secure)

Comment 32

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/18864a3c5b9a

Comment 33

[BugBot [:suhaib / :marco]]

2 months ago, dveditz placed a reminder on the bug using the whiteboard tag [reminder-test 2026-04-07] .

chunmin, please refer to the original comment to better understand the reason for the reminder.

Comment 34

[Daniel Veditz [:dveditz]]

(In reply to Daniel Veditz [:dveditz] from comment #27)

Let's wontfix ESR-115 in any case. None of the related crashes happen on any version earlier than ESR-140.

"unaffected" is better based on the lack of evidence.

Still needs the test landed.