Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2892663004:
Fix signed integer overflow in gles2_cmd_decoder_unittest_extensions.cc (Closed)
DescriptionFix signed integer overflow in gles2_cmd_decoder_unittest_extensions.cc
New versions of Clang warns about this.
BUG=724186
TBR=jbauman
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
Review-Url: https://codereview.chromium.org/2892663004
Cr-Commit-Position: refs/heads/master@{#472932}
Committed: https://chromium.googlesource.com/chromium/src/+/93304bdb30f35daf45c935a39f23d7277ee6ccfa
Patch Set 1 #
Total comments: 2
Messages
Total messages: 18 (7 generated)
Description was changed from ========== Fix signed integer overflow in gles2_cmd_decoder_unittest_extensions.cc New versions of Clang warns about this. BUG=724186 ========== to ========== Fix signed integer overflow in gles2_cmd_decoder_unittest_extensions.cc New versions of Clang warns about this. BUG=724186 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ==========
hans@chromium.org changed reviewers: + kkinnunen@nvidia.com, thakis@chromium.org
Please take a look.
Thanks! https://codereview.chromium.org/2892663004/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_extensions.cc (right): https://codereview.chromium.org/2892663004/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder_unittest_extensions.cc:592: delete_cmd.Init(static_cast<GLuint>(std::numeric_limits<GLsizei>::max()) + 1, Isn't GLsizei an unsigned type already? And can't you get the same in shorter with 1u instead of 1?
Also I haven't actually confirmed that this defeats the warning as my clang build is busy bisecting something. It should though, and the ToT bots will tell us. https://codereview.chromium.org/2892663004/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_extensions.cc (right): https://codereview.chromium.org/2892663004/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder_unittest_extensions.cc:592: delete_cmd.Init(static_cast<GLuint>(std::numeric_limits<GLsizei>::max()) + 1, On 2017/05/18 17:40:23, Nico wrote: > Isn't GLsizei an unsigned type already? No, it's signed. > And can't you get the same in shorter with 1u instead of 1? I was nervous that if GLsizei happened to be wider than int, then GLsizei + unsigned would promote the unsigned to GLsizei and we'd end up with signed addition after all.
https://www.khronos.org/opengl/wiki/OpenGL_Type says "non negative". I thought this was the size_t of OpenGL. On May 18, 2017 1:48 PM, <hans@chromium.org> wrote: > Also I haven't actually confirmed that this defeats the warning as my clang > build is busy bisecting something. It should though, and the ToT bots will > tell > us. > > > https://codereview.chromium.org/2892663004/diff/1/gpu/ > command_buffer/service/gles2_cmd_decoder_unittest_extensions.cc > File gpu/command_buffer/service/gles2_cmd_decoder_unittest_extensions.cc > (right): > > https://codereview.chromium.org/2892663004/diff/1/gpu/ > command_buffer/service/gles2_cmd_decoder_unittest_extensions.cc#newcode592 > gpu/command_buffer/service/gles2_cmd_decoder_unittest_extensions.cc:592: > delete_cmd.Init(static_cast<GLuint>(std::numeric_limits<GLsizei>::max()) > + 1, > On 2017/05/18 17:40:23, Nico wrote: > > Isn't GLsizei an unsigned type already? > > No, it's signed. > > > And can't you get the same in shorter with 1u instead of 1? > > I was nervous that if GLsizei happened to be wider than int, then > GLsizei + unsigned would promote the unsigned to GLsizei and we'd end up > with signed addition after all. > > https://codereview.chromium.org/2892663004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
(you're right that it's wider than int though, that's a good point) On May 18, 2017 2:08 PM, "Nico Weber" <thakis@chromium.org> wrote: > https://www.khronos.org/opengl/wiki/OpenGL_Type says "non negative". I > thought this was the size_t of OpenGL. > > On May 18, 2017 1:48 PM, <hans@chromium.org> wrote: > >> Also I haven't actually confirmed that this defeats the warning as my >> clang >> build is busy bisecting something. It should though, and the ToT bots >> will tell >> us. >> >> >> https://codereview.chromium.org/2892663004/diff/1/gpu/comman >> d_buffer/service/gles2_cmd_decoder_unittest_extensions.cc >> File gpu/command_buffer/service/gles2_cmd_decoder_unittest_extensions.cc >> (right): >> >> https://codereview.chromium.org/2892663004/diff/1/gpu/comman >> d_buffer/service/gles2_cmd_decoder_unittest_extensions.cc#newcode592 >> gpu/command_buffer/service/gles2_cmd_decoder_unittest_extensions.cc:592: >> delete_cmd.Init(static_cast<GLuint>(std::numeric_limits<GLsizei>::max()) >> + 1, >> On 2017/05/18 17:40:23, Nico wrote: >> > Isn't GLsizei an unsigned type already? >> >> No, it's signed. >> >> > And can't you get the same in shorter with 1u instead of 1? >> >> I was nervous that if GLsizei happened to be wider than int, then >> GLsizei + unsigned would promote the unsigned to GLsizei and we'd end up >> with signed addition after all. >> >> https://codereview.chromium.org/2892663004/ >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/05/18 18:09:10, Nico wrote: > (you're right that it's wider than int though, that's a good point) > > On May 18, 2017 2:08 PM, "Nico Weber" <mailto:thakis@chromium.org> wrote: > > > https://www.khronos.org/opengl/wiki/OpenGL_Type says "non negative". I > > thought this was the size_t of OpenGL. https://cs.chromium.org/search/?q=GLsizei suggests it's signed. Looks like it's the ssize_t of OpenGL :-) That's pretty confusing actually, given this at the bottom of the page you linked: "For example, a GLsizei is functionally equivalent to GLuint."
hans@chromium.org changed reviewers: + jbauman@chromium.org
+jbauman for ownership
lgtm Init() takes a GLuint as first arg, so this code wants to pass an unsigned that's larger than a int_max, but not uint_max. Took me a while to understand, sorry.
Description was changed from ========== Fix signed integer overflow in gles2_cmd_decoder_unittest_extensions.cc New versions of Clang warns about this. BUG=724186 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ========== Fix signed integer overflow in gles2_cmd_decoder_unittest_extensions.cc New versions of Clang warns about this. BUG=724186 TBR=jbauman CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ==========
tbr'ing for ownership as bots are red due to this
The CQ bit was checked by hans@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1495134387778800, "parent_rev":
"a0fbb7c0d4e4b52d9170031a8883c60e19f4916c", "commit_rev":
"93304bdb30f35daf45c935a39f23d7277ee6ccfa"}
Message was sent while issue was closed.
Description was changed from ========== Fix signed integer overflow in gles2_cmd_decoder_unittest_extensions.cc New versions of Clang warns about this. BUG=724186 TBR=jbauman CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ========== Fix signed integer overflow in gles2_cmd_decoder_unittest_extensions.cc New versions of Clang warns about this. BUG=724186 TBR=jbauman CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 Review-Url: https://codereview.chromium.org/2892663004 Cr-Commit-Position: refs/heads/master@{#472932} Committed: https://chromium.googlesource.com/chromium/src/+/93304bdb30f35daf45c935a39f23... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/93304bdb30f35daf45c935a39f23... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
