Fix WebGL validation of characters in shader source strings.
Shader source strings are allowed invalid ESSL characters when they are in
comments. Added a simple comment parser to determine which characters
should be validated.
BUG=angleproject:2093
Change-Id: If78a4ecbd61f1700fc18dcb844f3de03314a6a39
Reviewed-on: https://chromium-review.googlesource.com/578567
Commit-Queue: Geoff Lang <geofflang@chromium.org>
Reviewed-by: Jamie Madill <jmadill@chromium.org>
diff --git a/src/libANGLE/validationES2.cpp b/src/libANGLE/validationES2.cpp
index e817390..6598c58 100644
--- a/src/libANGLE/validationES2.cpp
+++ b/src/libANGLE/validationES2.cpp
@@ -782,11 +782,11 @@
// Return true if a character belongs to the ASCII subset as defined in GLSL ES 1.0 spec section
// 3.1.
-bool IsValidESSLCharacter(unsigned char c, bool allowBackslash)
+bool IsValidESSLCharacter(unsigned char c)
{
// Printing characters are valid except " $ ` @ \ ' DEL.
- if (c >= 32 && c <= 126 && c != '"' && c != '$' && c != '`' && c != '@' &&
- (allowBackslash || c != '\\') && c != '\'')
+ if (c >= 32 && c <= 126 && c != '"' && c != '$' && c != '`' && c != '@' && c != '\\' &&
+ c != '\'')
{
return true;
}
@@ -800,11 +800,11 @@
return false;
}
-bool IsValidESSLString(const char *str, size_t len, bool allowBackslash)
+bool IsValidESSLString(const char *str, size_t len)
{
for (size_t i = 0; i < len; i++)
{
- if (!IsValidESSLCharacter(str[i], allowBackslash))
+ if (!IsValidESSLCharacter(str[i]))
{
return false;
}
@@ -813,6 +813,126 @@
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:
+ // No matter what the character is, just pass it
+ // through. Do not parse comments in this state.
+ 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;
+}
+
} // anonymous namespace
bool ValidateES2TexImageParameters(Context *context,
@@ -2745,8 +2865,7 @@
// The WebGL spec (section 6.20) disallows strings containing invalid ESSL characters for
// shader-related entry points
- if (context->getExtensions().webglCompatibility &&
- !IsValidESSLString(name, strlen(name), false))
+ if (context->getExtensions().webglCompatibility && !IsValidESSLString(name, strlen(name)))
{
ANGLE_VALIDATION_ERR(context, InvalidValue(), InvalidNameCharacters);
return false;
@@ -4076,8 +4195,7 @@
// The WebGL spec (section 6.20) disallows strings containing invalid ESSL characters for
// shader-related entry points
- if (context->getExtensions().webglCompatibility &&
- !IsValidESSLString(name, strlen(name), false))
+ if (context->getExtensions().webglCompatibility && !IsValidESSLString(name, strlen(name)))
{
ANGLE_VALIDATION_ERR(context, InvalidValue(), InvalidNameCharacters);
return false;
@@ -4809,8 +4927,7 @@
{
// The WebGL spec (section 6.20) disallows strings containing invalid ESSL characters for
// shader-related entry points
- if (context->getExtensions().webglCompatibility &&
- !IsValidESSLString(name, strlen(name), false))
+ if (context->getExtensions().webglCompatibility && !IsValidESSLString(name, strlen(name)))
{
ANGLE_VALIDATION_ERR(context, InvalidValue(), InvalidNameCharacters);
return false;
@@ -4969,8 +5086,7 @@
// The WebGL spec (section 6.20) disallows strings containing invalid ESSL characters for
// shader-related entry points
- if (context->getExtensions().webglCompatibility &&
- !IsValidESSLString(name, strlen(name), false))
+ if (context->getExtensions().webglCompatibility && !IsValidESSLString(name, strlen(name)))
{
ANGLE_VALIDATION_ERR(context, InvalidValue(), InvalidNameCharacters);
return false;
@@ -5197,10 +5313,12 @@
{
for (GLsizei i = 0; i < count; i++)
{
- size_t len = length ? static_cast<size_t>(length[i]) : strlen(string[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 (!IsValidESSLString(string[i], len, context->getClientVersion() >= ES_3_0))
+ if (!IsValidESSLShaderSourceString(string[i], len,
+ context->getClientVersion() >= ES_3_0))
{
ANGLE_VALIDATION_ERR(context, InvalidValue(), ShaderSourceInvalidCharacters);
return false;
diff --git a/src/tests/gl_tests/WebGLCompatibilityTest.cpp b/src/tests/gl_tests/WebGLCompatibilityTest.cpp
index eea9553..4ebafb7 100644
--- a/src/tests/gl_tests/WebGLCompatibilityTest.cpp
+++ b/src/tests/gl_tests/WebGLCompatibilityTest.cpp
@@ -892,11 +892,14 @@
"precision highp float;\n"
"uniform vec4 ";
frag += validUniformName;
+ // Insert illegal characters into comments
frag +=
";\n"
+ " // $ \" @ /*\n"
"void main()\n"
- "{\n"
- " gl_FragColor = vec4(1.0);\n"
+ "{/*\n"
+ " ` @ $\n"
+ " */gl_FragColor = vec4(1.0);\n"
"}\n";
ANGLE_GL_PROGRAM(program, vert, frag);
@@ -942,6 +945,39 @@
}
}
+// Test that line continuation is handled correctly when valdiating shader source
+TEST_P(WebGL2CompatibilityTest, ShaderSourceLineContinuation)
+{
+ const char *validVert =
+ "#version 300 es\n"
+ "precision mediump float;\n"
+ "\n"
+ "void main ()\n"
+ "{\n"
+ " float f\\\n"
+ "oo = 1.0;\n"
+ " gl_Position = vec4(foo);\n"
+ "}\n";
+
+ const char *invalidVert =
+ "#version 300 es\n"
+ "precision mediump float;\n"
+ "\n"
+ "void main ()\n"
+ "{\n"
+ " float f\\$\n"
+ "oo = 1.0;\n"
+ " gl_Position = vec4(foo);\n"
+ "}\n";
+
+ GLuint shader = glCreateShader(GL_VERTEX_SHADER);
+ glShaderSource(shader, 1, &validVert, nullptr);
+ EXPECT_GL_NO_ERROR();
+ glShaderSource(shader, 1, &invalidVert, nullptr);
+ EXPECT_GL_ERROR(GL_INVALID_VALUE);
+ glDeleteShader(shader);
+}
+
// Test the checks for OOB reads in the vertex buffers, instanced version
TEST_P(WebGL2CompatibilityTest, DrawArraysBufferOutOfBoundsInstanced)
{