765179 - WebGL crash when empty string is passed to getUniformLocation, getAttribLocation or bindAttribLocation [@mozilla::WebGLProgram::MapIdentifier]

Status: VERIFIED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

This bug is related to bug 765161. 

A square bracket value as the third parameter of bindAttribLocation() will result in a crash. An empty string '' will also lead to a crash. Although this bug is most likely caused by the same issue described in bug 765161, I have created a separate bug for this particular method.

gl.bindAttribLocation(pg, 1, []);

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

[Boris Zbarsky [:bzbarsky]]

[] stringifies to "", so this is exactly the same as bug 765161.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: check for empty variable names

This should fix it, although i am running in Valgrind at the moment to confirm.

When the testcase calls BindAttribLocation with [] as the attribute name string parameter, the string that the DOM bindings pass to WebGLContext::BindAttribLocation is an empty string (but not a null string). So IsEmpty() is the right check for that. Doing it in ValidateGLSLVariableName() allows to fix this bug also in other functions that take such strings and do this long-identifier-mapping work on them: GetAttribLocation and GetUniformLocation.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Boris: is it normal that the DOM bindings silently accept [] as a DOMString parameter and silently convert [] to an empty string?

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Oh, I hadn't seen comment 2. OK.

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Moving to bug 765161 which is more to the point indeed.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

No valgrind errors with this patch.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Boris: is it normal that the DOM bindings silently accept [] as a DOMString parameter 

Yes.  When something is passed to a string parameter, the normal ToString() algorithm from the JS spec is applied; for an empty array that produces "".

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: check for empty string in SplitLastSquareBracket

As Jeff points out, this is the real fix, the other patch was just avoiding the problem. For safety, we probably want both.

Comment 10

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 633539 [details] [diff] [review]
check for empty variable names

Review of attachment 633539 [details] [diff] [review]:
-----------------------------------------------------------------

This is good to have even just as an early-out.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/cbe259c0ba18
http://hg.mozilla.org/integration/mozilla-inbound/rev/cebbfb5c0e02

Reopening just because the patch review ended up happening on this bug rather than the other one.

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 633539 [details] [diff] [review]
check for empty variable names

[Approval Request Comment]
Bug caused by (feature/regressing bug #): at least the crash on empty string has occurred ever since we did long identifier mapping (Gecko 13).
User impact if declined: serious crash, security sensitive
Testing completed (on m-c, etc.): m-i
Risk to taking this patch (and alternatives if risky): no risk, trivial patch checking for empty strings
String or UUID changes made by this patch: none

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 633661 [details] [diff] [review]
check for empty string in SplitLastSquareBracket

[Approval Request Comment]
Bug caused by (feature/regressing bug #): at least the crash on empty string has occurred ever since we did long identifier mapping (Gecko 13).
User impact if declined: serious crash, security sensitive
Testing completed (on m-c, etc.): m-i
Risk to taking this patch (and alternatives if risky): no risk, trivial patch checking for empty strings
String or UUID changes made by this patch: none

Comment 15

[Benoit Jacob [:bjacob] (mostly away)]

https://hg.mozilla.org/mozilla-central/rev/cbe259c0ba18
https://hg.mozilla.org/mozilla-central/rev/cebbfb5c0e02

Comment 16

[Daniel Veditz [:dveditz]]

I'm not sure what "sec-other" signifies here, that means "not s security bug, hidden for some other reason" but this looks like it's either a security bug or not sensitive at all.

If you were clever enough to arrange memory blocks (see upcoming blackhat talk on jemalloc) so that the block allocated next to string ended in a ']' then SplitLastSquareBracket would back up into some other block's memory until you found a '[' (or crashed harmlessly on an access violation, but we're already assuming a clever attacker who arranged a ']' at the end of the block). Then it'd slurp up the contents into the bracketPart (could the attacker read this back?) and stomp a null in this other block. Potentially exploitable but wouldn't be easy.

Shouldn't we try to get this into Beta as well?

Comment 17

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 633539 [details] [diff] [review]
check for empty variable names

[Approval Request Comment]
Same as for aurora. This does affect beta as well.

Comment 18

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 633661 [details] [diff] [review]
check for empty string in SplitLastSquareBracket

[Approval Request Comment]
Same as for aurora. This does affect beta as well.

Comment 19

[Al Billings [:abillings - ex-MoCo]]

I just tried the testcase on OS X with the current Aurora build (6-18) and the last released Firefox and I cannot get a crash with it. Is there a particular video card or environment needed to trigger this issue?

Comment 20

[Benoit Jacob [:bjacob] (mostly away)]

No, it should be independent of the video card, but since it's undefined behavior depending on the contents of memory beyond the end of an allocated buffer, it's not too surprising that this doesn't reproduce everywhere.

Comment 21

[Benoit Jacob [:bjacob] (mostly away)]

(I reproduced on first try on Linux 64-bit)

Comment 22

[Al Billings [:abillings - ex-MoCo]]

All right. Well, I have OS X and virtual machines of others, which may or may not help. I tried in Windows 7 and Ubuntu 12.04 64-bit and could not reproduce the crash in these either. I guess someone in QA will have to verify this fix.

Comment 23

[Christoph Diehl [:posidron]]

I can verify that the bug is fixed.

Comment 24

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/25409d56dde5
http://hg.mozilla.org/releases/mozilla-aurora/rev/dc053a16a5d9
http://hg.mozilla.org/releases/mozilla-beta/rev/61d6118ed89d
http://hg.mozilla.org/releases/mozilla-beta/rev/1a95b498abe3