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
Product: WebKit
Classification: Unclassified
Component: WebGL
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Zhenyao Mo
:
Depends on:
Blocks: 52022 52390
  Show dependency treegraph
 
Reported: 2010-12-13 09:55 PST by Kenneth Russell
Modified: 2011-01-13 12:09 PST (History)
6 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 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 Zhenyao Mo 2010-12-28 17:41:22 PST
Created attachment 77595 [details]
Patch
Comment 2 Zhenyao Mo 2010-12-28 17:45:08 PST
(In reply to comment #1)
> Created an attachment (id=77595) [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 Zhenyao Mo 2010-12-28 17:48:51 PST
Created attachment 77596 [details]
Patch
Comment 4 Zhenyao Mo 2010-12-28 17:49:21 PST
(In reply to comment #3)
> Created an attachment (id=77596) [details]
> Patch

This fixed an error in the comment.
Comment 5 Kenneth Russell 2011-01-04 12:10:19 PST
Comment on attachment 77596 [details]
Patch

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 Kenneth Russell 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 Zhenyao Mo 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 Zhenyao Mo 2011-01-04 12:25:15 PST
Will do.

(In reply to comment #5)
> (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 9 Kenneth Russell 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 Zhenyao Mo 2011-01-05 14:47:57 PST
Created attachment 78049 [details]
Patch
Comment 11 Kenneth Russell 2011-01-06 10:25:49 PST
Comment on attachment 78049 [details]
Patch

Looks great.
Comment 12 Zhenyao Mo 2011-01-06 11:34:31 PST
Committed r75175: <http://trac.webkit.org/changeset/75175>
Comment 13 Stephen White 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 Zhenyao Mo 2011-01-06 13:43:33 PST
Add the failing test to test_expectations.txt in r75194.

Reopen this bug to investigate.
Comment 15 Zhenyao Mo 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.