1656067 - SUMMARY: ThreadSanitizer: data race /builds/worker/workspace/obj-build/dist/include/nsAutoRef.h:566:16 in own

Status: RESOLVED FIXED

(Core :: Audio/Video: MediaStreamGraph, defect, P3)

Description

[Treeherder Bug Filer]

Filed by: bcampen [at] mozilla.com
Parsed log: https://treeherder.mozilla.org/logviewer.html#?job_id=311427425&repo=try
Full log: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/UNLvLP-LSfi5FB2kAnAGdg/runs/0/artifacts/public/logs/live_backing.log

---

Comment 1

[Christian Holler (:decoder)]

This kind of signature with a race on an nsAutoRef involved could be potentially dangerous.

Comment 2

[Byron Campen [:bwc]]

https://searchfox.org/mozilla-central/rev/cffd9b5302b6b6f51533d895a785b48ff418aec1/dom/media/webaudio/blink/HRTFDatabaseLoader.cpp#164

vs

https://searchfox.org/mozilla-central/rev/cffd9b5302b6b6f51533d895a785b48ff418aec1/dom/media/webaudio/blink/HRTFDatabaseLoader.h#89

on

https://searchfox.org/mozilla-central/rev/cffd9b5302b6b6f51533d895a785b48ff418aec1/dom/media/webaudio/blink/HRTFDatabaseLoader.h#138

Comment 3

[Byron Campen [:bwc]]

Yeah, this is pretty unsafe.

Comment 4

[Paul Adenot (:padenot)]

Attachment: Bug 1656067 - r?karlt

Comment 5

[Karl Tomlinson (:karlt)]

The set once and delete when unreferenced nature of the variable here removes the UaF risk associated with nsAutoRef races. There are still the usual undefined behavior characteristics.

Comment 6

[Paul Adenot (:padenot)]

Comment on attachment 9167009 [details]
Bug 1656067 - r?karlt

Security Approval Request

• How easily could an exploit be constructed based on the patch?: https://bugzilla.mozilla.org/show_bug.cgi?id=1656067#c5 explains the setup here. There is a single store from a thread (from nullptr to a valid pointer value). The windows to cause trouble is very very small. It's however reasonably clear what happens here for a reader of the patch
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: This backports cleanly.
• How likely is this patch to cause regressions; how much testing does it need?:

Comment 7

[Tom Ritter [:tjr]]

Comment on attachment 9167009 [details]
Bug 1656067 - r?karlt

Approved to land. If uplift is warranted (it seems from c5 this isn't likely exploitable?) feel free to request.

Comment 8

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

https://hg.mozilla.org/integration/autoland/rev/96b106f417694868b6e2a560a27783862fe54ed6
https://hg.mozilla.org/mozilla-central/rev/96b106f41769

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Doesn't seem like this is worth taking on ESR68 given its upcoming EOL, but uplifting to Beta/ESR78 seems reasonable. Please nominate for approval if you agree.

Comment 10

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

Changing the priority to p1 as the bug is tracked by a release manager for the current beta.
See What Do You Triage for more information

Comment 11

[Paul Adenot (:padenot)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)

Doesn't seem like this is worth taking on ESR68 given its upcoming EOL, but uplifting to Beta/ESR78 seems reasonable. Please nominate for approval if you agree.

I don't think it's worth it.