Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2176053002:
Avoid integer-overflow in hb_glyph_position_t.y_advance (Closed)
|
Created:
4 years, 5 months ago by kojii Modified:
4 years, 5 months ago CC:
ajuma+watch_chromium.org, blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), jbroman, Justin Novosad, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid integer-overflow in hb_glyph_position_t.y_advance
This patch avoids integer-overflow when hb_glyph_position_t.y_advance is
LONG_MIN.
BUG=630227
Committed: https://crrev.com/17fe9abaa3e964184be9940b0b8630940785a788
Cr-Commit-Position: refs/heads/master@{#407744}
Patch Set 1 #Patch Set 2 : style fix #Patch Set 3 : Add comment #Messages
Total messages: 22 (15 generated)
The CQ bit was checked by kojii@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 checked by kojii@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.
Description was changed from ========== hboverflow BUG= ========== to ========== Avoid integer-overflow in hb_glyph_position_t.y_advance This patch avoids integer-overflow when hb_glyph_position_t.y_advance is LONG_MIN. BUG=630227 ==========
kojii@chromium.org changed reviewers: + drott@chromium.org, eae@chromium.org
PTAL. Not sure if this is worth a fix, but I suppose this is slightly faster with LIKELY and by not reading y. I'm fine to WontFix the bug if that is preferred.
I am not sure, this LGTM for me - perhaps with adding a comment that this construct is meant to avoid the overflow. However, in a related bug, where we have overflows in HarfBuzz Behdad was suggesting that we clamp the max font size a bit lower in Blink. (I think that's in FontBuilder). Perhaps that would be enough? Can we analyze what would be the right maximum font size to avoid this overflow here?
Clamping font size is a good idea, but advances can be any larger than font size, so logically speaking it's irrelevant, no? I don't know the original intention of this line, if subtraction is supposed to be faster than "if LIKELY", or if either of you have any other concerns, I'm fine to resolve the bug as WontFix. The overflow should have no harm in other than *san builds. Do you prefer that?
ok but please add a comment LGTM
The CQ bit was checked by kojii@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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by kojii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from drott@chromium.org, eae@chromium.org Link to the patchset: https://codereview.chromium.org/2176053002/#ps40001 (title: "Add comment")
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Avoid integer-overflow in hb_glyph_position_t.y_advance This patch avoids integer-overflow when hb_glyph_position_t.y_advance is LONG_MIN. BUG=630227 ========== to ========== Avoid integer-overflow in hb_glyph_position_t.y_advance This patch avoids integer-overflow when hb_glyph_position_t.y_advance is LONG_MIN. BUG=630227 Committed: https://crrev.com/17fe9abaa3e964184be9940b0b8630940785a788 Cr-Commit-Position: refs/heads/master@{#407744} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/17fe9abaa3e964184be9940b0b8630940785a788 Cr-Commit-Position: refs/heads/master@{#407744} |
