Bug 147176 – Out of bounds in WebGLRenderingContext::simulateVertexAttrib0

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dean Jackson]

Out of bounds in WebGLRenderingContext::simulateVertexAttrib0

[Dean Jackson]

Created attachment 257225 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 257225 [details]
Patch

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

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1796
> +            synthesizeGLError(GraphicsContext3D::INVALID_OPERATION, functionName, "attempt to access out of bounds for simulated vertexAttrib0 array");

The message doesn't parse. ""attempt to access out of bounds for simulated vertexAttrib0 array" -> "attempt to access out of bounds SOMETHING in the simulated vertexAttrib0 array" or  "out of bounds access attempted on the simulated vertexAttrib0 array"

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1881
> +            synthesizeGLError(GraphicsContext3D::INVALID_OPERATION, functionName, "attempt to access out of bounds for simulated vertexAttrib0 array");

Same.

[Dean Jackson]

Created attachment 257229 [details]
Patch

[Dean Jackson]

Created attachment 257230 [details]
Patch

[Build Bot]

Comment on attachment 257230 [details]
Patch

Attachment 257230 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5996571871150080

New failing tests:
fast/canvas/webgl/out-of-bounds-simulated-vertexAttrib0-drawArrays.html

[Build Bot]

Created attachment 257232 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Dean Jackson]

I'm not sure what's up with the EWS failure on the new test. It works locally. It must be the GLSL failing to compile on the bot.

[Dean Jackson]

No, it's webgl that is failing to be created.

[Dean Jackson]

Created attachment 257239 [details]
Not for review. EWS test.

[Dean Jackson]

Only WK1 failed. Very weird.

[Build Bot]

Comment on attachment 257239 [details]
Not for review. EWS test.

Attachment 257239 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5201296399269888

New failing tests:
fast/canvas/webgl/out-of-bounds-simulated-vertexAttrib0-drawArrays.html

[Build Bot]

Created attachment 257241 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Oliver Hunt]

Comment on attachment 257230 [details]
Patch

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

r-, as i do not see the value in removing Checked<> in simulateVertexAttrib0

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:4708
> +    if (bufferDataSize.hasOverflowed())
> +        return false;
> +
> +    return true;

return !bufferDataSize.hasOverflowed();

Also you should simply be able to do all the arithmetic using Checked<> and just have the final check at the end.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:4728
> +    GC3Dsizei bufferSize = (numVertex + 1) * 4;

Why * 4? vs sizeof(type)

What is the harm in using Checked<> here as well? Using Checked<> protects us from any divergence in the logic -- you could just switch to Checked<GC3dsizei>, etc and if the logic ever does diverge at least we'll get a crash report saying which check failed and so help us find the divergence.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:4729
> +    GC3Dsizeiptr bufferDataSize = bufferSize * sizeof(GC3Dfloat);

Please use Checked<> here

[Dean Jackson]

Comment on attachment 257230 [details]
Patch

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

I added back the checks in ::simulate

>> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:4708
>> +    return true;
> 
> return !bufferDataSize.hasOverflowed();
> 
> Also you should simply be able to do all the arithmetic using Checked<> and just have the final check at the end.

Yeah, but I like that I can return early if the first calculation overflows.

>> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:4728
>> +    GC3Dsizei bufferSize = (numVertex + 1) * 4;
> 
> Why * 4? vs sizeof(type)
> 
> What is the harm in using Checked<> here as well? Using Checked<> protects us from any divergence in the logic -- you could just switch to Checked<GC3dsizei>, etc and if the logic ever does diverge at least we'll get a crash report saying which check failed and so help us find the divergence.

> Why * 4? vs sizeof(type)

Because it's actually 4. We're making an array for vec4 objects, so 4 * sizeof(float).

I also used Checked here, and your recommendation to remove the ReportOverflow so we could crash if necessary.

>> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:4729
>> +    GC3Dsizeiptr bufferDataSize = bufferSize * sizeof(GC3Dfloat);
> 
> Please use Checked<> here

Done.

[Dean Jackson]

I'm still missing something really obvious as to why the WK1 bot is failing the new test, in a way that suggests it doesn't have WebGL.

[Dean Jackson]

CONSOLE MESSAGE: line 14: TypeError: null is not an object (evaluating 'gl.createProgram')

[Darin Adler]

Comment on attachment 257230 [details]
Patch

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

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:4693
> +    const WebGLVertexArrayObjectBase::VertexAttribState& state = m_boundVertexArrayObject->getVertexAttribState(0);

I would use auto& here.

>>> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:4708
>>> +    return true;
>> 
>> return !bufferDataSize.hasOverflowed();
>> 
>> Also you should simply be able to do all the arithmetic using Checked<> and just have the final check at the end.
> 
> Yeah, but I like that I can return early if the first calculation overflows.

Why? I don’t think we should optimize performance for the overflow case.

[Dean Jackson]

Created attachment 257295 [details]
Patch

[Dean Jackson]

Created attachment 257296 [details]
Patch

[Dean Jackson]

(In reply to comment #17)

> > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:4693
> > +    const WebGLVertexArrayObjectBase::VertexAttribState& state = m_boundVertexArrayObject->getVertexAttribState(0);
> 
> I would use auto& here.

Done. I also realised I could move things about a bit. I'm not sure why we were getting these values well before we used them.

> 
> >>> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:4708
> >>> +    return true;
> >> 
> >> return !bufferDataSize.hasOverflowed();
> >> 
> >> Also you should simply be able to do all the arithmetic using Checked<> and just have the final check at the end.
> > 
> > Yeah, but I like that I can return early if the first calculation overflows.
> 
> Why? I don’t think we should optimize performance for the overflow case.

Ok. Fixed.

[Oliver Hunt]

Comment on attachment 257296 [details]
Patch

lgtm,r=me,r+

[Dean Jackson]

Committed r187189: <http://trac.webkit.org/changeset/187189>