Skip to content

A
angle
Commit b4fd4628 by Kenneth Russell Committed by Commit Bot
Options

Revise WebGL's shaderSource validation.

Per discussion in the WebGL working group, shaderSource no longer generates INVALID_VALUE for sources containing characters outside the ESSL character set. Compilation and/or linking is still specified to fail when illegal constructs are used. With this change, https://github.com/KhronosGroup/WebGL/pull/3206 passes with the passthrough command decoder. Revise WebGL compatibility tests to follow the new rules. Bug: chromium:1171506 Change-Id: Id132e0b64fa94b373f2732acf2a7071f38f0d4ff Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/2654264Reviewed-by: 's avatarGeoff Lang <geofflang@chromium.org> Reviewed-by: 's avatarJonah Ryan-Davis <jonahr@google.com> Commit-Queue: Kenneth Russell <kbr@chromium.org>
parent 80a4223e
Showing with 20 additions and 179 deletions
src/libANGLE/validationES2.cpp
...@@ -805,130 +805,6 @@ bool IsValidESSLString(const char *str, size_t len) ...@@ -805,130 +805,6 @@ bool IsValidESSLString(const char *str, size_t len)
return true; return true;
} }
bool IsValidESSLShaderSourceString(const char *str, size_t len, bool lineContinuationAllowed)
{
enum class ParseState
{
// Have not seen an ASCII non-whitespace character yet on
// this line. Possible that we might see a preprocessor
// directive.
BEGINING_OF_LINE,
// Have seen at least one ASCII non-whitespace character
// on this line.
MIDDLE_OF_LINE,
// Handling a preprocessor directive. Passes through all
// characters up to the end of the line. Disables comment
// processing.
IN_PREPROCESSOR_DIRECTIVE,
// Handling a single-line comment. The comment text is
// replaced with a single space.
IN_SINGLE_LINE_COMMENT,
// Handling a multi-line comment. Newlines are passed
// through to preserve line numbers.
IN_MULTI_LINE_COMMENT
};
ParseState state = ParseState::BEGINING_OF_LINE;
size_t pos = 0;
while (pos < len)
{
char c = str[pos];
char next = pos + 1 < len ? str[pos + 1] : 0;
// Check for newlines
if (c == '\n' || c == '\r')
{
if (state != ParseState::IN_MULTI_LINE_COMMENT)
{
state = ParseState::BEGINING_OF_LINE;
}
pos++;
continue;
}
switch (state)
{
case ParseState::BEGINING_OF_LINE:
if (c == ' ')
{
// Maintain the BEGINING_OF_LINE state until a non-space is seen
pos++;
}
else if (c == '#')
{
state = ParseState::IN_PREPROCESSOR_DIRECTIVE;
pos++;
}
else
{
// Don't advance, re-process this character with the MIDDLE_OF_LINE state
state = ParseState::MIDDLE_OF_LINE;
}
break;
case ParseState::MIDDLE_OF_LINE:
if (c == '/' && next == '/')
{
state = ParseState::IN_SINGLE_LINE_COMMENT;
pos++;
}
else if (c == '/' && next == '*')
{
state = ParseState::IN_MULTI_LINE_COMMENT;
pos++;
}
else if (lineContinuationAllowed && c == '\\' && (next == '\n' || next == '\r'))
{
// Skip line continuation characters
}
else if (!IsValidESSLCharacter(c))
{
return false;
}
pos++;
break;
case ParseState::IN_PREPROCESSOR_DIRECTIVE:
// Line-continuation characters may not be permitted.
// Otherwise, just pass it through. Do not parse comments in this state.
if (!lineContinuationAllowed && c == '\\')
{
return false;
}
pos++;
break;
case ParseState::IN_SINGLE_LINE_COMMENT:
// Line-continuation characters are processed before comment processing.
// Advance string if a new line character is immediately behind
// line-continuation character.
if (c == '\\' && (next == '\n' || next == '\r'))
{
pos++;
}
pos++;
break;
case ParseState::IN_MULTI_LINE_COMMENT:
if (c == '*' && next == '/')
{
state = ParseState::MIDDLE_OF_LINE;
pos++;
}
pos++;
break;
}
}
return true;
}
bool ValidateWebGLNamePrefix(const Context *context, const GLchar *name) bool ValidateWebGLNamePrefix(const Context *context, const GLchar *name)
{ {
ASSERT(context->isWebGL()); ASSERT(context->isWebGL());
...@@ -4963,25 +4839,6 @@ bool ValidateShaderSource(const Context *context, ...@@ -4963,25 +4839,6 @@ bool ValidateShaderSource(const Context *context,
return false; return false;
} }
// The WebGL spec (section 6.20) disallows strings containing invalid ESSL characters for
// shader-related entry points
if (context->getExtensions().webglCompatibility)
{
for (GLsizei i = 0; i < count; i++)
{
size_t len =
(length && length[i] >= 0) ? static_cast<size_t>(length[i]) : strlen(string[i]);
// Backslash as line-continuation is allowed in WebGL 2.0.
if (!IsValidESSLShaderSourceString(string[i], len,
context->getClientVersion() >= ES_3_0))
{
context->validationError(GL_INVALID_VALUE, kShaderSourceInvalidCharacters);
return false;
}
}
}
Shader *shaderObject = GetValidShader(context, shader); Shader *shaderObject = GetValidShader(context, shader);
if (!shaderObject) if (!shaderObject)
{ {
......
src/tests/gl_tests/WebGLCompatibilityTest.cpp
...@@ -2156,39 +2156,27 @@ void main() ...@@ -2156,39 +2156,27 @@ void main()
for (char invalidChar : invalidSet) for (char invalidChar : invalidSet)
{ {
std::string invalidAttribName = validAttribName + invalidChar; std::string invalidAttribName = validAttribName + invalidChar;
const char *invalidVert[] = { std::string invalidVert = "attribute float ";
"attribute float ", invalidVert += invalidAttribName;
invalidAttribName.c_str(), invalidVert += R"(;,
R"(;,
void main(), void main(),
{, {,
gl_Position = vec4(1.0);, gl_Position = vec4(1.0);,
})", })";
}; GLuint program = CompileProgram(invalidVert.c_str(), essl1_shaders::fs::Red());
EXPECT_EQ(0u, program);
GLuint shader = glCreateShader(GL_VERTEX_SHADER);
glShaderSource(shader, static_cast<GLsizei>(ArraySize(invalidVert)), invalidVert, nullptr);
EXPECT_GL_ERROR(GL_INVALID_VALUE);
glDeleteShader(shader);
} }
} }
// Test that line continuation is handled correctly when valdiating shader source // Test that line continuation is handled correctly when validating shader source
TEST_P(WebGLCompatibilityTest, ShaderSourceLineContinuation) TEST_P(WebGLCompatibilityTest, ShaderSourceLineContinuation)
{ {
// Verify that a line continuation character (i.e. backslash) cannot be used // With recent changes to WebGL's shader source validation in
// within a preprocessor directive in a ES2 context. // https://github.com/KhronosGroup/WebGL/pull/3206 and follow-ons,
ANGLE_SKIP_TEST_IF(getClientMajorVersion() >= 3); // the backslash character can be used in both WebGL 1.0 and 2.0
// contexts.
const char *validVert = const char *validVert =
R"(#define foo this is a test
precision mediump float;
void main()
{
gl_Position = vec4(1.0);
})";
const char *invalidVert =
R"(#define foo this \ R"(#define foo this \
is a test is a test
precision mediump float; precision mediump float;
...@@ -2197,13 +2185,9 @@ void main() ...@@ -2197,13 +2185,9 @@ void main()
gl_Position = vec4(1.0); gl_Position = vec4(1.0);
})"; })";
GLuint shader = glCreateShader(GL_VERTEX_SHADER); GLuint program = CompileProgram(validVert, essl1_shaders::fs::Red());
glShaderSource(shader, 1, &validVert, nullptr); EXPECT_NE(0u, program);
EXPECT_GL_NO_ERROR(); glDeleteProgram(program);
glShaderSource(shader, 1, &invalidVert, nullptr);
EXPECT_GL_ERROR(GL_INVALID_VALUE);
glDeleteShader(shader);
} }
// Test that line continuation is handled correctly when valdiating shader source // Test that line continuation is handled correctly when valdiating shader source
...@@ -2231,12 +2215,12 @@ oo = 1.0; ...@@ -2231,12 +2215,12 @@ oo = 1.0;
gl_Position = vec4(foo); gl_Position = vec4(foo);
})"; })";
GLuint shader = glCreateShader(GL_VERTEX_SHADER); GLuint program = CompileProgram(validVert, essl3_shaders::fs::Red());
glShaderSource(shader, 1, &validVert, nullptr); EXPECT_NE(0u, program);
EXPECT_GL_NO_ERROR(); glDeleteProgram(program);
glShaderSource(shader, 1, &invalidVert, nullptr);
EXPECT_GL_ERROR(GL_INVALID_VALUE); program = CompileProgram(invalidVert, essl3_shaders::fs::Red());
glDeleteShader(shader); EXPECT_EQ(0u, program);
} }
// Tests bindAttribLocations for reserved prefixes and length limits // Tests bindAttribLocations for reserved prefixes and length limits
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment