Bug 50929 – Must generate INVALID_VALUE errors for strings containing out-of-range characters

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

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.

[Zhenyao Mo]

Created attachment 77595 [details]
Patch

[Zhenyao Mo]

(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.

[Zhenyao Mo]

Created attachment 77596 [details]
Patch

[Zhenyao Mo]

(In reply to comment #3)
> Created an attachment (id=77596) [details]
> Patch

This fixed an error in the comment.

[Kenneth Russell]

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)?

[Kenneth Russell]

(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.

[Zhenyao Mo]

(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?

[Zhenyao Mo]

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)?

[Kenneth Russell]

(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.

[Zhenyao Mo]

Created attachment 78049 [details]
Patch

[Kenneth Russell]

Comment on attachment 78049 [details]
Patch

Looks great.

[Zhenyao Mo]

Committed r75175: <http://trac.webkit.org/changeset/75175>

[Stephen White]

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]

[Zhenyao Mo]

Add the failing test to test_expectations.txt in r75194.

Reopen this bug to investigate.

[Zhenyao Mo]

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.