Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2310243002:
Supress integer-overflow in TexSubImage2D(3D)Impl (Closed)
DescriptionSupress integer-overflow in TexSubImage2D(3D)Impl
Currently in these two functions, we are not using CheckedNumeric. This
CL uses CheckedNumeric to ensure that integer-overflow will not happen
BUG=644271
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Committed: https://crrev.com/e8c2ace2094db0c61c9a2deea1f51b5b1054a305
Cr-Commit-Position: refs/heads/master@{#417137}
Patch Set 1 #Patch Set 2 : one more place #
Total comments: 2
Patch Set 3 : remove un-necessary checks #Patch Set 4 : clean up #
Total comments: 4
Patch Set 5 : address comments #
Total comments: 4
Patch Set 6 : address comments #
Total comments: 2
Patch Set 7 : more on comments #Patch Set 8 : move check to call sites #
Total comments: 2
Patch Set 9 : check xoffset #Messages
Total messages: 38 (15 generated)
Description was changed from ========== Supress integer-overflow in TexSubImage2D(3D)Impl Currently in these two functions, we are not using CheckedNumeric. This CL uses CheckedNumeric to ensure that integer-overflow will not happen BUG=644271 ========== to ========== Supress integer-overflow in TexSubImage2D(3D)Impl Currently in these two functions, we are not using CheckedNumeric. This CL uses CheckedNumeric to ensure that integer-overflow will not happen BUG=644271 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
xidachen@chromium.org changed reviewers: + bajones@chromium.org, kbr@chromium.org, zmo@chromium.org
PTAL
https://codereview.chromium.org/2310243002/diff/20001/gpu/command_buffer/clie... File gpu/command_buffer/client/gles2_implementation.cc (right): https://codereview.chromium.org/2310243002/diff/20001/gpu/command_buffer/clie... gpu/command_buffer/client/gles2_implementation.cc:3051: base::CheckedNumeric<unsigned int> desired_size = buffer_padded_row_size; I dig a little further into the code. It seems to me that whoever calls this function already did the overflow check. For example, in TexSubImage2D(), GLES2Util::ComputeImageDataSizesES3() should make sure no overflow is possible in here. Can you check further and see why that function doesn't catch the overflow?
Description was changed from ========== Supress integer-overflow in TexSubImage2D(3D)Impl Currently in these two functions, we are not using CheckedNumeric. This CL uses CheckedNumeric to ensure that integer-overflow will not happen BUG=644271 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Supress integer-overflow in TexSubImage2D(3D)Impl Currently in these two functions, we are not using CheckedNumeric. This CL uses CheckedNumeric to ensure that integer-overflow will not happen BUG=644271 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
zmo@chromium.org changed reviewers: + piman@chromium.org
On 2016/09/06 21:52:10, Zhenyao Mo wrote: > https://codereview.chromium.org/2310243002/diff/20001/gpu/command_buffer/clie... > File gpu/command_buffer/client/gles2_implementation.cc (right): > > https://codereview.chromium.org/2310243002/diff/20001/gpu/command_buffer/clie... > gpu/command_buffer/client/gles2_implementation.cc:3051: > base::CheckedNumeric<unsigned int> desired_size = buffer_padded_row_size; > I dig a little further into the code. It seems to me that whoever calls this > function already did the overflow check. For example, in TexSubImage2D(), > GLES2Util::ComputeImageDataSizesES3() should make sure no overflow is possible > in here. > > Can you check further and see why that function doesn't catch the overflow? So the integer overflow happens in this line: yoffset += num_rows; Here yoffset is 2147483635, which is in the range of GLint, num_rows is 16, but doing yoffset += num_rows makes yoffset exceed INT_MAX. Does that make sense? In this patch, I used CheckNumeric in many places, just to be safe.
On 2016/09/07 00:47:25, xidachen wrote: > On 2016/09/06 21:52:10, Zhenyao Mo wrote: > > > https://codereview.chromium.org/2310243002/diff/20001/gpu/command_buffer/clie... > > File gpu/command_buffer/client/gles2_implementation.cc (right): > > > > > https://codereview.chromium.org/2310243002/diff/20001/gpu/command_buffer/clie... > > gpu/command_buffer/client/gles2_implementation.cc:3051: > > base::CheckedNumeric<unsigned int> desired_size = buffer_padded_row_size; > > I dig a little further into the code. It seems to me that whoever calls this > > function already did the overflow check. For example, in TexSubImage2D(), > > GLES2Util::ComputeImageDataSizesES3() should make sure no overflow is possible > > in here. > > > > Can you check further and see why that function doesn't catch the overflow? > > So the integer overflow happens in this line: > yoffset += num_rows; > > Here yoffset is 2147483635, which is in the range of GLint, num_rows is 16, but > doing yoffset += num_rows makes yoffset exceed INT_MAX. Does that make sense? > > In this patch, I used CheckNumeric in many places, just to be safe. I understand where the overflow happens, what I don't understand is why the callers of TexSubImage2D(3D)Impl do not prevent it, because the design is for callers to catch all the overflow cases in GLES2Util::ComputeImageDataSizesES3().
On 2016/09/06 21:52:10, Zhenyao Mo wrote: > https://codereview.chromium.org/2310243002/diff/20001/gpu/command_buffer/clie... > File gpu/command_buffer/client/gles2_implementation.cc (right): > > https://codereview.chromium.org/2310243002/diff/20001/gpu/command_buffer/clie... > gpu/command_buffer/client/gles2_implementation.cc:3051: > base::CheckedNumeric<unsigned int> desired_size = buffer_padded_row_size; > I dig a little further into the code. It seems to me that whoever calls this > function already did the overflow check. For example, in TexSubImage2D(), > GLES2Util::ComputeImageDataSizesES3() should make sure no overflow is possible > in here. > > Can you check further and see why that function doesn't catch the overflow? So I think GLES2Util::ComputeImageDataSizesES3() ensures that yoffset doesn't overflow, but it doesn't prevent yoffset + num_rows in this line: https://cs.chromium.org/chromium/src/gpu/command_buffer/client/gles2_implemen...
On 2016/09/07 00:50:20, Zhenyao Mo wrote: > On 2016/09/07 00:47:25, xidachen wrote: > > On 2016/09/06 21:52:10, Zhenyao Mo wrote: > > > > > > https://codereview.chromium.org/2310243002/diff/20001/gpu/command_buffer/clie... > > > File gpu/command_buffer/client/gles2_implementation.cc (right): > > > > > > > > > https://codereview.chromium.org/2310243002/diff/20001/gpu/command_buffer/clie... > > > gpu/command_buffer/client/gles2_implementation.cc:3051: > > > base::CheckedNumeric<unsigned int> desired_size = buffer_padded_row_size; > > > I dig a little further into the code. It seems to me that whoever calls > this > > > function already did the overflow check. For example, in TexSubImage2D(), > > > GLES2Util::ComputeImageDataSizesES3() should make sure no overflow is > possible > > > in here. > > > > > > Can you check further and see why that function doesn't catch the overflow? > > > > So the integer overflow happens in this line: > > yoffset += num_rows; > > > > Here yoffset is 2147483635, which is in the range of GLint, num_rows is 16, > but > > doing yoffset += num_rows makes yoffset exceed INT_MAX. Does that make sense? > > > > In this patch, I used CheckNumeric in many places, just to be safe. > > I understand where the overflow happens, what I don't understand is why the > callers of TexSubImage2D(3D)Impl do not prevent it, because the design is for > callers to catch all the overflow cases in > GLES2Util::ComputeImageDataSizesES3(). Oh, I see your point. Let me take a closer look.
https://codereview.chromium.org/2310243002/diff/20001/gpu/command_buffer/clie... File gpu/command_buffer/client/gles2_implementation.cc (right): https://codereview.chromium.org/2310243002/diff/20001/gpu/command_buffer/clie... gpu/command_buffer/client/gles2_implementation.cc:3051: base::CheckedNumeric<unsigned int> desired_size = buffer_padded_row_size; On 2016/09/06 21:52:10, Zhenyao Mo wrote: > I dig a little further into the code. It seems to me that whoever calls this > function already did the overflow check. For example, in TexSubImage2D(), > GLES2Util::ComputeImageDataSizesES3() should make sure no overflow is possible > in here. > > Can you check further and see why that function doesn't catch the overflow? After a closer look, I think you are right. Most checks are not necessary in TexSubImage2D(3D)Impl because they are already checked to be safe before it is called. Except one place which is the yoffset += num_rows; There are two call sites of TexSubImage2DImpl, and none of them checks yoffset against any "+" operations. In PS#4, I removed all un-necessary checks. PTAL.
Good catch. https://codereview.chromium.org/2310243002/diff/60001/gpu/command_buffer/clie... File gpu/command_buffer/client/gles2_implementation.cc (right): https://codereview.chromium.org/2310243002/diff/60001/gpu/command_buffer/clie... gpu/command_buffer/client/gles2_implementation.cc:3072: yoffset = updated_yoffset.ValueOrDefault(std::numeric_limits<int>::max()); Can we generate an INVALID_VALUE instead? https://codereview.chromium.org/2310243002/diff/60001/gpu/command_buffer/clie... gpu/command_buffer/client/gles2_implementation.cc:3164: target, level, xoffset, yoffset + row_index, zoffset + depth_index, Here, should we worry about overflow also?
https://codereview.chromium.org/2310243002/diff/60001/gpu/command_buffer/clie... File gpu/command_buffer/client/gles2_implementation.cc (right): https://codereview.chromium.org/2310243002/diff/60001/gpu/command_buffer/clie... gpu/command_buffer/client/gles2_implementation.cc:3072: yoffset = updated_yoffset.ValueOrDefault(std::numeric_limits<int>::max()); On 2016/09/07 02:15:16, Zhenyao Mo wrote: > Can we generate an INVALID_VALUE instead? I used GL_INVALID_VALUE. So do we have checks somewhere to make sure that we abort or doing something when we see that yoffset becomes GL_INVALID_VALUE? https://codereview.chromium.org/2310243002/diff/60001/gpu/command_buffer/clie... gpu/command_buffer/client/gles2_implementation.cc:3164: target, level, xoffset, yoffset + row_index, zoffset + depth_index, On 2016/09/07 02:15:16, Zhenyao Mo wrote: > Here, should we worry about overflow also? Ah, I totally missed that. Added in the new patch.
https://codereview.chromium.org/2310243002/diff/80001/gpu/command_buffer/clie... File gpu/command_buffer/client/gles2_implementation.cc (right): https://codereview.chromium.org/2310243002/diff/80001/gpu/command_buffer/clie... gpu/command_buffer/client/gles2_implementation.cc:3072: yoffset = updated_yoffset.ValueOrDefault(GL_INVALID_VALUE); You misunderstood me. It should be if (!updated_yoffset.valid()) { SetGLError(GL_INVALID_VALUE, function_name, "yoffset + height overflow"); } yoffset = updated_yoffset.ValueOrDefault(0); https://codereview.chromium.org/2310243002/diff/80001/gpu/command_buffer/clie... gpu/command_buffer/client/gles2_implementation.cc:3170: updated_zoffset.ValueOrDefault(GL_INVALID_VALUE), Same here.
https://codereview.chromium.org/2310243002/diff/80001/gpu/command_buffer/clie... File gpu/command_buffer/client/gles2_implementation.cc (right): https://codereview.chromium.org/2310243002/diff/80001/gpu/command_buffer/clie... gpu/command_buffer/client/gles2_implementation.cc:3072: yoffset = updated_yoffset.ValueOrDefault(GL_INVALID_VALUE); On 2016/09/07 17:05:33, Zhenyao Mo wrote: > You misunderstood me. > > It should be > > if (!updated_yoffset.valid()) { > SetGLError(GL_INVALID_VALUE, function_name, "yoffset + height overflow"); > } > yoffset = updated_yoffset.ValueOrDefault(0); Done. https://codereview.chromium.org/2310243002/diff/80001/gpu/command_buffer/clie... gpu/command_buffer/client/gles2_implementation.cc:3170: updated_zoffset.ValueOrDefault(GL_INVALID_VALUE), On 2016/09/07 17:05:34, Zhenyao Mo wrote: > Same here. Done.
lgtm with nits fixed. https://codereview.chromium.org/2310243002/diff/100001/gpu/command_buffer/cli... File gpu/command_buffer/client/gles2_implementation.cc (right): https://codereview.chromium.org/2310243002/diff/100001/gpu/command_buffer/cli... gpu/command_buffer/client/gles2_implementation.cc:3171: "yoffset + row_index overflows"); row_index and depth_index below are internal implementation details. You should use height and depth instead.
The CQ bit was checked by xidachen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2310243002/diff/100001/gpu/command_buffer/cli... File gpu/command_buffer/client/gles2_implementation.cc (right): https://codereview.chromium.org/2310243002/diff/100001/gpu/command_buffer/cli... gpu/command_buffer/client/gles2_implementation.cc:3074: "yoffset + height overflows"); Here and other places: we should return early and avoid issuing invalid TexSubImage2DImpl. In particular it is not legitimate to write to yoffset=0 in case of overflow, if the original request did not include those texels. Technically, if we issue a GL error, we should not be filling any texel at all. Could we check this overflow before calling TexSubImage2DImpl? Is there any case where we would overflow here but not by checking overflow of yoffset+height?
On 2016/09/07 17:48:18, piman wrote: > https://codereview.chromium.org/2310243002/diff/100001/gpu/command_buffer/cli... > File gpu/command_buffer/client/gles2_implementation.cc (right): > > https://codereview.chromium.org/2310243002/diff/100001/gpu/command_buffer/cli... > gpu/command_buffer/client/gles2_implementation.cc:3074: "yoffset + height > overflows"); > Here and other places: we should return early and avoid issuing invalid > TexSubImage2DImpl. In particular it is not legitimate to write to yoffset=0 in > case of overflow, if the original request did not include those texels. > > Technically, if we issue a GL error, we should not be filling any texel at all. > Could we check this overflow before calling TexSubImage2DImpl? Is there any case > where we would overflow here but not by checking overflow of yoffset+height? You have a good point. In general we don't check for overflow on the client side. Service side takes care of it. This is a special case, where we break a single client call into multiple service side calls, so overflow manifest on the client side during this "breaking up" process. I don't think the TexImage2D or TexImage3D calls can lead to overflow here because the ComputeImageDataSizesES3 will catch that. The only overflow cases are from TexSubImage2D and TexSubImage3D where the offset could be too large, and when we compute the image size, we don't take into consideration of the offsets. So If we validate that offset + size (xoffset + width, yoffset + height, zoffset + depth) are all valid in that two callers' side, we should be good.
On 2016/09/07 17:55:26, Zhenyao Mo wrote: > On 2016/09/07 17:48:18, piman wrote: > > > https://codereview.chromium.org/2310243002/diff/100001/gpu/command_buffer/cli... > > File gpu/command_buffer/client/gles2_implementation.cc (right): > > > > > https://codereview.chromium.org/2310243002/diff/100001/gpu/command_buffer/cli... > > gpu/command_buffer/client/gles2_implementation.cc:3074: "yoffset + height > > overflows"); > > Here and other places: we should return early and avoid issuing invalid > > TexSubImage2DImpl. In particular it is not legitimate to write to yoffset=0 in > > case of overflow, if the original request did not include those texels. > > > > Technically, if we issue a GL error, we should not be filling any texel at > all. > > Could we check this overflow before calling TexSubImage2DImpl? Is there any > case > > where we would overflow here but not by checking overflow of yoffset+height? > > You have a good point. In general we don't check for overflow on the client > side. Service side takes care of it. > > This is a special case, where we break a single client call into multiple > service side calls, so overflow manifest on the client side during this > "breaking up" process. > > I don't think the TexImage2D or TexImage3D calls can lead to overflow here > because the ComputeImageDataSizesES3 will catch that. The only overflow cases > are from TexSubImage2D and TexSubImage3D where the offset could be too large, > and when we compute the image size, we don't take into consideration of the > offsets. So If we validate that offset + size (xoffset + width, yoffset + > height, zoffset + depth) are all valid in that two callers' side, we should be > good. So should we move the check to TexSubImage2D(3D), before calling the Impl? Maybe also put a CheckGLError() before calling the Impl?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/07 17:48:18, piman wrote: > https://codereview.chromium.org/2310243002/diff/100001/gpu/command_buffer/cli... > File gpu/command_buffer/client/gles2_implementation.cc (right): > > https://codereview.chromium.org/2310243002/diff/100001/gpu/command_buffer/cli... > gpu/command_buffer/client/gles2_implementation.cc:3074: "yoffset + height > overflows"); > Here and other places: we should return early and avoid issuing invalid > TexSubImage2DImpl. In particular it is not legitimate to write to yoffset=0 in > case of overflow, if the original request did not include those texels. > > Technically, if we issue a GL error, we should not be filling any texel at all. > Could we check this overflow before calling TexSubImage2DImpl? Is there any case > where we would overflow here but not by checking overflow of yoffset+height? In the new patch, the check is moved to the call sites of TexSubImage2D(3D)Impl. If there is overflow, we generate a GL_INVALID_VALUE error and return early before calling TexSubImage2D(3D)Impl. PTAL
https://codereview.chromium.org/2310243002/diff/140001/gpu/command_buffer/cli... File gpu/command_buffer/client/gles2_implementation.cc (right): https://codereview.chromium.org/2310243002/diff/140001/gpu/command_buffer/cli... gpu/command_buffer/client/gles2_implementation.cc:2879: base::CheckedNumeric<GLint> checked_yoffset = yoffset; Let's do xoffset + width also.
https://codereview.chromium.org/2310243002/diff/140001/gpu/command_buffer/cli... File gpu/command_buffer/client/gles2_implementation.cc (right): https://codereview.chromium.org/2310243002/diff/140001/gpu/command_buffer/cli... gpu/command_buffer/client/gles2_implementation.cc:2879: base::CheckedNumeric<GLint> checked_yoffset = yoffset; On 2016/09/07 21:03:18, Zhenyao Mo wrote: > Let's do xoffset + width also. Done.
lgtm
The CQ bit was checked by xidachen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
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 xidachen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Supress integer-overflow in TexSubImage2D(3D)Impl Currently in these two functions, we are not using CheckedNumeric. This CL uses CheckedNumeric to ensure that integer-overflow will not happen BUG=644271 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Supress integer-overflow in TexSubImage2D(3D)Impl Currently in these two functions, we are not using CheckedNumeric. This CL uses CheckedNumeric to ensure that integer-overflow will not happen BUG=644271 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Supress integer-overflow in TexSubImage2D(3D)Impl Currently in these two functions, we are not using CheckedNumeric. This CL uses CheckedNumeric to ensure that integer-overflow will not happen BUG=644271 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Supress integer-overflow in TexSubImage2D(3D)Impl Currently in these two functions, we are not using CheckedNumeric. This CL uses CheckedNumeric to ensure that integer-overflow will not happen BUG=644271 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/e8c2ace2094db0c61c9a2deea1f51b5b1054a305 Cr-Commit-Position: refs/heads/master@{#417137} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/e8c2ace2094db0c61c9a2deea1f51b5b1054a305 Cr-Commit-Position: refs/heads/master@{#417137} |
