Closed Bug 1696442 Opened 3 years ago Closed 3 years ago

ThreadSanitizer: data race [@ AsyncPanZoomController::SetParent] vs. [@ AsyncPanZoomController::GetParent]

Categories

(Core :: Panning and Zooming, defect)

defect

Tracking

()

RESOLVED FIXED
88 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox86 --- unaffected
firefox87 --- unaffected
firefox88 --- fixed

People

(Reporter: Gankra, Assigned: hiro)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: csectype-race, regression, sec-moderate, Whiteboard: [post-critsmash-triage])

Attachments

(2 files)

I can't really provide any interesting insights other than that clearly the Parent of an APZController is getting racily read on the main thread while it's being written on the compositor thread. This is a potential security issue since that's a race on the value of a RefPtr (and therefore a potential UAF).

Is there some sort of system in place to synchronize the compositor's and the main thread's accesses? Why is it not applicable here?

(This was discovered in the reports for Bug 1612600, after we fixed the original issue that had the same RefPtr signature, I haven't investigated for how long it's been happening.)

Permalinks to problematic lines:

Group: core-security → gfx-core-security
Keywords: csectype-race
Component: Graphics → Panning and Zooming

Hiro, this is new since bug 1678505.

Calling AsyncPanZoomController::GetParent() needs to be done while holding APZCTreeManager::mTreeLock. Sorry for missing this during review.

Regressed by: 1678505
Has Regression Range: --- → yes

The cases where we need to walk up the APZC tree is when ScrollingDownWillMoveDynamicToolbar() returns true, here and here. So if ScrollingDownWillMoveDynamicToolbar() returns a pair of {boolean, const AsyncPanZoomController*} and the const AsyncPanZoomController* is the handoffed APZC, we don't need to walk up the APZC tree there.

(In reply to Botond Ballo [:botond] from comment #1)

Calling AsyncPanZoomController::GetParent() needs to be done while holding APZCTreeManager::mTreeLock. Sorry for missing this during review.

No problem, and thanks! TIL an important fundamental thing about APZ. :)

Assignee: nobody → hikezoe.birchill
Status: NEW → ASSIGNED

Make ScrollingDownWillMoveDynamicToolbar return a pair of boolean and cont AsyncPanZoomController*. r=botond
https://hg.mozilla.org/integration/autoland/rev/7cd1158b5e50b2a8b5a8d91e76a1545dd68b6210
https://hg.mozilla.org/mozilla-central/rev/7cd1158b5e50

Group: gfx-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: