Closed Bug 1425257 Opened 2 years ago Closed 2 years ago
Crash in mozilla::gfx::Draw
This bug was filed from the Socorro interface and is report bp-4161558c-9197-45e7-ba1c-cf2190171214. ============================================================= Top 6 frames of crashing thread: 0 xul.dll mozilla::gfx::DrawTargetD2D1::Flush gfx/2d/DrawTargetD2D1.cpp:176 1 xul.dll mozilla::gfx::DrawTargetD2D1::MarkChanged gfx/2d/DrawTargetD2D1.cpp:1295 2 xul.dll mozilla::gfx::DrawTargetD2D1::ClearRect gfx/2d/DrawTargetD2D1.cpp:327 3 xul.dll mozilla::dom::CanvasRenderingContext2D::ClearRect dom/canvas/CanvasRenderingContext2D.cpp:3219 4 xul.dll mozilla::dom::CanvasRenderingContext2DBinding::clearRect dom/bindings/CanvasRenderingContext2DBinding.cpp:5307 5 @0x31f93e112c7 ============================================================= these crashes in the content process on windows are continuing after the patch for bug 1416864 has landed in 58.0b9 targetting the same signature. https://mozilla.github.io/stab-crashes/supergraph.html?s=https%3A%2F%2Fcrash-stats.mozilla.com%2Fsearch%2F%3Fsignature%3D%253Dmozilla%253A%253Agfx%253A%253ADrawTargetD2D1%253A%253AFlush%26version%3D58.0b
From the crash dump, it crashed at  and the address of DrawTargetD2D1 instance was filled with 0xe5e5e5. I think this is a UAF issue. mDependingOnTargets is used to store the raw pointer of DrawTargetD2D1. Therefore, it is possible to access a DrawTargetD2D1 which got destroyed. https://searchfox.org/mozilla-central/rev/f0ab0b072ae090f11d6b02b7cf1e0829b4f08882/gfx/2d/DrawTargetD2D1.cpp#176
'mDependentTargets' is also used to store the raw pointer of DrawTargetD2D1. These two structures are used to perform some synchronization. I'm thinking to add a lock to protect 'mDependentTargets' but not mDependingOnTargets to prevent the deadlock.
A crash report that hasn't been deleted: https://crash-stats.mozilla.com/report/index/d77584a0-ac45-4ade-975c-8714d0180103 and a matching one on Skia - https://crash-stats.mozilla.com/report/index/91194efe-0672-48c1-9f05-467f60180103
Assignee: nobody → lsalzman
OS: Windows → All
Lee, this looks pretty inactive while we aim for a ~2 week timeframe for all sec-highs. Can you bump this in your queue? If not, is there a way we can support you getting there?
Bas, ideas? Looks like something still not going right with the locking added in bug 1416864?
Assignee: lsalzman → bas
Flags: needinfo?(lsalzman) → needinfo?(bas)
I feel I know what's going on here. (For the D2D case, I don't think that particular Skia issue has anything to do with this)
This seems like the safest and most upliftable approach.
Attachment #8948440 - Flags: review?(lsalzman)
Comment on attachment 8948440 [details] [diff] [review] Use a global mutex to guard the DT dependency graph. [Security approval request comment] How easily could an exploit be constructed based on the patch? Very difficult, the patch gives little information about where the freed objects are actually located. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? On the bug, yes, on a way to exploit it, no. Which older supported branches are affected by this flaw? 58+ This patch applies there. How likely is this patch to cause regressions; how much testing does it need? Extremely unlikely, the patch only introduces an additional lock.
Attachment #8948440 - Flags: sec-approval?
Attachment #8948440 - Flags: review?(lsalzman) → review+
sec-approval+ for trunk. Please request bounty approval on the patch (or another if necessary) as well.
Attachment #8948440 - Flags: sec-approval? → sec-approval+
(In reply to Al Billings [:abillings] from comment #9) > sec-approval+ for trunk. > Please request bounty approval on the patch (or another if necessary) as > well. I don't know much about out procedures here.. This was posted from the Socorro interface and not as a security bug, is a bounty appropriate here? (Sorry Philipp :-))
Backed out for failing R-e10s in slave/test/build/tests/reftest/tests/image/test/reftest/pngsuite-basic-n/basn0g01.png, furthermore webgl failures on Windows: https://hg.mozilla.org/integration/mozilla-inbound/rev/28d736c28e70c3c092d8b7118511ece0e87da541 Push which ran all the failing tests: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=05b29220fc4de84e676cf4bfe7a673ae640dd929&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log reftest: https://treeherder.mozilla.org/logviewer.html#?job_id=160540484&repo=mozilla-inbound
(In reply to Bas Schouten (:bas.schouten) from comment #10) > > I don't know much about out procedures here.. This was posted from the > Socorro interface and not as a security bug, is a bounty appropriate here? > (Sorry Philipp :-)) As something without a test case and which only has data as a top crash, it doesn't really meet the bar.
Given the low frequency with which this is happening on Nightly, are we likely to get any good sense of whether this is fixed before uplifting to Beta?
Comment on attachment 8948440 [details] [diff] [review] Use a global mutex to guard the DT dependency graph. I was trying to determine that, I'd say the answer is no. Approval Request Comment [Feature/Bug causing the regression]: OMTP [User impact if declined]: Rare, but sec-high crash. [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]:Yes [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: No [Why is the change risky/not risky?]: Only adds a lock, a tiny perf regression is the only potential risk. [String changes made/needed]: None
Attachment #8948440 - Flags: approval-mozilla-beta?
Comment on attachment 8948440 [details] [diff] [review] Use a global mutex to guard the DT dependency graph. Let's get this into today's 59b8 so we can see if it helps sooner.
Attachment #8948440 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main59+]
You need to log in before you can comment on or make changes to this bug.