Last Comment Bug 680722 - invalid-passed-params.html test fails because we don't check for illegal characters in strings
: invalid-passed-params.html test fails because we don't check for illegal char...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Doug Sherk (:drs) (inactive)
:
Mentors:
https://cvs.khronos.org/svn/repos/reg...
Depends on: 683710 684312
Blocks: webgl-conformance
  Show dependency treegraph
 
Reported: 2011-08-20 19:45 PDT by Benoit Jacob [:bjacob] (mostly away)
Modified: 2011-09-09 20:06 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1.0 (5.46 KB, patch)
2011-08-23 16:26 PDT, Doug Sherk (:drs) (inactive)
no flags Details | Diff | Review
Patch v1.1 (16.21 KB, patch)
2011-08-23 18:34 PDT, Doug Sherk (:drs) (inactive)
jacob.benoit.1: review-
Details | Diff | Review
Patch v1.2 (9.48 KB, patch)
2011-08-24 15:25 PDT, Doug Sherk (:drs) (inactive)
no flags Details | Diff | Review
Patch v1.3 (12.04 KB, patch)
2011-08-24 15:59 PDT, Doug Sherk (:drs) (inactive)
jacob.benoit.1: review-
Details | Diff | Review
Patch v1.4 (12.03 KB, patch)
2011-08-24 18:20 PDT, Doug Sherk (:drs) (inactive)
jacob.benoit.1: review-
Details | Diff | Review
Patch v1.5 (12.04 KB, patch)
2011-08-24 19:34 PDT, Doug Sherk (:drs) (inactive)
jacob.benoit.1: review+
Details | Diff | Review

Description Benoit Jacob [:bjacob] (mostly away) 2011-08-20 19:45:00 PDT
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
Comment 1 Doug Sherk (:drs) (inactive) 2011-08-22 13:05:34 PDT
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
Comment 2 Doug Sherk (:drs) (inactive) 2011-08-23 16:26:51 PDT
Created attachment 555244 [details] [diff] [review]
Patch v1.0

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
Comment 3 Doug Sherk (:drs) (inactive) 2011-08-23 18:34:55 PDT
Created attachment 555273 [details] [diff] [review]
Patch v1.1

removed the failed test file content/canvas/test/webgl/conformance/invalid-passed-parameters.html
Comment 4 Benoit Jacob [:bjacob] (mostly away) 2011-08-23 19:21:25 PDT
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 ?
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2011-08-23 19:23:28 PDT
(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
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2011-08-23 19:25:52 PDT
> 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
Comment 7 Doug Sherk (:drs) (inactive) 2011-08-23 20:41:38 PDT
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.
Comment 8 Doug Sherk (:drs) (inactive) 2011-08-24 15:25:01 PDT
Created attachment 555567 [details] [diff] [review]
Patch v1.2

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.
Comment 9 Doug Sherk (:drs) (inactive) 2011-08-24 15:59:12 PDT
Created attachment 555577 [details] [diff] [review]
Patch v1.3

Small mistake in last upload
Comment 10 Justin Wood (:Callek) 2011-08-24 16:48:02 PDT
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 11 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-08-24 17:21:08 PDT
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 12 Benoit Jacob [:bjacob] (mostly away) 2011-08-24 18:05:55 PDT
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.
Comment 13 Benoit Jacob [:bjacob] (mostly away) 2011-08-24 18:07:11 PDT
(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.
Comment 14 Doug Sherk (:drs) (inactive) 2011-08-24 18:20:29 PDT
Created attachment 555613 [details] [diff] [review]
Patch v1.4

Concerns from bjacob and dholbert should now be addressed.
Comment 15 Benoit Jacob [:bjacob] (mostly away) 2011-08-24 19:11:47 PDT
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".
Comment 16 Doug Sherk (:drs) (inactive) 2011-08-24 19:34:32 PDT
Created attachment 555621 [details] [diff] [review]
Patch v1.5

Addresses unicode issue.
Comment 17 Benoit Jacob [:bjacob] (mostly away) 2011-08-24 19:40:36 PDT
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.
Comment 18 Justin Wood (:Callek) 2011-08-24 20:48:24 PDT
(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)
Comment 19 Mozilla RelEng Bot 2011-08-24 21:01:06 PDT
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
Comment 20 Doug Sherk (:drs) (inactive) 2011-08-24 21:08:26 PDT
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)
Comment 21 Mozilla RelEng Bot 2011-08-25 00:51:06 PDT
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
Comment 22 Benoit Jacob [:bjacob] (mostly away) 2011-08-25 11:30:33 PDT
Inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/1b09c80c46c3
Comment 23 Ed Morley [:emorley] 2011-08-25 18:33:19 PDT
http://hg.mozilla.org/mozilla-central/rev/1b09c80c46c3
Comment 24 Benoit Jacob [:bjacob] (mostly away) 2011-09-02 12:44:36 PDT
backed out: http://hg.mozilla.org/mozilla-central/rev/e1d9d6120f84
due to bug 684312
Comment 25 Benoit Jacob [:bjacob] (mostly away) 2011-09-07 14:20:28 PDT
Folded into http://hg.mozilla.org/integration/mozilla-inbound/rev/bdb0bff93ce8 as discussed on bug 683710
Comment 26 Benoit Jacob [:bjacob] (mostly away) 2011-09-09 20:06:53 PDT
http://hg.mozilla.org/mozilla-central/rev/d04d43c81294

Note You need to log in before you can comment on or make changes to this bug.