Changeset 76063 in webkit


Ignore:
Timestamp:
Jan 18, 2011 2:11:48 PM (13 years ago)
Author:
kbr@google.com
Message:

2011-01-18 Kenneth Russell <​kbr@google.com>

Reviewed by David Levin.

Must strip comments from WebGL shaders before enforcing character set
​https://bugs.webkit.org/show_bug.cgi?id=52390

Strip comments from incoming shaders, preserving line numbers,
before validating that they conform to the ESSL character set.
Revert changes from ​http://trac.webkit.org/changeset/75735 which
allowed invalid characters to be passed to certain APIs.

Tested with WebGL layout tests, conformance test suite and several
WebGL demos in both Safari and Chromium.

  • html/canvas/WebGLRenderingContext.cpp: (WebCore::StripComments::StripComments::process): (WebCore::WebGLRenderingContext::shaderSource):

2011-01-18 Kenneth Russell <​kbr@google.com>

Reviewed by David Levin.

Must strip comments from WebGL shaders before enforcing character set
​https://bugs.webkit.org/show_bug.cgi?id=52390

Incorporated non-ASCII GLSL conformance tests from Khronos
repository. Updated and synchronized invalid-passed-params.html
with Khronos repository, undoing changes from
​http://trac.webkit.org/changeset/75735 .

  • fast/canvas/webgl/glsl-conformance-expected.txt:
  • fast/canvas/webgl/invalid-passed-params-expected.txt:
  • fast/canvas/webgl/invalid-passed-params.html:
  • fast/canvas/webgl/shaders/00_shaders.txt:
  • fast/canvas/webgl/shaders/misc: Added.
  • fast/canvas/webgl/shaders/misc/00_shaders.txt: Added.
  • fast/canvas/webgl/shaders/misc/non-ascii-comments.vert: Added.
  • fast/canvas/webgl/shaders/misc/non-ascii.vert: Added.
Location:
trunk
Files:
4 added
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r76058 r76063  
     12011-01-18  Kenneth Russell  <kbr@google.com>
     2
     3        Reviewed by David Levin.
     4
     5        Must strip comments from WebGL shaders before enforcing character set
     6        https://bugs.webkit.org/show_bug.cgi?id=52390
     7
     8        Incorporated non-ASCII GLSL conformance tests from Khronos
     9        repository. Updated and synchronized invalid-passed-params.html
     10        with Khronos repository, undoing changes from
     11        http://trac.webkit.org/changeset/75735 .
     12
     13        * fast/canvas/webgl/glsl-conformance-expected.txt:
     14        * fast/canvas/webgl/invalid-passed-params-expected.txt:
     15        * fast/canvas/webgl/invalid-passed-params.html:
     16        * fast/canvas/webgl/shaders/00_shaders.txt:
     17        * fast/canvas/webgl/shaders/misc: Added.
     18        * fast/canvas/webgl/shaders/misc/00_shaders.txt: Added.
     19        * fast/canvas/webgl/shaders/misc/non-ascii-comments.vert: Added.
     20        * fast/canvas/webgl/shaders/misc/non-ascii.vert: Added.
     21
    1222011-01-18  Chris Marrin  <cmarrin@apple.com>
    223

  • trunk/LayoutTests/fast/canvas/webgl/glsl-conformance-expected.txt

    r68806 r76063  
    9595PASS [shaders/implicit/ternary_ivec3_vec3.vert/fshader]: implicit cast of ivec3 to vec3 in ternary expression should fail
    9696PASS [shaders/implicit/ternary_ivec4_vec4.vert/fshader]: implicit cast of ivec4 to vec4 in ternary expression should fail
     97PASS [shaders/misc/non-ascii.vert/fshader]: Non ascii data in source should fail
     98PASS [shaders/misc/non-ascii-comments.vert/fshader]: Non ascii comments in source should succeed
    9799PASS [shaders/reserved/_webgl_field.vert/fshader]: use of reserved _webgl prefix as structure field should fail
    98100PASS [shaders/reserved/_webgl_function.vert/fshader]: use of reserved _webgl prefix as function name should fail

  • trunk/LayoutTests/fast/canvas/webgl/invalid-passed-params-expected.txt

    r75735 r76063  
    6464
    6565Test shaderSource() with invalid characters
     66PASS context.getError() is context.NO_ERROR
     67PASS context.getError() is context.INVALID_VALUE
     68PASS context.getError() is context.NO_ERROR
     69PASS context.getError() is context.INVALID_VALUE
     70PASS context.getError() is context.NO_ERROR
     71PASS context.getError() is context.INVALID_VALUE
     72PASS context.getError() is context.NO_ERROR
     73PASS context.getError() is context.INVALID_VALUE
     74PASS context.getError() is context.NO_ERROR
     75PASS context.getError() is context.INVALID_VALUE
     76PASS context.getError() is context.NO_ERROR
     77PASS context.getError() is context.INVALID_VALUE
     78
     79Test bindAttribLocation() with invalid characters
    6680PASS context.getError() is context.INVALID_VALUE
    6781PASS context.getError() is context.INVALID_VALUE
    6882PASS context.getError() is context.INVALID_VALUE
    69 
    70 Test bindAttribLocation() with invalid characters
    7183PASS context.getError() is context.INVALID_VALUE
    7284PASS context.getError() is context.INVALID_VALUE
    … …  
    7789PASS context.getError() is context.INVALID_VALUE
    7890PASS context.getError() is context.INVALID_VALUE
     91PASS context.getError() is context.INVALID_VALUE
     92PASS context.getError() is context.INVALID_VALUE
     93PASS context.getError() is context.INVALID_VALUE
    7994
    8095Test getUniformLocation() with invalid characters
     96PASS context.getError() is context.INVALID_VALUE
     97PASS context.getError() is context.INVALID_VALUE
     98PASS context.getError() is context.INVALID_VALUE
    8199PASS context.getError() is context.INVALID_VALUE
    82100PASS context.getError() is context.INVALID_VALUE

  • trunk/LayoutTests/fast/canvas/webgl/invalid-passed-params.html

    r75735 r76063  
    7878debug("");
    7979debug("Set up a program to test invalid characters");
    80 var invalidSet = ['$', '@', '\\'];
     80var invalidSet = ['"', '$', '`', '@', '\\', "'"];
    8181var validUniformName = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ_1234567890";
    8282var validAttribName = "abcdefghijklmnopqrstuvwxyz_ABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890";
    83 var validShaderSource = "uniform float " + validUniformName + ";\n"
    84                         + "varying float " + validAttribName + ";\n"
    85                         + "void main() {\n"
    86                         + validAttribName  + " = " + validUniformName + ";\n"
    87                         + "gl_Position = vec4(0.0, 0.0, 0.0, 1.0); }\n";
    88                         + "//.+-/*%<>[](){}^|&~=!:;,?# ";
     83function generateShaderSource(opt_invalidIdentifierChar, opt_invalidCommentChar) {
     84  var invalidIdentifierString = "";
     85  var invalidCommentString = "";
     86  if (opt_invalidIdentifierChar != undefined) {
     87    invalidIdentifierString += opt_invalidIdentifierChar;
     88  }
     89  if (opt_invalidCommentChar != undefined) {
     90    invalidCommentString += opt_invalidCommentChar;
     91  }
     92  return "uniform float " + validUniformName + invalidIdentifierString + ";\n"
     93                          + "varying float " + validAttribName + ";\n"
     94                          + "void main() {\n"
     95                          + validAttribName  + " = " + validUniformName + ";\n"
     96                          + "gl_Position = vec4(0.0, 0.0, 0.0, 1.0); }\n";
     97                          + "//.+-/*%<>[](){}^|&~=!:;,?# " + invalidCommentString;
     98}
    8999var vShader = context.createShader(context.VERTEX_SHADER);
    90 context.shaderSource(vShader, validShaderSource);
     100context.shaderSource(vShader, generateShaderSource());
    91101context.compileShader(vShader);
    92102shouldBe("context.getError()", "context.NO_ERROR");
    … …  
    114124debug("Test shaderSource() with invalid characters");
    115125for (var i = 0; i < invalidSet.length; ++i) {
    116   var invalidShaderSource = validShaderSource + "\n//" + invalidSet[i];
     126  var validShaderSource = generateShaderSource(undefined, invalidSet[i]);
     127  context.shaderSource(vShader, validShaderSource);
     128  shouldBe("context.getError()", "context.NO_ERROR");
     129  var invalidShaderSource = generateShaderSource(invalidSet[i], undefined);
    117130  context.shaderSource(vShader, invalidShaderSource);
    118131  shouldBe("context.getError()", "context.INVALID_VALUE");

  • trunk/LayoutTests/fast/canvas/webgl/shaders/00_shaders.txt

    r68806 r76063  
    11implicit/00_shaders.txt
     2misc/00_shaders.txt
    23reserved/00_shaders.txt
    34

  • trunk/Source/WebCore/ChangeLog

    r76057 r76063  
     12011-01-18  Kenneth Russell  <kbr@google.com>
     2
     3        Reviewed by David Levin.
     4
     5        Must strip comments from WebGL shaders before enforcing character set
     6        https://bugs.webkit.org/show_bug.cgi?id=52390
     7
     8        Strip comments from incoming shaders, preserving line numbers,
     9        before validating that they conform to the ESSL character set.
     10        Revert changes from http://trac.webkit.org/changeset/75735 which
     11        allowed invalid characters to be passed to certain APIs.
     12
     13        Tested with WebGL layout tests, conformance test suite and several
     14        WebGL demos in both Safari and Chromium.
     15
     16        * html/canvas/WebGLRenderingContext.cpp:
     17        (WebCore::StripComments::StripComments::process):
     18        (WebCore::WebGLRenderingContext::shaderSource):
     19
    1202011-01-18  Ryosuke Niwa  <rniwa@webkit.org>
    221

  • trunk/Source/WebCore/html/canvas/WebGLRenderingContext.cpp

    r75741 r76063  
    6363#include <wtf/OwnArrayPtr.h>
    6464#include <wtf/PassOwnArrayPtr.h>
     65#include <wtf/text/StringBuilder.h>
    6566
    6667namespace WebCore {
    … …  
    102103    // Return true if a character belongs to the ASCII subset as defined in
    103104    // GLSL ES 1.0 spec section 3.1.
    104     // We make exceptions for " ' `.
    105105    bool validateCharacter(unsigned char c)
    106106    {
    107107        // Printing characters are valid except " $ ` @ \ ' DEL.
    108108        if (c >= 32 && c <= 126
    109             && c != '$' && c != '@' && c != '\\')
     109            && c != '"' && c != '$' && c != '`' && c != '@' && c != '\\' && c != '\'')
    110110            return true;
    111111        // Horizontal tab, line feed, vertical tab, form feed, carriage return
    … …  
    116116    }
    117117
     118    // Strips comments from shader text. This allows non-ASCII characters
     119    // to be used in comments without potentially breaking OpenGL
     120    // implementations not expecting characters outside the GLSL ES set.
     121    class StripComments {
     122    public:
     123        StripComments(const String& str)
     124            : m_parseState(BeginningOfLine)
     125            , m_sourceString(str)
     126            , m_length(str.length())
     127            , m_position(0)
     128        {
     129            parse();
     130        }
     131
     132        String result()
     133        {
     134            return m_builder.toString();
     135        }
     136
     137    private:
     138        bool hasMoreCharacters()
     139        {
     140            return (m_position < m_length);
     141        }
     142
     143        void parse()
     144        {
     145            while (hasMoreCharacters()) {
     146                process(current());
     147                // process() might advance the position.
     148                if (hasMoreCharacters())
     149                    advance();
     150            }
     151        }
     152
     153        void process(UChar);
     154
     155        bool peek(UChar& character)
     156        {
     157            if (m_position + 1 >= m_length)
     158                return false;
     159            character = m_sourceString[m_position + 1];
     160            return true;
     161        }
     162
     163        UChar current()
     164        {
     165            ASSERT(m_position < m_length);
     166            return m_sourceString[m_position];
     167        }
     168
     169        void advance()
     170        {
     171            ++m_position;
     172        }
     173
     174        bool isNewline(UChar character)
     175        {
     176            // Don't attempt to canonicalize newline related characters.
     177            return (character == '\n' || character == '\r');
     178        }
     179
     180        void emit(UChar character)
     181        {
     182            m_builder.append(character);
     183        }
     184
     185        enum ParseState {
     186            // Have not seen an ASCII non-whitespace character yet on
     187            // this line. Possible that we might see a preprocessor
     188            // directive.
     189            BeginningOfLine,
     190
     191            // Have seen at least one ASCII non-whitespace character
     192            // on this line.
     193            MiddleOfLine,
     194
     195            // Handling a preprocessor directive. Passes through all
     196            // characters up to the end of the line. Disables comment
     197            // processing.
     198            InPreprocessorDirective,
     199
     200            // Handling a single-line comment. The comment text is
     201            // replaced with a single space.
     202            InSingleLineComment,
     203
     204            // Handling a multi-line comment. Newlines are passed
     205            // through to preserve line numbers.
     206            InMultiLineComment
     207        };
     208
     209        ParseState m_parseState;
     210        String m_sourceString;
     211        unsigned m_length;
     212        unsigned m_position;
     213        StringBuilder m_builder;
     214    };
     215
     216    void StripComments::process(UChar c)
     217    {
     218        if (isNewline(c)) {
     219            // No matter what state we are in, pass through newlines
     220            // so we preserve line numbers.
     221            emit(c);
     222
     223            if (m_parseState != InMultiLineComment)
     224                m_parseState = BeginningOfLine;
     225
     226            return;
     227        }
     228
     229        UChar temp = 0;
     230        switch (m_parseState) {
     231        case BeginningOfLine:
     232            if (WTF::isASCIISpace(c)) {
     233                emit(c);
     234                break;
     235            }
     236
     237            if (c == '#') {
     238                m_parseState = InPreprocessorDirective;
     239                emit(c);
     240                break;
     241            }
     242
     243            // Transition to normal state and re-handle character.
     244            m_parseState = MiddleOfLine;
     245            process(c);
     246            break;
     247
     248        case MiddleOfLine:
     249            if (c == '/' && peek(temp)) {
     250                if (temp == '/') {
     251                    m_parseState = InSingleLineComment;
     252                    emit(' ');
     253                    advance();
     254                    break;
     255                }
     256
     257                if (temp == '*') {
     258                    m_parseState = InMultiLineComment;
     259                    // Emit the comment start in case the user has
     260                    // an unclosed comment and we want to later
     261                    // signal an error.
     262                    emit('/');
     263                    emit('*');
     264                    advance();
     265                    break;
     266                }
     267            }
     268
     269            emit(c);
     270            break;
     271
     272        case InPreprocessorDirective:
     273            // No matter what the character is, just pass it
     274            // through. Do not parse comments in this state. This
     275            // might not be the right thing to do long term, but it
     276            // should handle the #error preprocessor directive.
     277            emit(c);
     278            break;
     279
     280        case InSingleLineComment:
     281            // The newline code at the top of this function takes care
     282            // of resetting our state when we get out of the
     283            // single-line comment. Swallow all other characters.
     284            break;
     285
     286        case InMultiLineComment:
     287            if (c == '*' && peek(temp) && temp == '/') {
     288                emit('*');
     289                emit('/');
     290                m_parseState = MiddleOfLine;
     291                advance();
     292                break;
     293            }
     294
     295            // Swallow all other characters. Unclear whether we may
     296            // want or need to just emit a space per character to try
     297            // to preserve column numbers for debugging purposes.
     298            break;
     299        }
     300    }
    118301} // namespace anonymous
    119302
    … …  
    25482731    if (isContextLost() || !validateWebGLObject(shader))
    25492732        return;
    2550     if (!validateString(string))
    2551         return;
    2552     m_context->shaderSource(objectOrZero(shader), string);
     2733    String stringWithoutComments = StripComments(string).result();
     2734    if (!validateString(stringWithoutComments))
     2735        return;
     2736    m_context->shaderSource(objectOrZero(shader), stringWithoutComments);
    25532737    cleanupAfterGraphicsCall(false);
    25542738}
Note: See TracChangeset for help on using the changeset viewer.