Closed Bug 1417312 Opened 2 years ago Closed 2 years ago

ASAN crash in DEBUG builds WebGLBuffer::Delete() (from CC) invalidating the cache

Categories

(Core :: Canvas: WebGL, defect, P3, minor)

52 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: jesup, Assigned: jgilbert)

References

Details

Attachments

(4 files, 2 obsolete files)

Attached file ars.asan1
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)
Hit it again sitting on arstechnica.  Was right after this, which might point to the GL code being used.  Also this implies it can be reproed, which is good for fixing it, and bad for it being a sec issue.

JavaScript warning: https://cdnjs.cloudflare.com/ajax/libs/pixi.js/4.5.5/pixi.min.js, line 14: Error: WebGL warning: drawElements: This operation requires zeroing texture data. This is slow.
=================================================================
==19829==ERROR: AddressSanitizer: heap-use-after-free on address 0x602000ce9bd8 at pc 0x7f5e2f8286e2 bp 0x7fffc3d378a0 sp 0x7fffc3d37898
Flags: needinfo?(milan)
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)
Group: core-security → gfx-core-security
No, but this is in the new CacheMap code, so I'm looking into it.
Assignee: nobody → jgilbert
Flags: needinfo?(jgilbert)
Attached file ars.asan2
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...
Flags: needinfo?(jgilbert)
Flags: needinfo?(jgilbert)
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?
Attachment #8928792 - Flags: feedback?(rjesup)
Split out from visibility changes.
Attachment #8928792 - Attachment is obsolete: true
Attachment #8928792 - Flags: feedback?(rjesup)
Attachment #8928795 - Flags: feedback?(rjesup)
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?
Flags: needinfo?(dveditz)
Summary: ASAN crash in WebGLBuffer::Delete() (from CC) invalidating the cache → ASAN crash in DEBUG builds WebGLBuffer::Delete() (from CC) invalidating the cache
Group: gfx-core-security
Flags: needinfo?(dveditz)
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 jgilbert@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/e9767488caca
https://hg.mozilla.org/mozilla-central/rev/8a49cb129f99
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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?
Depends on: 1404196
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+
Duplicate of this bug: 1414725
You need to log in before you can comment on or make changes to this bug.