The default bug view has changed. See this FAQ.

invalid-passed-params.html test fails because we don't check for illegal characters in strings

RESOLVED FIXED in mozilla9

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bjacob, Assigned: drs)

Tracking

Trunk
mozilla9
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 5 obsolete attachments)

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
(Assignee)

Comment 1

6 years ago
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
(Assignee)

Updated

6 years ago
Assignee: nobody → dsherk
(Assignee)

Comment 2

6 years ago
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
Attachment #555244 - Flags: review?(bjacob)
(Assignee)

Comment 3

6 years ago
Created attachment 555273 [details] [diff] [review]
Patch v1.1

removed the failed test file content/canvas/test/webgl/conformance/invalid-passed-parameters.html
Attachment #555244 - Attachment is obsolete: true
Attachment #555244 - Flags: review?(bjacob)
Attachment #555273 - Flags: review?(bjacob)
(Reporter)

Comment 4

6 years ago
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 ?
Attachment #555273 - Flags: review?(bjacob) → review-
(Reporter)

Comment 5

6 years ago
(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
(Reporter)

Comment 6

6 years ago
> 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
(Assignee)

Comment 7

6 years ago
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.
Version: unspecified → Trunk
(Assignee)

Comment 8

6 years ago
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.
Attachment #555273 - Attachment is obsolete: true
Attachment #555567 - Flags: review?(bjacob)
(Assignee)

Comment 9

6 years ago
Created attachment 555577 [details] [diff] [review]
Patch v1.3

Small mistake in last upload
Attachment #555567 - Attachment is obsolete: true
Attachment #555567 - Flags: review?(bjacob)
Attachment #555577 - Flags: review?(bjacob)
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.
Attachment #555577 - Flags: review?(bjacob) → review-
(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.
(Assignee)

Comment 14

6 years ago
Created attachment 555613 [details] [diff] [review]
Patch v1.4

Concerns from bjacob and dholbert should now be addressed.
Attachment #555577 - Attachment is obsolete: true
Attachment #555613 - Flags: review?(bjacob)
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".
Attachment #555613 - Flags: review?(bjacob) → review-
(Assignee)

Comment 16

6 years ago
Created attachment 555621 [details] [diff] [review]
Patch v1.5

Addresses unicode issue.
Attachment #555613 - Attachment is obsolete: true
Attachment #555621 - Flags: review?(bjacob)
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.
Attachment #555621 - Flags: review?(bjacob) → review+
(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

6 years ago
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
(Assignee)

Comment 20

6 years ago
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

6 years ago
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
http://hg.mozilla.org/mozilla-central/rev/1b09c80c46c3
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
(Reporter)

Updated

6 years ago
Depends on: 683710
Depends on: 684312
backed out: http://hg.mozilla.org/mozilla-central/rev/e1d9d6120f84
due to bug 684312
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Folded into http://hg.mozilla.org/integration/mozilla-inbound/rev/bdb0bff93ce8 as discussed on bug 683710
http://hg.mozilla.org/mozilla-central/rev/d04d43c81294
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.