Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1315983010:
WebGL: validations and fixes to avoid buffer/texture overflow (Closed)
DescriptionWebGL: validations and fixes to avoid buffer/texture overflow
BUG=528177
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201851
Patch Set 1 : #
Total comments: 11
Patch Set 2 : addressed zmo@'s feedback: overflow detection #Patch Set 3 : code rebase #
Messages
Total messages: 22 (9 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
yunchao.he@intel.com changed reviewers: + bajones@chromium.org, kbr@chromium.org, zmo@chromium.org
The conformance test is submitted too: https://github.com/KhronosGroup/WebGL/pull/1175. PTAL. Thanks!
https://codereview.chromium.org/1315983010/diff/40001/Source/modules/webgl/We... File Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1315983010/diff/40001/Source/modules/webgl/We... Source/modules/webgl/WebGL2RenderingContextBase.cpp:109: if (readOffset + size > readBuffer->getSize() || writeOffset + size > writeBuffer->getSize()) { Overflow detection. We made sure the offset/size fits uint32, so two plus together could overflow int64. https://codereview.chromium.org/1315983010/diff/40001/Source/modules/webgl/We... Source/modules/webgl/WebGL2RenderingContextBase.cpp:143: if (offset + returnedData->byteLength() > buffer->getSize()) { Same here. overflow detection. https://codereview.chromium.org/1315983010/diff/40001/Source/modules/webgl/We... Source/modules/webgl/WebGL2RenderingContextBase.cpp:513: if (width + xoffset > tex->getWidth(target, level) Same here, overflow detection. https://codereview.chromium.org/1315983010/diff/40001/Source/modules/webgl/We... Source/modules/webgl/WebGL2RenderingContextBase.cpp:1676: if (buffer && (offset + size > buffer->getSize())) { Same here, overflow detection. https://codereview.chromium.org/1315983010/diff/40001/Source/modules/webgl/We... File Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1315983010/diff/40001/Source/modules/webgl/We... Source/modules/webgl/WebGLRenderingContextBase.cpp:5885: if (width + xoffset > tex->getWidth(target, level) You need to do overflow test here. See Checked<> example in validateCopyTexSubImage().
On 2015/09/02 17:59:03, Zhenyao Mo wrote: > https://codereview.chromium.org/1315983010/diff/40001/Source/modules/webgl/We... > File Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): > > https://codereview.chromium.org/1315983010/diff/40001/Source/modules/webgl/We... > Source/modules/webgl/WebGL2RenderingContextBase.cpp:109: if (readOffset + size > > readBuffer->getSize() || writeOffset + size > writeBuffer->getSize()) { > Overflow detection. We made sure the offset/size fits uint32, so two plus > together could overflow int64. > > https://codereview.chromium.org/1315983010/diff/40001/Source/modules/webgl/We... > Source/modules/webgl/WebGL2RenderingContextBase.cpp:143: if (offset + > returnedData->byteLength() > buffer->getSize()) { > Same here. overflow detection. > > https://codereview.chromium.org/1315983010/diff/40001/Source/modules/webgl/We... > Source/modules/webgl/WebGL2RenderingContextBase.cpp:513: if (width + xoffset > > tex->getWidth(target, level) > Same here, overflow detection. > > https://codereview.chromium.org/1315983010/diff/40001/Source/modules/webgl/We... > Source/modules/webgl/WebGL2RenderingContextBase.cpp:1676: if (buffer && (offset > + size > buffer->getSize())) { > Same here, overflow detection. > > https://codereview.chromium.org/1315983010/diff/40001/Source/modules/webgl/We... > File Source/modules/webgl/WebGLRenderingContextBase.cpp (right): > > https://codereview.chromium.org/1315983010/diff/40001/Source/modules/webgl/We... > Source/modules/webgl/WebGLRenderingContextBase.cpp:5885: if (width + xoffset > > tex->getWidth(target, level) > You need to do overflow test here. See Checked<> example in > validateCopyTexSubImage(). Could you please also add test cases in the conformance tests for these overflow situations?
Please also reference a bug ID, in order to provide more background and a centralized place where any reverts and re-lands would be recorded.
Thanks for your review, zmo@ and kbr@. I have created an issue, and updated the code too. PTAL. https://codereview.chromium.org/1315983010/diff/40001/Source/modules/webgl/We... File Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1315983010/diff/40001/Source/modules/webgl/We... Source/modules/webgl/WebGL2RenderingContextBase.cpp:109: if (readOffset + size > readBuffer->getSize() || writeOffset + size > writeBuffer->getSize()) { On 2015/09/02 17:59:03, Zhenyao Mo wrote: > Overflow detection. We made sure the offset/size fits uint32, so two plus > together could overflow int64. the function validateValueFitNonNegInt32 makes sure that offset/size are less than 2 ^ 31 -1, although offset/size are both int64(long long). So offset plus size will not overflow int64, right? https://codereview.chromium.org/1315983010/diff/40001/Source/modules/webgl/We... Source/modules/webgl/WebGL2RenderingContextBase.cpp:143: if (offset + returnedData->byteLength() > buffer->getSize()) { On 2015/09/02 17:59:03, Zhenyao Mo wrote: > Same here. overflow detection. same here. https://codereview.chromium.org/1315983010/diff/40001/Source/modules/webgl/We... Source/modules/webgl/WebGL2RenderingContextBase.cpp:513: if (width + xoffset > tex->getWidth(target, level) On 2015/09/02 17:59:03, Zhenyao Mo wrote: > Same here, overflow detection. Done. https://codereview.chromium.org/1315983010/diff/40001/Source/modules/webgl/We... Source/modules/webgl/WebGL2RenderingContextBase.cpp:1676: if (buffer && (offset + size > buffer->getSize())) { On 2015/09/02 17:59:03, Zhenyao Mo wrote: > Same here, overflow detection. same here https://codereview.chromium.org/1315983010/diff/40001/Source/modules/webgl/We... File Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1315983010/diff/40001/Source/modules/webgl/We... Source/modules/webgl/WebGLRenderingContextBase.cpp:5885: if (width + xoffset > tex->getWidth(target, level) On 2015/09/02 17:59:03, Zhenyao Mo wrote: > You need to do overflow test here. See Checked<> example in > validateCopyTexSubImage(). Done.
https://codereview.chromium.org/1315983010/diff/40001/Source/modules/webgl/We... File Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): https://codereview.chromium.org/1315983010/diff/40001/Source/modules/webgl/We... Source/modules/webgl/WebGL2RenderingContextBase.cpp:109: if (readOffset + size > readBuffer->getSize() || writeOffset + size > writeBuffer->getSize()) { On 2015/09/04 08:22:07, yunchao wrote: > On 2015/09/02 17:59:03, Zhenyao Mo wrote: > > Overflow detection. We made sure the offset/size fits uint32, so two plus > > together could overflow int64. > > the function validateValueFitNonNegInt32 makes sure that offset/size are less > than 2 ^ 31 -1, although offset/size are both int64(long long). So offset plus > size will not overflow int64, right? Not really. Two uint32 adding won't overflow uint64, but it still can overflow int64. Here and below.
On 2015/09/04 18:36:28, Zhenyao Mo wrote: > https://codereview.chromium.org/1315983010/diff/40001/Source/modules/webgl/We... > File Source/modules/webgl/WebGL2RenderingContextBase.cpp (right): > > https://codereview.chromium.org/1315983010/diff/40001/Source/modules/webgl/We... > Source/modules/webgl/WebGL2RenderingContextBase.cpp:109: if (readOffset + size > > readBuffer->getSize() || writeOffset + size > writeBuffer->getSize()) { > On 2015/09/04 08:22:07, yunchao wrote: > > On 2015/09/02 17:59:03, Zhenyao Mo wrote: > > > Overflow detection. We made sure the offset/size fits uint32, so two plus > > > together could overflow int64. > > > > the function validateValueFitNonNegInt32 makes sure that offset/size are less > > than 2 ^ 31 -1, although offset/size are both int64(long long). So offset plus > > size will not overflow int64, right? > > Not really. Two uint32 adding won't overflow uint64, but it still can overflow > int64. Here and below. Never mind. My brain is apparently not working properly (thanks Brandon) LGTM
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1315983010/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1315983010/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1315983010/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1315983010/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yunchao.he@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org Link to the patchset: https://codereview.chromium.org/1315983010/#ps80001 (title: "code rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1315983010/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1315983010/80001
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201851 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
