Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2529913002:
Support overriding the CheckedNumeric value extraction functions (Closed)
DescriptionSupport overriding the CheckedNumeric value extraction functions
This adds support for explicit return types and makes the failure
handler overridable for consistency with the cast functions.
Most importantly, this CL is the first step in fixing the silent
truncations we have on extracting the values from CheckedNumeric.
BUG=668713
Committed: https://crrev.com/224f1d7c944d05d5d0a918e7f1d79b49dc55a283
Cr-Commit-Position: refs/heads/master@{#434544}
Patch Set 1 #Patch Set 2 : refactor + tests + docs #Patch Set 3 : nits #
Total comments: 4
Patch Set 4 : tweaks and comment fixes #Messages
Total messages: 20 (12 generated)
The CQ bit was checked by jschuh@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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
scottmg@chromium.org changed reviewers: + scottmg@chromium.org
https://codereview.chromium.org/2529913002/diff/40001/base/numerics/safe_math.h File base/numerics/safe_math.h (right): https://codereview.chromium.org/2529913002/diff/40001/base/numerics/safe_math... base/numerics/safe_math.h:121: // parameter, which will trigger a CHECK if the value is not in bounds for This comment is misleading since there's an override. Are you sure you want to overload this to actually be ValueOrDieMostOfTheTime()? How about adding ValueOrLog()? Alternatively, if it's just for tests, maybe using a death test? https://codereview.chromium.org/2529913002/diff/40001/base/numerics/safe_math... base/numerics/safe_math.h:138: constexpr Dst ValueOrDefault(const Src default_value) const { Similar for the other ones, ValueOrDefault() and ValueOrDefaultAsTargetType<T>() or whatever you want to call them might be clearer.
Description was changed from ========== Support overriding the CheckedNumeric value extraction functions This adds support for explicit return types and makes the failure handler overridable for consistency with the cast functions. ========== to ========== Support overriding the CheckedNumeric value extraction functions This adds support for explicit return types and makes the failure handler overridable for consistency with the cast functions. Most importantly, this CL is the first step in fixing the silent truncations we have on extracting the values from CheckedNumeric. BUG=668713 ==========
I created a bug that better explains the types of silent truncations I'm working towards fixing with this CL. Here's an example of the anti-pattern in crash code: https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/util/file/... As noted in the comment, I feel the names on the functions are still correct because you can't extract the value if you override the crash handler. And the library already regularly uses the pattern of explicitly specifying a range-checked destination type (e.g. checked_cast, static_cast, CheckedNumeric::Cast, and CheckNum), although I expect to use this more internally than see callers use it externally. https://codereview.chromium.org/2529913002/diff/40001/base/numerics/safe_math.h File base/numerics/safe_math.h (right): https://codereview.chromium.org/2529913002/diff/40001/base/numerics/safe_math... base/numerics/safe_math.h:121: // parameter, which will trigger a CHECK if the value is not in bounds for On 2016/11/24 20:23:37, scottmg wrote: > This comment is misleading since there's an override. > > Are you sure you want to overload this to actually be ValueOrDieMostOfTheTime()? > How about adding ValueOrLog()? Alternatively, if it's just for tests, maybe > using a death test? I've tweaked the comment, and I'm okay with the override because if you prevent the CHECK then you can't get the value out. So, it really still is ValueOrDie. https://codereview.chromium.org/2529913002/diff/40001/base/numerics/safe_math... base/numerics/safe_math.h:138: constexpr Dst ValueOrDefault(const Src default_value) const { On 2016/11/24 20:23:37, scottmg wrote: > Similar for the other ones, ValueOrDefault() and ValueOrDefaultAsTargetType<T>() > or whatever you want to call them might be clearer. This comment was actually wrong, so I fixed it.
The CQ bit was checked by jschuh@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...
lgtm
On 2016/11/25 14:56:39, jschuh (very slow) wrote: > I created a bug that better explains the types of silent truncations I'm working > towards fixing with this CL. Here's an example of the anti-pattern in crash > code: > > https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/util/file/... https://codereview.chromium.org/434103003/diff/1/util/file/string_file_writer.cc FWIW (not much), I would have said that code is correct because it's going signed->unsigned with AssignIfInRange call, followed by "truncating" back to the original signed type without modification. But maybe I'm confused? (It's also fortunately test-only code too, not that that means it should be incorrect of course.) > > As noted in the comment, I feel the names on the functions are still correct > because you can't extract the value if you override the crash handler. And the > library already regularly uses the pattern of explicitly specifying a > range-checked destination type (e.g. checked_cast, static_cast, > CheckedNumeric::Cast, and CheckNum), although I expect to use this more > internally than see callers use it externally. > > https://codereview.chromium.org/2529913002/diff/40001/base/numerics/safe_math.h > File base/numerics/safe_math.h (right): > > https://codereview.chromium.org/2529913002/diff/40001/base/numerics/safe_math... > base/numerics/safe_math.h:121: // parameter, which will trigger a CHECK if the > value is not in bounds for > On 2016/11/24 20:23:37, scottmg wrote: > > This comment is misleading since there's an override. > > > > Are you sure you want to overload this to actually be > ValueOrDieMostOfTheTime()? > > How about adding ValueOrLog()? Alternatively, if it's just for tests, maybe > > using a death test? > > I've tweaked the comment, and I'm okay with the override because if you prevent > the CHECK then you can't get the value out. So, it really still is ValueOrDie. > > https://codereview.chromium.org/2529913002/diff/40001/base/numerics/safe_math... > base/numerics/safe_math.h:138: constexpr Dst ValueOrDefault(const Src > default_value) const { > On 2016/11/24 20:23:37, scottmg wrote: > > Similar for the other ones, ValueOrDefault() and > ValueOrDefaultAsTargetType<T>() > > or whatever you want to call them might be clearer. > > This comment was actually wrong, so I fixed it.
On 2016/11/25 17:16:17, scottmg (slow on 25nov) wrote: > On 2016/11/25 14:56:39, jschuh (very slow) wrote: > > I created a bug that better explains the types of silent truncations I'm > working > > towards fixing with this CL. Here's an example of the anti-pattern in crash > > code: > > > > > https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/util/file/... > > https://codereview.chromium.org/434103003/diff/1/util/file/string_file_writer.cc > > FWIW (not much), I would have said that code is correct because it's going > signed->unsigned with AssignIfInRange call, followed by "truncating" back to the > original signed type without modification. But maybe I'm confused? > > (It's also fortunately test-only code too, not that that means it should be > incorrect of course.) Ah, you're right. I have a dependent StrictNumeric CL and I just grabbed one of the first compile errors in code I figured you'd be familiar with. But now I just look dumb. > > As noted in the comment, I feel the names on the functions are still correct > > because you can't extract the value if you override the crash handler. And the > > library already regularly uses the pattern of explicitly specifying a > > range-checked destination type (e.g. checked_cast, static_cast, > > CheckedNumeric::Cast, and CheckNum), although I expect to use this more > > internally than see callers use it externally. > > > > > https://codereview.chromium.org/2529913002/diff/40001/base/numerics/safe_math.h > > File base/numerics/safe_math.h (right): > > > > > https://codereview.chromium.org/2529913002/diff/40001/base/numerics/safe_math... > > base/numerics/safe_math.h:121: // parameter, which will trigger a CHECK if the > > value is not in bounds for > > On 2016/11/24 20:23:37, scottmg wrote: > > > This comment is misleading since there's an override. > > > > > > Are you sure you want to overload this to actually be > > ValueOrDieMostOfTheTime()? > > > How about adding ValueOrLog()? Alternatively, if it's just for tests, maybe > > > using a death test? > > > > I've tweaked the comment, and I'm okay with the override because if you > prevent > > the CHECK then you can't get the value out. So, it really still is ValueOrDie. > > > > > https://codereview.chromium.org/2529913002/diff/40001/base/numerics/safe_math... > > base/numerics/safe_math.h:138: constexpr Dst ValueOrDefault(const Src > > default_value) const { > > On 2016/11/24 20:23:37, scottmg wrote: > > > Similar for the other ones, ValueOrDefault() and > > ValueOrDefaultAsTargetType<T>() > > > or whatever you want to call them might be clearer. > > > > This comment was actually wrong, so I fixed it.
The CQ bit was unchecked by jschuh@chromium.org
The CQ bit was checked by jschuh@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": 60001, "attempt_start_ts": 1480095542255390,
"parent_rev": "5f792a5d07dd68134b73f4b43b155bdb6656ec15", "commit_rev":
"89ae214cdb53abbcf26b1a781c7d950def8696d9"}
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Support overriding the CheckedNumeric value extraction functions This adds support for explicit return types and makes the failure handler overridable for consistency with the cast functions. Most importantly, this CL is the first step in fixing the silent truncations we have on extracting the values from CheckedNumeric. BUG=668713 ========== to ========== Support overriding the CheckedNumeric value extraction functions This adds support for explicit return types and makes the failure handler overridable for consistency with the cast functions. Most importantly, this CL is the first step in fixing the silent truncations we have on extracting the values from CheckedNumeric. BUG=668713 Committed: https://crrev.com/224f1d7c944d05d5d0a918e7f1d79b49dc55a283 Cr-Commit-Position: refs/heads/master@{#434544} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/224f1d7c944d05d5d0a918e7f1d79b49dc55a283 Cr-Commit-Position: refs/heads/master@{#434544} |
