Strip comments from shader before checking for invalid characters

BUG=101870
TEST=unit tests


Review URL: http://codereview.chromium.org/8404029

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@107631 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 223 insertions, 1 deletions

diff --git a/gpu/command_buffer/service/gles2_cmd_decoder.cc b/gpu/command_buffer/service/gles2_cmd_decoder.cc
index 081d371..c525908 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder.cc
@@ -4748,7 +4748,8 @@ GLuint GLES2DecoderImpl::DoGetMaxValueInBufferCHROMIUM(
 error::Error GLES2DecoderImpl::ShaderSourceHelper(
     GLuint client_id, const char* data, uint32 data_size) {
   std::string str(data, data + data_size);
-  if (!StringIsValidForGLES(str.c_str())) {
+  std::string stripped = ShaderManager::StripComments(str);
+  if (!StringIsValidForGLES(stripped.c_str())) {
     SetGLError(GL_INVALID_VALUE, "glShaderSource: Invalid character");
     return error::kNoError;
   }
diff --git a/gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc b/gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc
index 6b58fea..286731f 100644
--- a/gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc
+++ b/gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc
@@ -1254,6 +1254,16 @@ TEST_F(GLES2DecoderTest, ShaderSourceBucketInvalidArgs) {
   EXPECT_EQ(GL_INVALID_VALUE, GetGLError());
 }
 
+TEST_F(GLES2DecoderTest, ShaderSourceStripComments) {
+  const uint32 kInBucketId = 123;
+  const char kSource[] = "hello/*te\ast*/world//a\ab";
+  SetBucketAsCString(kInBucketId, kSource);
+  ShaderSourceBucket cmd;
+  cmd.Init(client_shader_id_, kInBucketId);
+  EXPECT_EQ(error::kNoError, ExecuteCmd(cmd));
+  EXPECT_EQ(GL_NO_ERROR, GetGLError());
+}
+
 TEST_F(GLES2DecoderTest, GenerateMipmapWrongFormatsFails) {
   EXPECT_CALL(*gl_, GenerateMipmapEXT(_))
        .Times(0);
diff --git a/gpu/command_buffer/service/shader_manager.cc b/gpu/command_buffer/service/shader_manager.cc
index b230581..caac494 100644
--- a/gpu/command_buffer/service/shader_manager.cc
+++ b/gpu/command_buffer/service/shader_manager.cc
@@ -4,6 +4,7 @@
 
 #include "gpu/command_buffer/service/shader_manager.h"
 #include "base/logging.h"
+#include "base/string_util.h"
 
 namespace gpu {
 namespace gles2 {
@@ -151,6 +152,192 @@ void ShaderManager::UnuseShader(ShaderManager::ShaderInfo* info) {
   RemoveShaderInfoIfUnused(info);
 }
 
+namespace {
+
+// Strips comments from shader text. This allows non-ASCII characters
+// to be used in comments without potentially breaking OpenGL
+// implementations not expecting characters outside the GLSL ES set.
+class Stripper {
+ public:
+  Stripper(const std::string& str)
+    : parse_state_(kBeginningOfLine)
+    , source_string_(str)
+    , length_(str.length())
+    , position_(0) {
+    Parse();
+  }
+
+  const std::string& result() {
+    return result_;
+  }
+
+ private:
+  bool HasMoreCharacters() {
+    return position_ < length_;
+  }
+
+  void Parse() {
+    while (HasMoreCharacters()) {
+      Process(Current());
+      // Process() might Advance the position.
+      if (HasMoreCharacters()) {
+        Advance();
+      }
+    }
+  }
+
+  void Process(char);
+
+  bool Peek(char* character) {
+    DCHECK(character);
+    if (position_ + 1 >= length_) {
+      return false;
+    }
+    *character = source_string_[position_ + 1];
+    return true;
+  }
+
+  char Current() {
+    DCHECK_LT(position_, length_);
+    return source_string_[position_];
+  }
+
+  void Advance() {
+    ++position_;
+  }
+
+  bool IsNewline(char character) {
+    // Don't attempt to canonicalize newline related characters.
+    return (character == '\n' || character == '\r');
+  }
+
+  void Emit(char character) {
+    result_.push_back(character);
+  }
+
+  enum ParseState {
+    // Have not seen an ASCII non-whitespace character yet on
+    // this line. Possible that we might see a preprocessor
+    // directive.
+    kBeginningOfLine,
+
+    // Have seen at least one ASCII non-whitespace character
+    // on this line.
+    kMiddleOfLine,
+
+    // Handling a preprocessor directive. Passes through all
+    // characters up to the end of the line. Disables comment
+    // processing.
+    kInPreprocessorDirective,
+
+    // Handling a single-line comment. The comment text is
+    // replaced with a single space.
+    kInSingleLineComment,
+
+    // Handling a multi-line comment. Newlines are passed
+    // through to preserve line numbers.
+    kInMultiLineComment
+  };
+
+  ParseState parse_state_;
+  std::string source_string_;
+  unsigned length_;
+  unsigned position_;
+  std::string result_;
+};
+
+void Stripper::Process(char c) {
+  if (IsNewline(c)) {
+    // No matter what state we are in, pass through newlines
+    // so we preserve line numbers.
+    Emit(c);
+
+    if (parse_state_ != kInMultiLineComment)
+      parse_state_ = kBeginningOfLine;
+
+    return;
+  }
+
+  char temp = 0;
+  switch (parse_state_) {
+    case kBeginningOfLine:
+      if (IsAsciiWhitespace(c)) {
+        Emit(c);
+        break;
+      }
+
+      if (c == '#') {
+        parse_state_ = kInPreprocessorDirective;
+        Emit(c);
+        break;
+      }
+
+      // Transition to normal state and re-handle character.
+      parse_state_ = kMiddleOfLine;
+      Process(c);
+      break;
+
+    case kMiddleOfLine:
+      if (c == '/' && Peek(&temp)) {
+        if (temp == '/') {
+          parse_state_ = kInSingleLineComment;
+          Emit(' ');
+          Advance();
+          break;
+        }
+
+        if (temp == '*') {
+          parse_state_ = kInMultiLineComment;
+          // Emit the comment start in case the user has
+          // an unclosed comment and we want to later
+          // signal an error.
+          Emit('/');
+          Emit('*');
+          Advance();
+          break;
+        }
+      }
+
+      Emit(c);
+      break;
+
+    case kInPreprocessorDirective:
+      // No matter what the character is, just pass it
+      // through. Do not Parse comments in this state. This
+      // might not be the right thing to do long term, but it
+      // should handle the #error preprocessor directive.
+      Emit(c);
+      break;
+
+    case kInSingleLineComment:
+      // The newline code at the top of this function takes care
+      // of resetting our state when we get out of the
+      // single-line comment. Swallow all other characters.
+      break;
+
+    case kInMultiLineComment:
+      if (c == '*' && Peek(&temp) && temp == '/') {
+        Emit('*');
+        Emit('/');
+        parse_state_ = kMiddleOfLine;
+        Advance();
+        break;
+      }
+
+      // Swallow all other characters. Unclear whether we may
+      // want or need to just Emit a space per character to try
+      // to preserve column numbers for debugging purposes.
+      break;
+  }
+}
+
+}  // anonymous namespace
+
+std::string ShaderManager::StripComments(const std::string source) {
+  Stripper stripper(source);
+  return stripper.result();
+}
+
 }  // namespace gles2
 }  // namespace gpu
 
diff --git a/gpu/command_buffer/service/shader_manager.h b/gpu/command_buffer/service/shader_manager.h
index 4168152..c8f2b56 100644
--- a/gpu/command_buffer/service/shader_manager.h
+++ b/gpu/command_buffer/service/shader_manager.h
@@ -150,6 +150,9 @@ class ShaderManager {
   // Check if a ShaderInfo is owned by this ShaderManager.
   bool IsOwned(ShaderInfo* info);
 
+  // Strips the comments from a shader.
+  static std::string StripComments(const std::string source);
+
  private:
   // Info for each shader by service side shader Id.
   typedef base::hash_map<GLuint, ShaderInfo::Ref> ShaderInfoMap;
diff --git a/gpu/command_buffer/service/shader_manager_unittest.cc b/gpu/command_buffer/service/shader_manager_unittest.cc
index f927e26..30f87d7 100644
--- a/gpu/command_buffer/service/shader_manager_unittest.cc
+++ b/gpu/command_buffer/service/shader_manager_unittest.cc
@@ -245,6 +245,27 @@ TEST_F(ShaderManagerTest, ShaderInfoUseCount) {
   EXPECT_TRUE(info2 == NULL);
 }
 
+TEST_F(ShaderManagerTest, StripComments) {
+  const char* kSource = ""
+      "// this is a single line comment\n"
+      "this is not a comment\n"
+      "/* this is\n"
+      "a multi line\n"
+      "comment */\n"
+      "this is not a comment\n"
+      "this is an /* inline */ comment";
+  const char* kExpected = ""
+      " \n"
+      "this is not a comment\n"
+      "/*\n"
+      "\n"
+      "*/\n"
+      "this is not a comment\n"
+      "this is an /**/ comment";
+  std::string actual = ShaderManager::StripComments(kSource);
+  EXPECT_STREQ(kExpected, actual.c_str());
+}
+
 }  // namespace gles2
 }  // namespace gpu