Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2013243002:
Silence ASAN int32 overflow warning (Closed)
DescriptionSilence ASAN int32 overflow warning
It's fine to overflow SK_MaxS32 by one, the subsequent cast ensures
correct clamping. But we need to cast earlier in order to make ASAN
happy.
TBR=mtklein@google.com,reed@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2013243002
Committed: https://skia.googlesource.com/skia/+/7dcb131935bda4aada0e37085c9f5e1f6a8f3842
Patch Set 1 #
Total comments: 1
Messages
Total messages: 18 (9 generated)
Description was changed from ========== Silence ASAN int32 overflow warning It's fine to overflow SK_MaxS32 by one, the subsequent cast ensures correct clamping. But we need to cast earlier in order to make ASAN happy. R=mtklein@google.com ========== to ========== Silence ASAN int32 overflow warning It's fine to overflow SK_MaxS32 by one, the subsequent cast ensures correct clamping. But we need to cast earlier in order to make ASAN happy. R=mtklein@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2013243002 ==========
Description was changed from ========== Silence ASAN int32 overflow warning It's fine to overflow SK_MaxS32 by one, the subsequent cast ensures correct clamping. But we need to cast earlier in order to make ASAN happy. R=mtklein@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2013243002 ========== to ========== Silence ASAN int32 overflow warning It's fine to overflow SK_MaxS32 by one, the subsequent cast ensures correct clamping. But we need to cast earlier in order to make ASAN happy. R=mtklein@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2013243002 ==========
fmalita@chromium.org changed reviewers: + reed@google.com
Description was changed from ========== Silence ASAN int32 overflow warning It's fine to overflow SK_MaxS32 by one, the subsequent cast ensures correct clamping. But we need to cast earlier in order to make ASAN happy. R=mtklein@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2013243002 ========== to ========== Silence ASAN int32 overflow warning It's fine to overflow SK_MaxS32 by one, the subsequent cast ensures correct clamping. But we need to cast earlier in order to make ASAN happy. R=mtklein@google.com,reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2013243002 ==========
Looks like it's working, so I'll TBR to hush the bot.
The CQ bit was checked by fmalita@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2013243002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2013243002/1
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2016-05-27 06:57 UTC
Description was changed from ========== Silence ASAN int32 overflow warning It's fine to overflow SK_MaxS32 by one, the subsequent cast ensures correct clamping. But we need to cast earlier in order to make ASAN happy. R=mtklein@google.com,reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2013243002 ========== to ========== Silence ASAN int32 overflow warning It's fine to overflow SK_MaxS32 by one, the subsequent cast ensures correct clamping. But we need to cast earlier in order to make ASAN happy. TBR=mtklein@google.com,reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2013243002 ==========
The CQ bit was unchecked by fmalita@chromium.org
The CQ bit was checked by fmalita@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2013243002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2013243002/1
Message was sent while issue was closed.
Description was changed from ========== Silence ASAN int32 overflow warning It's fine to overflow SK_MaxS32 by one, the subsequent cast ensures correct clamping. But we need to cast earlier in order to make ASAN happy. TBR=mtklein@google.com,reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2013243002 ========== to ========== Silence ASAN int32 overflow warning It's fine to overflow SK_MaxS32 by one, the subsequent cast ensures correct clamping. But we need to cast earlier in order to make ASAN happy. TBR=mtklein@google.com,reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2013243002 Committed: https://skia.googlesource.com/skia/+/7dcb131935bda4aada0e37085c9f5e1f6a8f3842 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://skia.googlesource.com/skia/+/7dcb131935bda4aada0e37085c9f5e1f6a8f3842
Message was sent while issue was closed.
https://codereview.chromium.org/2013243002/diff/1/src/effects/gradients/SkLin... File src/effects/gradients/SkLinearGradient.cpp (right): https://codereview.chromium.org/2013243002/diff/1/src/effects/gradients/SkLin... src/effects/gradients/SkLinearGradient.cpp:615: int n = SkTMin<uint32_t>(static_cast<uint32_t>(SkFloatToIntFloor(-fx * invDx)) + 1, See if I'm following this correctly? 1) We're a safe int32_t after SkFloatToIntFloor(...), with a max of 0x7fffffff. 2) Then we cast it to uint32_t and add 1. This is a safe uint32_t but a possibly wrapped int32_t (0x8000000). 3) Then we min it with count as uint32_t. Is that what makes this safe? We know count is in [0, 0x7ffffff] ? 4) That uint32_t in [0, 0x7fffffff] is finally cast back to a safe int32_t.
Message was sent while issue was closed.
On 2016/05/27 05:17:03, mtklein wrote: > https://codereview.chromium.org/2013243002/diff/1/src/effects/gradients/SkLin... > File src/effects/gradients/SkLinearGradient.cpp (right): > > https://codereview.chromium.org/2013243002/diff/1/src/effects/gradients/SkLin... > src/effects/gradients/SkLinearGradient.cpp:615: int n = > SkTMin<uint32_t>(static_cast<uint32_t>(SkFloatToIntFloor(-fx * invDx)) + 1, > See if I'm following this correctly? > > 1) We're a safe int32_t after SkFloatToIntFloor(...), with a max of > 0x7fffffff. > > 2) Then we cast it to uint32_t and add 1. This is a safe uint32_t but a > possibly wrapped int32_t (0x8000000). > > 3) Then we min it with count as uint32_t. Is that what makes this safe? We > know count is in [0, 0x7ffffff] ? > > 4) That uint32_t in [0, 0x7fffffff] is finally cast back to a safe int32_t. Correct, #3 is the key.
Message was sent while issue was closed.
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
