Bug 119513 – [WebGL] compileShader map iterator validation

• Status: RESOLVED
• Resolution: INVALID
• Component: WebGL

[Przemyslaw Szymanski]

There is a missing validation when it is read from the map. Iterators should always have to be checked if point to the end of the container.

[Przemyslaw Szymanski]

Created attachment 208172 [details]
map iterator validation

[Kalyan]

LGTM

[Andreas Kling]

Comment on attachment 208172 [details]
map iterator validation

View in context: https://bugs.webkit.org/attachment.cgi?id=208172&action=review

> Source/WebCore/ChangeLog:15
> +        No new tests. Covered by existing tests.
> +        LayoutTests/webgl/conformance/glsl/functions/glsl-function-abs.html
> +        LayoutTests/webgl/conformance/glsl/functions/glsl-function-acos.html
> +        LayoutTests/webgl/conformance/glsl/functions/glsl-function-length.html

These tests currently crash, or exhibit bad behavior?
If so, we should unskip them with this change.

[Przemyslaw Szymanski]

(In reply to comment #3)
> (From update of attachment 208172 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208172&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        No new tests. Covered by existing tests.
> > +        LayoutTests/webgl/conformance/glsl/functions/glsl-function-abs.html
> > +        LayoutTests/webgl/conformance/glsl/functions/glsl-function-acos.html
> > +        LayoutTests/webgl/conformance/glsl/functions/glsl-function-length.html
> 
> These tests currently crash, or exhibit bad behavior?
> If so, we should unskip them with this change.

Thank you for your review.
These tests works fine. I ran them in the minibrowser directly. I ran Khronos conformance tests too. This patch does not break any webgl test.

[Andreas Kling]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 208172 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=208172&action=review
> > 
> > > Source/WebCore/ChangeLog:15
> > > +        No new tests. Covered by existing tests.
> > > +        LayoutTests/webgl/conformance/glsl/functions/glsl-function-abs.html
> > > +        LayoutTests/webgl/conformance/glsl/functions/glsl-function-acos.html
> > > +        LayoutTests/webgl/conformance/glsl/functions/glsl-function-length.html
> > 
> > These tests currently crash, or exhibit bad behavior?
> > If so, we should unskip them with this change.
> 
> Thank you for your review.
> These tests works fine. I ran them in the minibrowser directly. I ran Khronos conformance tests too. This patch does not break any webgl test.

If these tests work fine without the change, how is this "covered by existing tests"?

[Antonio Gomes]

Comment on attachment 208172 [details]
map iterator validation

View in context: https://bugs.webkit.org/attachment.cgi?id=208172&action=review

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:467
> +        if (result == m_shaderSourceMap.end())
> +            return;

what situation does the patch help?

[Przemyslaw Szymanski]

(In reply to comment #6)
> (From update of attachment 208172 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208172&action=review
> 
> > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:467
> > +        if (result == m_shaderSourceMap.end())
> > +            return;
> 
> what situation does the patch help?

It avoids possibly bug/crash. Assume m_shaderSourceMap does not contain search object. Then result iterator will point to m_shaderSourceMap.end() and read from it should crash. 
Look in other methods with use of m_shaderSourceMap.find. There are that checks too. I think someone forgot about this check here.

[Brent Fulgham]

Comment on attachment 208172 [details]
map iterator validation

This patch isn't necessary (though it would be harmless).

We call "m_extensions->getTranslatedShaderSourceANGLE(shader)", which does it's own check of the m_shaderSourceMap, and returns an empty string if there is no shader source entry. We check for the empty string and return prior to hitting the point where you propose adding a check.

I don't think this change would actually be executed.