This WebGL conformance test: https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/invalid-passed-params.html fails on "Test shaderSource() with invalid characters" and similar parts about bindAttribLocation(), getAttribLocation() and getUniformLocation(). In all these cases, the reason for these test failures is the same: these WebGL functions take strings as parameters, and the WebGL specification requires that all characters in these strings must be in the 'ISO/IEC 646:1991' range. See http://www.khronos.org/registry/webgl/specs/latest/#6.18 This roughly means 7-bit ASCII but really for a practical definition the easiest is to just look at the WebKit helper function checking that a character is in this range: http://trac.webkit.org/browser/trunk/Source/WebCore/html/canvas/WebGLRenderingContext.cpp#L123 Our bug is that we don't perform this check in our implementation of these functions. We need to fix this by having bindAttribLocation, getAttribLocation, getUniformLocation, and shaderSource check their string parameters for illegal characters before going on. In case of illegal characters, they need to call WebGLContext::ErrorInvalidOperation("Some helpful text that will show up as a JS warning"). All these 4 functions are implemented in this file: http://hg.mozilla.org/mozilla-central/file/6dc468c41136/content/canvas/src/WebGLContextGL.cpp For example, getAttribLocation is here: http://hg.mozilla.org/mozilla-central/file/6dc468c41136/content/canvas/src/WebGLContextGL.cpp#l1832 We probably need to add a new helper function, which could be called "ValidateGLSLCharacterSet" or some such, to do this job. Notice that WebKit already has one, http://trac.webkit.org/browser/trunk/Source/WebCore/html/canvas/WebGLRenderingContext.cpp#L4174 http://trac.webkit.org/browser/trunk/Source/WebCore/html/canvas/WebGLRenderingContext.cpp#L123

Note: the conformance test link seems to be broken. Here's the corrected one: https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/misc/invalid-passed-params.html

Now passes this test case on Khronos site: https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/misc/invalid-passed-params.html

removed the failed test file content/canvas/test/webgl/conformance/invalid-passed-parameters.html

Comment on attachment 555273 [details] [diff] [review] Patch v1.1 Great first patch but we'll need another iteration before this can land: > bool ValidateGLSLIdentifier(const nsAString& name, const char *info); >+ PRBool ValidateGLSLCharacter(unsigned char c); >+ PRBool ValidateGLSLCharacterSet(const nsAString& charset); A few comments already at this point: * we're moving away from PRBool, in favor of standard bools. Please use bool in all new code. Old code will be replaced all at once soon, see for instance: http://blog.mozilla.com/mwu/2011/07/28/the-twelve-booleans-of-mozilla/ * 'charset' is a bit misleading here, as this parameter is not the character set we're checking for, it's the string being checked. So I'd rename this parameter to just 'string'. * There's a discrepancy between the names ValidateGLSLCharacter and ValidateGLSLCharacterSet: in the first case, the name tells what is being checked, while in the second one, the name tells what it's checked for. Maybe rename ValidateGLSLCharacterSet to ValidateGLSLString (and then, sorry for the bad suggestion in comment 0). * GLSL identifiers are a special case of strings, so why not have ValidateGLSLIdentifier itself call ValidateGLSLString? So that GetUniformLocation and GetAttribLocation wouldn't have to call both. Otherwise it's a bit confusing: "hmm, should I call ValidateGLSLIdentifier or ValidateGLSLString?" * our Validate... functions tend to take a "info" parameter (see ValidateGLSLIdentifier) and call ErrorInvalidXxx themselves. I suggest having ValidateGLSLString behave the same way :-) This helps factor out a bit more code. >+PRBool WebGLContext::ValidateGLSLCharacter(unsigned char c) >+{ >+ // Printing characters are valid except " $ ` @ \ ' DEL. >+ if (c >= 32 && c <= 126 && >+ c != '"' && c != '$' && c != '`' && c != '@' && c != '\\' && c != '\'') >+ { >+ return PR_TRUE; >+ } >+ >+ // Horizontal tab, line feed, vertical tab, form feed, carriage return are also valid. >+ if (c >= 9 && c <= 13) >+ { >+ return PR_TRUE; >+ } >+ >+ return PR_FALSE; >+} It's OK to copy code straight from the WebKit repository, but you need to add a comment just above this function explaining this clearly, and reproducing the copyright/license notice. I would suggest taking this to a separate header file, so it will be clear that the license block applies only to this function. Again, replace PRBool by bool, etc. >--- a/content/canvas/test/webgl/conformance/invalid-passed-params.html >+++ /dev/null Oops, you deleted this file :-) Undo please. Also, this patch doesn't seem to contain the changes to failing_tests_*.txt ?

(In reply to Doug Sherk from comment #3) > Created attachment 555273 [details] [diff] [review] > Patch v1.1 > > removed the failed test file > content/canvas/test/webgl/conformance/invalid-passed-parameters.html Oh! I get the misunderstanding now. Sorry. I didn't say that you should delete the test that is now passing! Otherwise we would not be protected from regressions in the future. What I meant was that you should edit content/canvas/test/webgl/failing_tests_*.txt and remove from each of these files the line 'conformance/invalid-passed-parameters.html' Sorry

> It's OK to copy code straight from the WebKit repository, but you need to > add a comment just above this function explaining this clearly, and > reproducing the copyright/license notice. I would suggest taking this to a > separate header file, so it will be clear that the license block applies > only to this function. You can find an example of doing this here: content/canvas/src/WebGLTexelConversions.h

I see, thanks for the info. I wasn't sure how to deal with the WebKit code and assumed we cited them somewhere else in the code, which I thought would be sufficient. I made a mistake on the failed test removal. I wasn't trying to delete the test itself, I thought I was in a directory with a list of failed tests :) I read your instructions incorrectly. Will fix everything mentioned.

A summary of what ended up happening: An extra vulnerability in bindAttribLocation was discovered and also patched with this. I also removed the check for a blank name passed into bindAttribLocation, since the check for a valid variable should be include that already (and the WebGL spec doesn't mention that this should be a source of a specific error). bjacob and I discussed the naming and design of the changes and decided that ValidateGLSLIdentifier() needed to be renamed, and a check for ValidateGLSLString() should be included with it.

Small mistake in last upload

http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=b79e3554f0cd with: try: -b do -p all -u mochitest-1 -t none --post-to-bugzilla Bug 680722

Comment on attachment 555577 [details] [diff] [review] Patch v1.3 +++ b/content/canvas/src/WebGLValidateStrings.h >+#ifndef WEBGLVALIDATESTRINGS_H_ >+#define WEBGLVALIDATESTRINGS_H_ >+ >+#ifdef __SUNPRO_CC >+#define __restrict >+#endif This SUNPRO chunk is unneeded, since your new header doesn't use __restrict. (The SUNPRO chunk exists in WebGLTexelConversions.h because that file *does* use "__restrict". See bug 673865 for details -- that's what added that chunk.)

Comment on attachment 555577 [details] [diff] [review] Patch v1.3 Review of attachment 555577 [details] [diff] [review]: ----------------------------------------------------------------- Great patch. I only see one last thing to change: ::: content/canvas/src/WebGLContext.h @@ +493,5 @@ > PRBool ValidateStencilParamsForDrawCall(); > > + bool ValidateGLSLVariableName(const nsAString& name, const char *info); > + bool ValidateGLSLCharacter(unsigned char c); > + bool ValidateGLSLString(const nsAString& string); I think that ValidateGLSLString should take a |info| parameter too, and should take care of calling ErrorInvalidXxx, like the other ValidateXxx functions do. Just for consistency. ::: content/canvas/src/WebGLContextGL.cpp @@ +4133,5 @@ > if (!GetConcreteObjectAndGLName("shaderSource: shader", sobj, &shader, &shadername)) > return NS_OK; > + > + if (!ValidateGLSLString(source)) > + return ErrorInvalidValue("shaderSource: invalid source string"); ...so here, with my proposed ValidateGLSLString change, calling ErrorInvalidValue would be done by ValidateGLSLString. Also, please be more specific in the warning message: do specifically say that the string contains an illegal character. ::: content/canvas/src/WebGLContextValidate.cpp @@ +332,2 @@ > { > + const PRUint32 maxSize = 255; Yep. For the record: as discussed in last week's WebGL conference call, we're going to restrict variable names to 255 characters (even if they contain multiple identifiers, like "x.y[z]"). @@ +337,5 @@ > return false; > } > + > + if (!ValidateGLSLString(name)) { > + ErrorInvalidValue("%s: invalid name string", info); ...and again, with my proposed ValidateGLSLString change, you wouldn't have to call ErrorInvalidValue again here. Also, please be more specific in the warning message: do specifically say that the string contains an illegal character.

(In reply to Daniel Holbert [:dholbert] from comment #11) > Comment on attachment 555577 [details] [diff] [review] > Patch v1.3 > > +++ b/content/canvas/src/WebGLValidateStrings.h > >+#ifndef WEBGLVALIDATESTRINGS_H_ > >+#define WEBGLVALIDATESTRINGS_H_ > >+ > >+#ifdef __SUNPRO_CC > >+#define __restrict > >+#endif > > This SUNPRO chunk is unneeded, since your new header doesn't use __restrict. > > (The SUNPRO chunk exists in WebGLTexelConversions.h because that file *does* > use "__restrict". See bug 673865 for details -- that's what added that > chunk.) Oh yes, that too. Good catch.

Concerns from bjacob and dholbert should now be addressed.

Comment on attachment 555613 [details] [diff] [review] Patch v1.4 Review of attachment 555613 [details] [diff] [review]: ----------------------------------------------------------------- Sorry I forgot to catch something important in my previous review. A nsAString is a unicode string. So CharAt(i) returns a PRUnichar, not a char: https://developer.mozilla.org/en/nsAString_internal#CharAt So ValidateGLSLCharacter needs to take a PRUnichar parameter, not a char. Aside from that, a minor nit below: ::: content/canvas/src/WebGLContextValidate.cpp @@ +347,5 @@ > +bool WebGLContext::ValidateGLSLString(const nsAString& string, const char *info) > +{ > + for (PRUint32 i = 0; i < string.Length(); ++i) { > + if (!ValidateGLSLCharacter(string.CharAt(i))) { > + ErrorInvalidValue("%s: invalid string character, '%c'", info, string.CharAt(i)); Nit: I have a feeling that "invalid string character" would be better rephrased as "string contains the illegal character".

Addresses unicode issue.

Comment on attachment 555621 [details] [diff] [review] Patch v1.5 Review of attachment 555621 [details] [diff] [review]: ----------------------------------------------------------------- Great! Thanks, looks perfect. Will run this on tryserver tomorrow.

(In reply to Benoit Jacob [:bjacob] from comment #17) > Comment on attachment 555621 [details] [diff] [review] > Patch v1.5 first, I *still* have conflicts in content/canvas/test/webgl/failing_tests_windows.txt which made me re-edit that file myself (so not sure whats wrong specifically, as it is the only file with a conflict) > Great! Thanks, looks perfect. Will run this on tryserver tomorrow. Why wait? http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=06be373c5c4b with: try: -b do -p all -u mochitest-1 -t none --post-to-bugzilla Bug 680722 Based on m-c: b354d9b3e9e1 (which was one push behind the tip, since I couldn't tell if some orange on the tip was intermittent or not)

Try run for b79e3554f0cd is complete. Detailed breakdown of the results available here: http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=b79e3554f0cd Results (out of 30 total builds): success: 30 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/Callek@gmail.com-b79e3554f0cd

Thanks again Callek! :D Looks like the first one passed, I'll check tomorrow what happened to the second (this does have to wait :P)

Try run for 06be373c5c4b is complete. Detailed breakdown of the results available here: http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=06be373c5c4b Results (out of 30 total builds): success: 29 warnings: 1 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/Callek@gmail.com-06be373c5c4b

Inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/1b09c80c46c3

backed out: http://hg.mozilla.org/mozilla-central/rev/e1d9d6120f84 due to bug 684312

Folded into http://hg.mozilla.org/integration/mozilla-inbound/rev/bdb0bff93ce8 as discussed on bug 683710