Currently, in the implementation of uniform array setters, we are happily passing too-long array sizes to glUniformXXv and relying the the specified behavior that anything that falls outside array boundaries gets silently ignored. So a call like this, gl.uniform4fv(uniV4, [1, 2, 3, 4, 5, 6, 7, 8]); results in a GL call like this, glUniform4fv(location, 2, ptr); and we are not checking that the uniform array is really of size >= 2. We are relying on the GL to check that. Instead, we should clamp these sizes ourselves. That requires knowing what the real array sizes are. ANGLE gives us that, and we're already querying that : ShGetActiveUniform in CompileShader. We just need to store these values somewhere and use them.

bjacob mentions on IRC that this should be s-s.

Try: https://tbpl.mozilla.org/?tree=Try&rev=d47add43e96b Ugly code. Notes: 1. the only reasonable way to get uniform array sizes is from ANGLE. See how we query this from ANGLE ShGetActiveUniform in CompileShader. Well, we could also do the crazy approach we do in GetUniform, using GL getters, but it's very convoluted and the ANGLE getter (ShGetActiveUniform) was added in the first place because GL implementations are not trustworthy in this area. So it's the GetUniform implementation that should be rewritten to be like this. 2. I didn't want to put the array size information in WebGLMappedIdentifier because identifier mapping is pending big changes, see http://code.google.com/p/angleproject/issues/detail?id=315 , it should become purely a parser thing instead of being specific to uniforms/attribs as it currently is. 3. Sorry that this adds more stuff to the ugly macros implementing uniform getters. We need to refactor all of this anyway to no longer use macros. But for now I wanted a minimal patch, to minimally disrupt Boris's ongoing new-DOM-bindings work and to get this fixed quickly as this is both a security issue and a conformance issue.

The try run is expected to give an orange on Windows due to unexpected pass in conformance/more/functions/uniformfArrayLen1.html . That's the last entry in failing_tests_windows.txt! So once we remove it, we'll be fully passing 1.0.1 conformance on the Windows test slaves.

Comment on attachment 615218 [details] [diff] [review] Check uniform array lengths in uniform setters cancelling review request for now, test failure in conformance/uniforms/gl-uniform-arrays.html

This time we're green at least on my machine. Try: https://tbpl.mozilla.org/?tree=Try&rev=6b1148729ae1

Now with adjusted failing_test_* files. Also note that on Mac, the failing tests list was manipulated directly in the test runner, some of that is now passing with this patch, the rest was redundant with failing_tests_mac.txt, so all is now gone. Try: https://tbpl.mozilla.org/?tree=Try&rev=4efbd53b6bd1

Comment on attachment 615353 [details] [diff] [review] Check uniform array lengths in uniform setters Review of attachment 615353 [details] [diff] [review]: ----------------------------------------------------------------- Exciting, but answer me these questions three. Well, two questions and a style nit. ::: content/canvas/src/WebGLContext.h @@ +1808,5 @@ > return true; > } > > typedef nsDataHashtable<nsCStringHashKey, nsCString> CStringHash; > +typedef nsDataHashtable<nsCStringHashKey, WebGLUniformInfo> CStringToUniformInfoHash; This does match the above line in shortening 'Hashtable' to 'Hash', but 'Table' or 'Map' would be more descriptive. @@ +2005,5 @@ > + * the uniform with given mapped identifier. > + * > + * Note: the input string |name| is the mapped identifier, not the original identifier. > + */ > + WebGLUniformInfo GetUniformInfoForMappedIdentifier(const nsACString& name) { If I remember right, this looks familiar. How hard would it be to fold some of this code out to a shared function? ::: content/canvas/src/WebGLContextGL.cpp @@ +4198,2 @@ > } \ > + int elementSize = location_object->ElementSize(); \ Can we not split out these (near-)duplicate code pieces? It would also reduce the backslash spam.

(In reply to Jeff Gilbert [:jgilbert] from comment #8) > Comment on attachment 615353 [details] [diff] [review] > Check uniform array lengths in uniform setters > > Review of attachment 615353 [details] [diff] [review]: > ----------------------------------------------------------------- > > Exciting, but answer me these questions three. Well, two questions and a > style nit. > > ::: content/canvas/src/WebGLContext.h > @@ +1808,5 @@ > > return true; > > } > > > > typedef nsDataHashtable<nsCStringHashKey, nsCString> CStringHash; > > +typedef nsDataHashtable<nsCStringHashKey, WebGLUniformInfo> CStringToUniformInfoHash; > > This does match the above line in shortening 'Hashtable' to 'Hash', but > 'Table' or 'Map' would be more descriptive. You're right. I took this from MXR, http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistory.h#348 but I agree with you. > > @@ +2005,5 @@ > > + * the uniform with given mapped identifier. > > + * > > + * Note: the input string |name| is the mapped identifier, not the original identifier. > > + */ > > + WebGLUniformInfo GetUniformInfoForMappedIdentifier(const nsACString& name) { > > If I remember right, this looks familiar. How hard would it be to fold some > of this code out to a shared function? Probably reminds you of the MapIdentifier and ReverseMapIdentifier functions just above in this file. It's possible that some could be factored together. But once ANGLE bug 315 is fixed, these MapIdentifier functions are going to change significantly as they will no longer be specific to uniforms and attribs, and will instead map arbitrary tokens. So, I would like to keep things this way for now. See bug 744382. > > ::: content/canvas/src/WebGLContextGL.cpp > @@ +4198,2 @@ > > } \ > > + int elementSize = location_object->ElementSize(); \ > > Can we not split out these (near-)duplicate code pieces? It would also > reduce the backslash spam. We can, but we should go even farther and completely refactot this code to use functions, not macros. I've just filed bug 745840 for that.

Comment on attachment 615353 [details] [diff] [review] Check uniform array lengths in uniform setters Review of attachment 615353 [details] [diff] [review]: ----------------------------------------------------------------- R+ after comments, though please do rename CStringToUniformInfoHash.

http://hg.mozilla.org/mozilla-central/rev/b56db6eab47c (did the renaming, also on CStringHash)

Comment on attachment 615353 [details] [diff] [review] Check uniform array lengths in uniform setters [Approval Request Comment] Regression caused by (bug #): We've had this bug since Firefox 4 User impact if declined: Still no protection from certain buffer overflow vulnerabilities in certain OpenGL drivers Testing completed (on m-c, etc.): Been on m-c for a few days Risk to taking this patch (and alternatives if risky): The risk would be that it doesn't fix the problem and users still aren't protected, if there is an unforeseen bug. But it can't be worse security-wise than not taking the patch. Alternatively, an unforeseen bug could also cause WebGL pages to not display correctly... but that risk seems low and wouldn't be a terrible thing to happen, while the security problem fixed by this patch is real. String changes made by this patch: none

Comment on attachment 615353 [details] [diff] [review] Check uniform array lengths in uniform setters [Triage Comment] Approved for Aurora 13 based upon security interests.

Ouch. patch doesn't apply cleanly on aurora, and I'd rather land this together with the fix for bug 747619 which is a regression caused by this patch.

Is there a way to verify this fix?

Wontfix'ing for Firefox 13, because the fix would be a too large patch (larger than it was for 14+), not worth the risk.

Are we still tracking this for ESR 13+ now or is it going to be later (or should we won't fix this for ESR)?

This seems too heavy to backpoint to ESR, as ESR is based on Firefox 10 and this patch relies on large changes landed in 14.

s/backpoint/backport

(In reply to Benoit Jacob [:bjacob] from comment #18) > This seems too heavy to backpoint to ESR, as ESR is based on Firefox 10 and > this patch relies on large changes landed in 14. We'd really like to address this on the ESR - we're willing to take the additional risk since we don't expect our enterprise community to be affected by WebGL regressions. They could be attacked on this vector though. Are there any other alternatives to the large patch? Driver blocklists, etc.?

There is another way that I can think of, but it will be a serious performance hit. Indeed, this information can in principle be queried from the GL, but in a very convoluted and inefficient way. Actually, we already do something like that in WebGLContext::GetUniform, and that is complicated and inefficient for sure. Fortunately, WebGLContext::GetUniform is practically never called (and when it's called it's typically as a debugging helper) so we don't care about its performance. To use this approach to fix this bug, we would have to do this in the more performance-sensitive WebGLContext::GetUniformLocation.

...however, since ESR users give a rat's ass about WebGL, I suppose we might as well go for it.

Here's the patch implementing that idea. To be as small as possible, this patch just calls the existing unchanged GetUniform method in where we generate WebGLUniformLocation objects. Try: https://tbpl.mozilla.org/?tree=Try&rev=7d10f2615f30 I expect mochitest-1 oranges due to unexpected-passes. Will address this in the failing_test_*.txt files in the next version of this patch.

Tryserver is failing with: configure: error: SDK not found. When using --with-macos-sdk, you must specify a valid SDK. SDKs are installed when the optional cross-development tools are selected during the Xcode/Developer Tools installation. Is pushing ESR10 to try not supported?

Another try... https://tbpl.mozilla.org/?tree=Try&rev=c9d6d8610e0e If pushing-esr10-to-try still doesn't work, I don't know what to do. If it's OK to have M1 oranges on esr10 for a few hours, I could push and land the follow-up patch updating the lists of expected-to-fail tests a few hours later.

Failed again. OK to try what I suggested in comment 26, i.e. going ahead with the push to esr10 and mopping the mochitest floor a few hours after?

(In reply to Benoit Jacob [:bjacob] from comment #26) > Another try... > https://tbpl.mozilla.org/?tree=Try&rev=c9d6d8610e0e > > If pushing-esr10-to-try still doesn't work, I don't know what to do. If it's > OK to have M1 oranges on esr10 for a few hours, I could push and land the > follow-up patch updating the lists of expected-to-fail tests a few hours > later. Yes, it's ok to have M1 oranges on the esr branch if you can push and then get that back to green a few hours after. Please go ahead and nominate what you intend to land for esr.

Comment on attachment 627228 [details] [diff] [review] inefficient but non-intrusive patch for ESR10 [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: It's marked sg:vector but that is optimistic, as it's very likely that drivers to allow reading illegal memory, so in practice it's more like sg:high. User impact if declined: Keeping this (potential, but very likely) vulnerability to illegal memory reads in ESR10 Fix Landed on Version: 14 Risk to taking this patch (and alternatives if risky): could break WebGL rendering if something went wrong, but normally the conformance tests (webgl mochitest) would catch that String or UUID changes made by this patch: none

http://hg.mozilla.org/releases/mozilla-esr10/rev/e0efd9847069 This WILL BE ORANGE and the oranges will be fixed in a followup push later today.

Specifically, mochitest-1 will be orange. Other test suites are not expected to be orange.

http://hg.mozilla.org/releases/mozilla-esr10/rev/3488a16b8dc0 Now I am hoping that this will be all green.

https://hg.mozilla.org/releases/mozilla-esr10/rev/0d2158c1541d Now this will really be green. At least the mochitest fully passes on my machine which it didn't before (but I was wrongly thinking to be caused by a driver issue). Sorry for the sketchy landing, I think I underestimated how far m-c and esr10 had diverged.

all backed out: https://hg.mozilla.org/releases/mozilla-esr10/rev/28d350e964fc will look into remaining failures tomorrow.

Re-landed: https://hg.mozilla.org/releases/mozilla-esr10/rev/b53cce148fbd The only difference is I updated the list of known-to-fail tests on Mac: - removed uniform[if]BadArgs.html as they are now passing - added uniformfArrayLen1.html as it is now failing. It sucks to have a test regression, but this is the best compromise I could find: this is a not-so-important test about the difference between uniform non-arrays and uniform arrays of length 1; It was only succeeding before on Mac as the drivers on our Mac slaves were getting it right, but in general drivers often dont get it right so we were failing on this for many users anyway, for example the drivers on our Windows and Linux slaves get it wrong and so this test is already known as failing there; finally, getting this really right like on m-c would require very intrusive changes.

Is there something QA can do to verify this fix?

Not really; if we made a testcase hammering uniform array setters to try to scribble video memory, maybe one could observe visible symptoms depending on drivers, but we have so many other things to do, and we have good confidence already that this is working, that I'm not advocating us doing that.

Thanks Benoit, given that I'm untracking this for QA verification.