Bug 50929 - Must generate INVALID_VALUE errors for strings containing out-of-range characters
: Must generate INVALID_VALUE errors for strings containing out-of-range charac...
Status: RESOLVED FIXED
: WebKit
WebGL
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
: 52022 52390
  Show dependency treegraph
 
Reported: 2010-12-13 09:55 PST by
Modified: 2011-01-13 12:09 PST (History)


Attachments
Patch (7.32 KB, patch)
2010-12-28 17:41 PST, Zhenyao Mo
no flags Review Patch | Details | Formatted Diff | Diff
Patch (7.38 KB, patch)
2010-12-28 17:48 PST, Zhenyao Mo
no flags Review Patch | Details | Formatted Diff | Diff
Patch (16.81 KB, patch)
2011-01-05 14:47 PST, Zhenyao Mo
kbr: review+
Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2010-12-13 09:55:06 PST
James Robinson points out in https://www.khronos.org/webgl/public-mailing-list/archives/1012/msg00133.html that there are several WebGL APIs accepting DOMString. Those mirroring OpenGL ES 2.0 APIs are bindAttribLocation(), getAttribLocation(), getUniformLocation(), and shaderSource(). The GLSL ES specification restricts the valid character range of the incoming shader strings, and therefore implicitly the names of attributes and uniforms.

The WebGL implementation needs to do a pre-pass over the incoming strings to these APIs to ensure that they do not contain out-of-range characters before passing them down to lower levels, and generate an INVALID_VALUE error if they do.
------- Comment #1 From 2010-12-28 17:41:22 PST -------
Created an attachment (id=77595) [details]
Patch
------- Comment #2 From 2010-12-28 17:45:08 PST -------
(In reply to comment #1)
> Created an attachment (id=77595) [details] [details]
> Patch

Will sync the test with khronos once reviewed.

Also, I already fixed the khronos shaders with invalid characters (un-intended), mostly due to invalid characters in Apple/Chromium copyright headers.

Note that it's impossible to test the behavior for getAttribLocation/getUniformLocation because with invalid characters won't be part of a legal variable name, so with/without this patch, the behaviors are the same.
------- Comment #3 From 2010-12-28 17:48:51 PST -------
Created an attachment (id=77596) [details]
Patch
------- Comment #4 From 2010-12-28 17:49:21 PST -------
(In reply to comment #3)
> Created an attachment (id=77596) [details] [details]
> Patch

This fixed an error in the comment.
------- Comment #5 From 2011-01-04 12:10:19 PST -------
(From update of attachment 77596 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=77596&action=review

> LayoutTests/fast/canvas/webgl/invalid-passed-params.html:76
> +shouldGenerateGLError(context, context.NO_ERROR, "context.shaderSource(shader, 'azAZ_09.+-/*%<>[](){}^|&~=!:;,?# ')");
> +shouldGenerateGLError(context, context.INVALID_VALUE, "context.shaderSource(shader, 'azAZ_09.+-/*%<>[](){}^|&~=!:;,?# $')");
> +shouldGenerateGLError(context, context.INVALID_VALUE, "context.shaderSource(shader, 'azAZ_09.+-/*%<>[](){}^|&~=!:;,?# `')");
> +shouldGenerateGLError(context, context.INVALID_VALUE, "context.shaderSource(shader, 'azAZ_09.+-/*%<>[](){}^|&~=!:;,?# @')");

Could you expand the failing test cases to include all of the forbidden printing characters? Also, it might be a good idea to make this syntactially valid GLSL. You could do so by enclosing the entire string in a comment.

> LayoutTests/fast/canvas/webgl/invalid-passed-params.html:83
> +debug("Test bindAttribLocation() with invalid characters");
> +var program = context.createProgram();
> +shouldGenerateGLError(context, context.NO_ERROR, "context.bindAttribLocation(program, 1, 'azAZ_09')");
> +shouldGenerateGLError(context, context.INVALID_VALUE, "context.bindAttribLocation(program, 1, 'azAZ_09$')");
> +shouldGenerateGLError(context, context.INVALID_VALUE, "context.bindAttribLocation(program, 1, 'azAZ_09`')");
> +shouldGenerateGLError(context, context.INVALID_VALUE, "context.bindAttribLocation(program, 1, 'azAZ_09@')");

Same comment about testing all of the forbidden characters. Also, would it be difficult to test the other affected entry points (getAttribLocation, getUniform)?
------- Comment #6 From 2011-01-04 12:12:41 PST -------
(In reply to comment #2)
> Note that it's impossible to test the behavior for getAttribLocation/getUniformLocation because with invalid characters won't be part of a legal variable name, so with/without this patch, the behaviors are the same.

I don't follow this; presumably non-compliant implementations might silently allow the illegal characters in and return -1 from getAttribLocation/getUniformLocation without generating an INVALID_VALUE error.
------- Comment #7 From 2011-01-04 12:24:51 PST -------
(In reply to comment #6)
> (In reply to comment #2)
> > Note that it's impossible to test the behavior for getAttribLocation/getUniformLocation because with invalid characters won't be part of a legal variable name, so with/without this patch, the behaviors are the same.
> 
> I don't follow this; presumably non-compliant implementations might silently allow the illegal characters in and return -1 from getAttribLocation/getUniformLocation without generating an INVALID_VALUE error.

My question is whether we should generate INVALID_VALUE or not.  Return -1 already indicates invalid input value.  If we pass invalid characters into GLES2, they will simply return -1 without an error.  Shouldn't the behavior be the same here?
------- Comment #8 From 2011-01-04 12:25:15 PST -------
Will do.

(In reply to comment #5)
> (From update of attachment 77596 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77596&action=review
> 
> > LayoutTests/fast/canvas/webgl/invalid-passed-params.html:76
> > +shouldGenerateGLError(context, context.NO_ERROR, "context.shaderSource(shader, 'azAZ_09.+-/*%<>[](){}^|&~=!:;,?# ')");
> > +shouldGenerateGLError(context, context.INVALID_VALUE, "context.shaderSource(shader, 'azAZ_09.+-/*%<>[](){}^|&~=!:;,?# $')");
> > +shouldGenerateGLError(context, context.INVALID_VALUE, "context.shaderSource(shader, 'azAZ_09.+-/*%<>[](){}^|&~=!:;,?# `')");
> > +shouldGenerateGLError(context, context.INVALID_VALUE, "context.shaderSource(shader, 'azAZ_09.+-/*%<>[](){}^|&~=!:;,?# @')");
> 
> Could you expand the failing test cases to include all of the forbidden printing characters? Also, it might be a good idea to make this syntactially valid GLSL. You could do so by enclosing the entire string in a comment.
> 
> > LayoutTests/fast/canvas/webgl/invalid-passed-params.html:83
> > +debug("Test bindAttribLocation() with invalid characters");
> > +var program = context.createProgram();
> > +shouldGenerateGLError(context, context.NO_ERROR, "context.bindAttribLocation(program, 1, 'azAZ_09')");
> > +shouldGenerateGLError(context, context.INVALID_VALUE, "context.bindAttribLocation(program, 1, 'azAZ_09$')");
> > +shouldGenerateGLError(context, context.INVALID_VALUE, "context.bindAttribLocation(program, 1, 'azAZ_09`')");
> > +shouldGenerateGLError(context, context.INVALID_VALUE, "context.bindAttribLocation(program, 1, 'azAZ_09@')");
> 
> Same comment about testing all of the forbidden characters. Also, would it be difficult to test the other affected entry points (getAttribLocation, getUniform)?
------- Comment #9 From 2011-01-04 12:59:51 PST -------
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #2)
> > > Note that it's impossible to test the behavior for getAttribLocation/getUniformLocation because with invalid characters won't be part of a legal variable name, so with/without this patch, the behaviors are the same.
> > 
> > I don't follow this; presumably non-compliant implementations might silently allow the illegal characters in and return -1 from getAttribLocation/getUniformLocation without generating an INVALID_VALUE error.
> 
> My question is whether we should generate INVALID_VALUE or not.  Return -1 already indicates invalid input value.  If we pass invalid characters into GLES2, they will simply return -1 without an error.  Shouldn't the behavior be the same here?

By the same argument the test for bindAttribLocation shouldn't be ensuring that INVALID_VALUE is generated for strings containing illegal characters. All of these entry points are implicitly dealing with GLSL ES compliant strings, and as has been pointed out there might be bugs in OpenGL ES drivers receiving utf-8 encoded strings with out-of-range characters. I think the best thing to do would be to make the test stringent, forbidding illegal identifiers from being passed to bindAttribLocation, getAttribLocation, and getUniformLocation, and loosen up the tests if other vendors complain for some reason that these requirements are too stringent.
------- Comment #10 From 2011-01-05 14:47:57 PST -------
Created an attachment (id=78049) [details]
Patch
------- Comment #11 From 2011-01-06 10:25:49 PST -------
(From update of attachment 78049 [details])
Looks great.
------- Comment #12 From 2011-01-06 11:34:31 PST -------
Committed r75175: <http://trac.webkit.org/changeset/75175>
------- Comment #13 From 2011-01-06 13:04:32 PST -------
This commit seems to have caused glsl-conformance layout test to start crashing:

base::debug::StackTrace::StackTrace() [0x83c65bb]
    base::(anonymous namespace)::StackDumpSignalHandler() [0x83b8fbd]
    0x4001c420
    byte_scan [0x8f8c49a]
    yylex_CPP [0x8f8bf5c]
    yylex() [0x8f902d5]
    yyparse() [0x8f9183f]
    PaParseStrings() [0x8f80fcc]
    TCompiler::compile() [0x8f6b1c1]
    ShCompile [0x8f69e80]
    WebKit::WebGraphicsContext3DDefaultImpl::angleValidateShaderSource() [0x8dee013]
    WebKit::WebGraphicsContext3DDefaultImpl::compileShader() [0x8def6b1]
    WebCore::WebGLRenderingContextInternal::compileShaderCallback() [0x826d19b]
    0x440549f2
    0x44231888
    0x43f43630
    0x439d0a79
    0x439c5e22
    v8::internal::Invoke() [0x844b703]
    WebCore::V8Proxy::handleOutOfMemory() [0x873de27]
------- Comment #14 From 2011-01-06 13:43:33 PST -------
Add the failing test to test_expectations.txt in r75194.

Reopen this bug to investigate.
------- Comment #15 From 2011-01-06 14:59:29 PST -------
After more investigation, it turns out that the test does not crash with command-buffer port, only with in-process-webgl.

There should be an existing bug in the in-process-webgl implementation that's exposed by this patch.

Closing this bug.  Create another bug to track down this issue.