Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2598383002:
Fix integer overflow. (Closed)
DescriptionFix integer overflow.
BUG=676902
Review-Url: https://codereview.chromium.org/2598383002
Cr-Commit-Position: refs/heads/master@{#446639}
Committed: https://chromium.googlesource.com/chromium/src/+/0f6dfbc4793fab2a08ea912039ca8ed6f958d81d
Patch Set 1 #Patch Set 2 : Fix includes #
Total comments: 2
Patch Set 3 : Move check to Widget level #Patch Set 4 : Rebase #Messages
Total messages: 24 (14 generated)
eustas@chromium.org changed reviewers: + erg@chromium.org
The CQ bit was checked by eustas@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sky@chromium.org changed reviewers: + sky@chromium.org
https://codereview.chromium.org/2598383002/diff/20001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (left): https://codereview.chromium.org/2598383002/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1095: unsigned long cardinality = static_cast<int>(opacity * 255) * 0x1010101; Can you outline where this goes wrong? Before my change the code was this: void DesktopWindowTreeHostX11::SetOpacity(unsigned char opacity) { unsigned long cardinality = opacity * 0x1010101; But as long as opacity (in the new code) is 0-1, shouldn't this be the same? https://codereview.chromium.org/2598383002/diff/20001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/2598383002/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1098: float clamped_opacity = std::max(0.0f, std::min(1.0f, opacity)); We don't do this anywhere else. It's assumed the value is 0-1. A DCHECK here is fine (or better yet in Widget::SetOpacity() which is generally the entry point for this.
On 2017/01/03 17:54:27, sky (OOO) wrote: > https://codereview.chromium.org/2598383002/diff/20001/ui/views/widget/desktop... > File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (left): > > https://codereview.chromium.org/2598383002/diff/20001/ui/views/widget/desktop... > ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1095: unsigned long > cardinality = static_cast<int>(opacity * 255) * 0x1010101; > Can you outline where this goes wrong? Before my change the code was this: > > void DesktopWindowTreeHostX11::SetOpacity(unsigned char opacity) { > unsigned long cardinality = opacity * 0x1010101; > > But as long as opacity (in the new code) is 0-1, shouldn't this be the same? > > https://codereview.chromium.org/2598383002/diff/20001/ui/views/widget/desktop... > File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): > > https://codereview.chromium.org/2598383002/diff/20001/ui/views/widget/desktop... > ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1098: float > clamped_opacity = std::max(0.0f, std::min(1.0f, opacity)); > We don't do this anywhere else. It's assumed the value is 0-1. A DCHECK here is > fine (or better yet in Widget::SetOpacity() which is generally the entry point > for this. In unsigned long cardinality = opacity * 0x1010101 there was implicit cast to unsigned int first, then multiplication. After change signed int is multiplied by constant -> could cause overflow (values bigger than 0x7FFFFFFF). Will move check (DCHECK) to upped level tomorrow. Thanks.
Why does the unsigned matter here when the value is always >=0 0? On Tue, Jan 3, 2017 at 11:42 AM, <eustas@chromium.org> wrote: > On 2017/01/03 17:54:27, sky (OOO) wrote: >> > https://codereview.chromium.org/2598383002/diff/20001/ui/views/widget/desktop... >> File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (left): >> >> > https://codereview.chromium.org/2598383002/diff/20001/ui/views/widget/desktop... >> ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1095: >> unsigned > long >> cardinality = static_cast<int>(opacity * 255) * 0x1010101; >> Can you outline where this goes wrong? Before my change the code was this: >> >> void DesktopWindowTreeHostX11::SetOpacity(unsigned char opacity) { >> unsigned long cardinality = opacity * 0x1010101; >> >> But as long as opacity (in the new code) is 0-1, shouldn't this be the >> same? >> >> > https://codereview.chromium.org/2598383002/diff/20001/ui/views/widget/desktop... >> File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): >> >> > https://codereview.chromium.org/2598383002/diff/20001/ui/views/widget/desktop... >> ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1098: float >> clamped_opacity = std::max(0.0f, std::min(1.0f, opacity)); >> We don't do this anywhere else. It's assumed the value is 0-1. A DCHECK >> here > is >> fine (or better yet in Widget::SetOpacity() which is generally the entry >> point >> for this. > > In unsigned long cardinality = opacity * 0x1010101 there was implicit cast > to > unsigned int first, then multiplication. > After change signed int is multiplied by constant -> could cause overflow > (values bigger than 0x7FFFFFFF). > > Will move check (DCHECK) to upped level tomorrow. Thanks. > > https://codereview.chromium.org/2598383002/ -- 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/01/03 23:14:24, sky wrote: > Why does the unsigned matter here when the value is always >=0 0? > > > In unsigned long cardinality = opacity * 0x1010101 there was implicit cast > > to unsigned int first, then multiplication. > > After change signed int is multiplied by constant -> could cause overflow See (int)(0.75 * 255) == 191 191UL * 0x01010101UL = 0xBFBFBFBFUL >= 0x80000000 -> it is negative, be it signed -> i.e. formal overflow occurs. Normally, we can expect, that nothing bad happens, and on most 32-bit platforms resulting value will be 0xBFBFBFBFUL; but it is not guaranteed. On 64-bit platforms value might be 0xFFFFFFFFBFBFBFBFULL and X11 API might also be confused.
The CQ bit was checked by eustas@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ping
lgtm
Thanks for the clarification. LGTM
The CQ bit was checked by eustas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2598383002/#ps60001 (title: "Rebase")
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": 60001, "attempt_start_ts": 1485511693890610,
"parent_rev": "42a09a818af028bcf707d131de519009c020792f", "commit_rev":
"0f6dfbc4793fab2a08ea912039ca8ed6f958d81d"}
Message was sent while issue was closed.
Description was changed from ========== Fix integer overflow. BUG=676902 ========== to ========== Fix integer overflow. BUG=676902 Review-Url: https://codereview.chromium.org/2598383002 Cr-Commit-Position: refs/heads/master@{#446639} Committed: https://chromium.googlesource.com/chromium/src/+/0f6dfbc4793fab2a08ea912039ca... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/0f6dfbc4793fab2a08ea912039ca... |
