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

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect, P3)

Description

[Randell Jesup [:jesup] (needinfo me)]

Attachment: 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)

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

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?

Comment 3

[Kelsey Gilbert [:jgilbert]]

No, but this is in the new CacheMap code, so I'm looking into it.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Attachment: ars.asan2

Another hit (could have been ars in a background tab, or could have been http://mixer.com/channelone  in the foreground

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Yup, mixer.com this time

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

repeatably crashes in ASAN there.

I'll try to get an rr trace...

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Right; rr and WebGL don't mix :-(  Still - it looks to be quite repeatable

Comment 8

[Kelsey Gilbert [:jgilbert]]

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.

Comment 9

[Kelsey Gilbert [:jgilbert]]

Attachment: 0001-Bug-1417312-Use-copy-instead-of-reference-to-avoid-U.patch

Does this fix it?

Comment 10

[Kelsey Gilbert [:jgilbert]]

Attachment: 0001-Bug-1417312-Use-copy-instead-of-reference-to-avoid-U.patch

Split out from visibility changes.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Seems good; let it run for a while, restarted a few times
not exhaustive, but looks good

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 13

[Kelsey Gilbert [:jgilbert]]

(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.

Comment 14

[Kelsey Gilbert [:jgilbert]]

Can we clear the sec flag here?

Comment 15

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1417312 - Use copy instead of reference to avoid UAF in ASSERT. -

Review commit: https://reviewboard.mozilla.org/r/200480/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200480/

Comment 16

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1417312 - Adjust visibility of some CacheMap.h members. -

Review commit: https://reviewboard.mozilla.org/r/200482/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200482/

Comment 17

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8929186 [details]
Bug 1417312 - Adjust visibility of some CacheMap.h members. -

https://reviewboard.mozilla.org/r/200482/#review205790

lgtm!

Comment 18

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8929185 [details]
Bug 1417312 - Use copy instead of reference to avoid UAF in ASSERT. -

https://reviewboard.mozilla.org/r/200480/#review205914

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/e9767488caca
https://hg.mozilla.org/mozilla-central/rev/8a49cb129f99

Comment 21

[Kelsey Gilbert [:jgilbert]]

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

Comment 22

[Gerry Chang [:gchang]]

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+.

Comment 23

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

https://hg.mozilla.org/releases/mozilla-beta/rev/b1d67871321b
https://hg.mozilla.org/releases/mozilla-beta/rev/a06b50c23f1c