Validate pixel data array is enough for request by texImage3D and texSubImage3D

Validate pixel data array is enough for request by texImage3D and texSubImage3D

BUG=551750
TEST=conformance2/textures/misc/tex-3d-size-limit.html

Committed: https://crrev.com/965cef3e5c3ac49c8f784317804072fe0d2d0b16
Cr-Commit-Position: refs/heads/master@{#361022}

[qiankun, 2015-11-10 09:03:12]

Description was changed from

==========
Validate pixel data array is enough for request by texImage3D and texSubImage3D

BUG=551750
==========

to

==========
Validate pixel data array is enough for request by texImage3D and texSubImage3D

BUG=551750
TEST=conformance2/textures/misc/tex-3d-size-limit.html
==========

[qiankun, 2015-11-10 09:03:27]

qiankun.miao@intel.com changed reviewers:
+ bajones@chromium.org, kbr@chromium.org, zmo@chromium.org

[qiankun, 2015-11-10 09:03:28]

conformance2/textures/misc/tex-3d-size-limit.html covers this bug. Please help
to review this CL. Thanks!

[Zhenyao Mo, 2015-11-12 02:01:20]

https://codereview.chromium.org/1414853008/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/1414853008/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5780:
totalBytesRequired *= depth;
This is a bad hack.  You should add the |depth| into
WebGLImageConversion::computeImageSizeInBytes(). There you will see, *= isn't
safe.  We need to consider overflow.

[qiankun, 2015-11-12 05:46:19]

https://codereview.chromium.org/1414853008/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/1414853008/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5780:
totalBytesRequired *= depth;
On 2015/11/12 02:01:20, Zhenyao Mo wrote:
> This is a bad hack.  You should add the |depth| into
> WebGLImageConversion::computeImageSizeInBytes(). There you will see, *= isn't
> safe.  We need to consider overflow.

Thanks. I passed depth to  WebGLImageConversion::computeImageSizeInBytes() as a
parameter.

[Zhenyao Mo, 2015-11-12 11:31:52]

https://codereview.chromium.org/1414853008/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp
(right):

https://codereview.chromium.org/1414853008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp:2012:
return GL_INVALID_VALUE;
Another issue: see above, "last row needs no padding".  Now every last row in
every 2d image have no padding, but my understanding is only the last row of the
last 2d image has no padding.

[qiankun, 2015-11-13 08:29:59]

https://codereview.chromium.org/1414853008/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp
(right):

https://codereview.chromium.org/1414853008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp:2012:
return GL_INVALID_VALUE;
On 2015/11/12 11:31:52, Zhenyao Mo wrote:
> Another issue: see above, "last row needs no padding".  Now every last row in
> every 2d image have no padding, but my understanding is only the last row of
the
> last 2d image has no padding.

Done.

[Ken Russell (switch to Gerrit), 2015-11-20 00:15:42]

https://codereview.chromium.org/1414853008/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp
(right):

https://codereview.chromium.org/1414853008/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp:2006:
checkedValue *= (height * depth - 1);
More changes are needed to this function in order to make it correct for ES 3.0,
because many more pixel storage parameters are exposed: UNPACK_ROW_LENGTH,
UNPACK_SKIP_ROWS, UNPACK_SKIP_PIXELS, UNPACK_IMAGE_HEIGHT, and
UNPACK_SKIP_IMAGES. Will you continue working on making this function more
correct, and adding more tests?

[qiankun, 2015-11-20 01:01:04]

On 2015/11/20 00:15:42, Ken Russell wrote:
>
https://codereview.chromium.org/1414853008/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp
> (right):
> 
>
https://codereview.chromium.org/1414853008/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp:2006:
> checkedValue *= (height * depth - 1);
> More changes are needed to this function in order to make it correct for ES
3.0,
> because many more pixel storage parameters are exposed: UNPACK_ROW_LENGTH,
> UNPACK_SKIP_ROWS, UNPACK_SKIP_PIXELS, UNPACK_IMAGE_HEIGHT, and
> UNPACK_SKIP_IMAGES. Will you continue working on making this function more
> correct, and adding more tests?

Yes. I will do this work in next few days.

[Ken Russell (switch to Gerrit), 2015-11-20 23:26:01]

LGTM. Thanks for this contribution.

[Ken Russell (switch to Gerrit), 2015-11-20 23:26:05]

The CQ bit was checked by kbr@chromium.org

[commit-bot: I haz the power, 2015-11-20 23:28:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414853008/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414853008/60001

[commit-bot: I haz the power, 2015-11-21 02:55:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-21 02:55:48]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[qiankun, 2015-11-21 14:12:14]

The CQ bit was checked by qiankun.miao@intel.com

[commit-bot: I haz the power, 2015-11-21 14:12:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414853008/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414853008/60001

[commit-bot: I haz the power, 2015-11-21 15:23:17]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-21 15:24:21]

Patchset 4 (id:??) landed as
https://crrev.com/965cef3e5c3ac49c8f784317804072fe0d2d0b16
Cr-Commit-Position: refs/heads/master@{#361022}