Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(496)

Issue 2537773009: Fix a bunch of undefined behavior in CheckedNumericState (Closed)

Created:
4 years ago by Nico
Modified:
4 years ago
Reviewers:
jschuh
CC:
chromium-reviews, vmpstr+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix a bunch of undefined behavior in CheckedNumericState CheckedNumericState<T, NUMERIC_INTEGER> would unconditionally cast the src type to the dest int type. If the source type was a floating point type that didn't fit in the integer type, this was undefined behavior. So only cast if is_valid_ is true. To be able to check that, move is_valid_ in front of the value_ field (since these are constexpr ctors and we don't have C++14 constexpr). BUG=669642 Committed: https://crrev.com/e5b63cc2f7896b3ed1e8d89c3cab0b077ac14cc9 Cr-Commit-Position: refs/heads/master@{#436055}

Patch Set 1 #

Total comments: 2

Patch Set 2 : comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+11 lines, -7 lines) Patch
M base/numerics/safe_math_impl.h View 1 2 chunks +11 lines, -7 lines 0 comments Download

Messages

Total messages: 13 (7 generated)
Nico
https://codereview.chromium.org/2537773009/diff/1/base/numerics/safe_math_impl.h File base/numerics/safe_math_impl.h (right): https://codereview.chromium.org/2537773009/diff/1/base/numerics/safe_math_impl.h#newcode823 base/numerics/safe_math_impl.h:823: : is_valid_(rhs.IsValid()), value_(static_cast<T>(rhs.value())) {} This one probably needs this ...
4 years ago (2016-12-02 16:46:10 UTC) #4
jschuh
I'm totally fine with the reordering. I split the state out into a separate class ...
4 years ago (2016-12-02 17:19:15 UTC) #5
Nico
all done, thanks!
4 years ago (2016-12-02 18:27:04 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2537773009/20001
4 years ago (2016-12-02 18:27:44 UTC) #9
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years ago (2016-12-02 22:26:31 UTC) #11
commit-bot: I haz the power
4 years ago (2016-12-02 22:28:46 UTC) #13
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/e5b63cc2f7896b3ed1e8d89c3cab0b077ac14cc9
Cr-Commit-Position: refs/heads/master@{#436055}

Powered by Google App Engine
This is Rietveld 408576698