Closed Bug 1417312 Opened 3 years ago Closed 3 years ago
ASAN crash in DEBUG builds Web
GLBuffer::Delete() (from CC) invalidating the cache
35.81 KB, text/plain
36.38 KB, text/plain
59 bytes, text/x-review-board-request
59 bytes, text/x-review-board-request
ASAN crash caught while leaving some tabs idle on arstechnica.com Almost certainly from an ad. Called from CC via WebGLContext::~WebGLContext() The memory was deleted from a call to WebGLContext::~WebGLContext(), but there's no trace of where that came from. Perhaps two contexts were referencing the same data or webglbuffer? Build was inbound from the morning of 11/14 -- 309536e6c69c. Linux64 (Fedora 26) on Lenovo desktop (standard Hub build; Quadro M2000 IIRC)
Jeff, do we have a debugging mode where we can turn on more checking for valid objects and the two types of ref counting we do for WebGL objects?
Flags: needinfo?(milan) → needinfo?(jgilbert)
No, but this is in the new CacheMap code, so I'm looking into it.
Assignee: nobody → jgilbert
Another hit (could have been ars in a background tab, or could have been http://mixer.com/channelone in the foreground
Yup, mixer.com this time
repeatably crashes in ASAN there. I'll try to get an rr trace...
Right; rr and WebGL don't mix :-( Still - it looks to be quite repeatable
Ah, ok, I think I see it. const auto& foo = *(bar.begin()); bar.erase(foo); bar.find(foo) // UAF This is debug-only, and limited in scope, so this should not be a security issue. Let's double-check with a fix, first.
Severity: critical → minor
Status: NEW → ASSIGNED
Priority: -- → P3
Does this fix it?
Split out from visibility changes.
Seems good; let it run for a while, restarted a few times not exhaustive, but looks good
Comment on attachment 8928795 [details] [diff] [review] 0001-Bug-1417312-Use-copy-instead-of-reference-to-avoid-U.patch Haven't seen a crash there in ASAN in WebGL with the patch. Not definitive since actually crashing seemed random (but frequent-ish), so f+. The dangers of auto&... sometimes C++ features save typing but add risks.
Attachment #8928795 - Flags: feedback?(rjesup) → feedback+
(In reply to Randell Jesup [:jesup] from comment #12) > Comment on attachment 8928795 [details] [diff] [review] > 0001-Bug-1417312-Use-copy-instead-of-reference-to-avoid-U.patch > > Haven't seen a crash there in ASAN in WebGL with the patch. Not definitive > since actually crashing seemed random (but frequent-ish), so f+. > > The dangers of auto&... sometimes C++ features save typing but add risks. Eh, it's just a dangling reference due to a remote free. Nothing special about auto& here.
Can we clear the sec flag here?
Comment on attachment 8929186 [details] Bug 1417312 - Adjust visibility of some CacheMap.h members. - https://reviewboard.mozilla.org/r/200482/#review205790 lgtm!
Attachment #8929186 - Flags: review?(dmu) → review+
Comment on attachment 8929185 [details] Bug 1417312 - Use copy instead of reference to avoid UAF in ASSERT. - https://reviewboard.mozilla.org/r/200480/#review205914
Attachment #8929185 - Flags: review?(dmu) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/e9767488caca Use copy instead of reference to avoid UAF in ASSERT. - r=daoshengmu https://hg.mozilla.org/integration/mozilla-inbound/rev/8a49cb129f99 Adjust visibility of some CacheMap.h members. - r=daoshengmu
Attachment #8928795 - Attachment is obsolete: true
Comment on attachment 8929185 [details] Bug 1417312 - Use copy instead of reference to avoid UAF in ASSERT. - Approval Request Comment [Feature/Bug causing the regression]: bug 1404196 [User impact if declined]: Gecko devs may hit spurious ASAN crashes. [Is this code covered by automated tests?]: no [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]: no [Is the change risky?]: no [Why is the change risky/not risky?]: it's debug-only. [String changes made/needed]: no
Attachment #8929185 - Flags: approval-mozilla-beta?
Comment on attachment 8929185 [details] Bug 1417312 - Use copy instead of reference to avoid UAF in ASSERT. - Fix an ASAN crash in debug build issue. Beta58+.
Attachment #8929185 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.