| Summary: | Out of bounds in WebGLRenderingContext::simulateVertexAttrib0 | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dean Jackson <dino> | ||||||||||||||||||
| Component: | New Bugs | Assignee: | Dean Jackson <dino> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | buildbot, commit-queue, esprehn+autocc, gyuyoung.kim, kondapallykalyan, rniwa, roger_fong | ||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Dean Jackson
2015-07-21 17:44:52 PDT
Created attachment 257225 [details]
Patch
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. Created attachment 257229 [details]
Patch
Created attachment 257230 [details]
Patch
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 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
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. No, it's webgl that is failing to be created. Created attachment 257239 [details]
Not for review. EWS test.
Only WK1 failed. Very weird. 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 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
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 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. 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. CONSOLE MESSAGE: line 14: TypeError: null is not an object (evaluating 'gl.createProgram') 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. Created attachment 257295 [details]
Patch
Created attachment 257296 [details]
Patch
(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. Comment on attachment 257296 [details]
Patch
lgtm,r=me,r+
Committed r187189: <http://trac.webkit.org/changeset/187189> |