New issue
Advanced search Search tips

Issue 668713 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug

Blocked on:
issue 670120



Sign in to add a comment

Fix silent truncations when extracting values from CheckedNumeric types

Project Member Reported by jsc...@chromium.org, Nov 25 2016

Issue description

We have a fairly common anti-pattern where a CheckedNumeric is correctly used for all the necessary math operations, but then the value is extracted into a destination type that cannot represent the full range of source values. It looks like this:

  base CheckedNumeric<size_t> buffer_size;

  // ... various math operations on buffer_size ...

  int len = buffer_size.ValueOrDie();  // silent truncation from size_t to int.

The easiest way to detect these is just change the return type on the value returning functions from Dst to StrictNumeric<Dst> and then look at the list of compile failures.

The only solution I have in mind right now is to support overloaded return types on the value extraction functions and have them all return StrictNumeric<Dst> as a proxy class. The downside is that this breaks auto, but I can add some operator overloads to mitigate that, and a value extraction method to handle the remaining corner cases.

 
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 25 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/224f1d7c944d05d5d0a918e7f1d79b49dc55a283

commit 224f1d7c944d05d5d0a918e7f1d79b49dc55a283
Author: jschuh <jschuh@chromium.org>
Date: Fri Nov 25 20:08:48 2016

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 

Review-Url: https://codereview.chromium.org/2529913002
Cr-Commit-Position: refs/heads/master@{#434544}

[modify] https://crrev.com/224f1d7c944d05d5d0a918e7f1d79b49dc55a283/base/numerics/safe_math.h
[modify] https://crrev.com/224f1d7c944d05d5d0a918e7f1d79b49dc55a283/base/numerics/safe_numerics_unittest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Nov 26 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e17e0f13e7c600c813674aca0265021da82b7f02

commit e17e0f13e7c600c813674aca0265021da82b7f02
Author: jschuh <jschuh@chromium.org>
Date: Sat Nov 26 03:00:08 2016

Add CheckedNumeric::AssignIfValid() method

Helper for safely testing and extracting the underlying value.

BUG= 668713 

Review-Url: https://codereview.chromium.org/2535603002
Cr-Commit-Position: refs/heads/master@{#434564}

[modify] https://crrev.com/e17e0f13e7c600c813674aca0265021da82b7f02/base/numerics/safe_math.h
[modify] https://crrev.com/e17e0f13e7c600c813674aca0265021da82b7f02/base/numerics/safe_numerics_unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Nov 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/013492ec62d7e9ca751a90e6234c25773e94867f

commit 013492ec62d7e9ca751a90e6234c25773e94867f
Author: jschuh <jschuh@chromium.org>
Date: Wed Nov 30 07:58:25 2016

Fix sandbox use of base/numerics

TBR=wfh@chromium.org
R=wfh@chromium.org
BUG= 668713 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

Review-Url: https://codereview.chromium.org/2540083002
Cr-Commit-Position: refs/heads/master@{#435148}

[modify] https://crrev.com/013492ec62d7e9ca751a90e6234c25773e94867f/sandbox/win/src/win_utils.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Nov 30 2016

Project Member

Comment 5 by bugdroid1@chromium.org, Nov 30 2016

Project Member

Comment 6 by bugdroid1@chromium.org, Nov 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7dd748dcf998ad8bac843e0e1b46e7b47bb0ba46

commit 7dd748dcf998ad8bac843e0e1b46e7b47bb0ba46
Author: jschuh <jschuh@chromium.org>
Date: Wed Nov 30 08:26:57 2016

Fix courgette use of base/numerics

TBR=wfh@chromium.org
BUG= 668713 

Review-Url: https://codereview.chromium.org/2534203003
Cr-Commit-Position: refs/heads/master@{#435167}

[modify] https://crrev.com/7dd748dcf998ad8bac843e0e1b46e7b47bb0ba46/courgette/label_manager.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Nov 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/565f9c7c77356de8a134b5a519c6626f7a16d6a6

commit 565f9c7c77356de8a134b5a519c6626f7a16d6a6
Author: jschuh <jschuh@chromium.org>
Date: Wed Nov 30 08:38:24 2016

Fix use of base/numerics in IntToString

TBR=dcheng@chromium.org
R=dcheng@chromium.org
BUG= 668713 

Review-Url: https://codereview.chromium.org/2540963002
Cr-Commit-Position: refs/heads/master@{#435175}

[modify] https://crrev.com/565f9c7c77356de8a134b5a519c6626f7a16d6a6/base/strings/string_number_conversions.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Nov 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/af8a512edd329df02689a73aad98560e53f5d9f4

commit af8a512edd329df02689a73aad98560e53f5d9f4
Author: jschuh <jschuh@chromium.org>
Date: Wed Nov 30 14:47:10 2016

Fix font use of base/numerics

R=avi@chromium.org
TBR=avi@chromium.org
BUG= 668713 

Review-Url: https://codereview.chromium.org/2535723006
Cr-Commit-Position: refs/heads/master@{#435258}

[modify] https://crrev.com/af8a512edd329df02689a73aad98560e53f5d9f4/content/child/font_warmup_win.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Nov 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7c1c053a104eb258e327eb235ccda9eb493f5e08

commit 7c1c053a104eb258e327eb235ccda9eb493f5e08
Author: jschuh <jschuh@chromium.org>
Date: Wed Nov 30 15:40:32 2016

Fix ImageData use of CheckedNumeric

TBR=eae@chromium.org
R=eae@chromium.org
BUG= 668713 

Review-Url: https://codereview.chromium.org/2546453002
Cr-Commit-Position: refs/heads/master@{#435268}

[modify] https://crrev.com/7c1c053a104eb258e327eb235ccda9eb493f5e08/third_party/WebKit/Source/core/html/ImageData.cpp

Project Member

Comment 11 by bugdroid1@chromium.org, Nov 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/42745d115830d552eaa6adddc047223f49037c1a

commit 42745d115830d552eaa6adddc047223f49037c1a
Author: jschuh <jschuh@chromium.org>
Date: Wed Nov 30 17:24:49 2016

Fix WebGL use of base/numerics

TBR=kbr@chromium.org
BUG= 668713 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2540153003
Cr-Commit-Position: refs/heads/master@{#435310}

[modify] https://crrev.com/42745d115830d552eaa6adddc047223f49037c1a/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
[modify] https://crrev.com/42745d115830d552eaa6adddc047223f49037c1a/third_party/WebKit/Source/platform/graphics/gpu/WebGLImageConversion.cpp

Project Member

Comment 12 by bugdroid1@chromium.org, Nov 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8794c4be74f56e495da28f388ee30098dc16eb88

commit 8794c4be74f56e495da28f388ee30098dc16eb88
Author: jschuh <jschuh@chromium.org>
Date: Wed Nov 30 18:23:02 2016

Fix RowSizeForBufferFormatChecked use of base/numerics

R=danakj@chromium.org
TBR=danakj@chromium.org
BUG= 668713 

Review-Url: https://codereview.chromium.org/2545523002
Cr-Commit-Position: refs/heads/master@{#435341}

[modify] https://crrev.com/8794c4be74f56e495da28f388ee30098dc16eb88/ui/gfx/buffer_format_util.cc

Comment 13 by kbr@chromium.org, Dec 1 2016

Blockedon: 670120
Project Member

Comment 14 by bugdroid1@chromium.org, Dec 2 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/23a4b06b83fe4b11f6d5a48078eb543ea88c747b

commit 23a4b06b83fe4b11f6d5a48078eb543ea88c747b
Author: jschuh <jschuh@chromium.org>
Date: Fri Dec 02 02:55:08 2016

Fix silent truncations when extracting values from CheckedNumeric

This CL does a few closely intertwined things:
 - CheckedNumeric value functions now return a StrictNumeric proxy class
 - ValueFloating function is now removed (it was unused)
 - StrictNumeric now supports comparisons, ostream, and pointer math
 - Remaining callsites are fixed to support the new proxy class

TBR=vmiura@chromium.org,nparker@chromium.org
BUG= 668713 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2528243002
Cr-Commit-Position: refs/heads/master@{#435834}

[modify] https://crrev.com/23a4b06b83fe4b11f6d5a48078eb543ea88c747b/base/numerics/safe_conversions.h
[modify] https://crrev.com/23a4b06b83fe4b11f6d5a48078eb543ea88c747b/base/numerics/safe_conversions_impl.h
[modify] https://crrev.com/23a4b06b83fe4b11f6d5a48078eb543ea88c747b/base/numerics/safe_math.h
[modify] https://crrev.com/23a4b06b83fe4b11f6d5a48078eb543ea88c747b/base/numerics/safe_math_impl.h
[modify] https://crrev.com/23a4b06b83fe4b11f6d5a48078eb543ea88c747b/base/numerics/safe_numerics_unittest.cc
[modify] https://crrev.com/23a4b06b83fe4b11f6d5a48078eb543ea88c747b/chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader_unittest.cc
[modify] https://crrev.com/23a4b06b83fe4b11f6d5a48078eb543ea88c747b/chrome/utility/safe_browsing/mac/hfs.cc
[modify] https://crrev.com/23a4b06b83fe4b11f6d5a48078eb543ea88c747b/chrome/utility/safe_browsing/mac/udif.cc
[modify] https://crrev.com/23a4b06b83fe4b11f6d5a48078eb543ea88c747b/components/webcrypto/algorithms/aes_cbc.cc
[modify] https://crrev.com/23a4b06b83fe4b11f6d5a48078eb543ea88c747b/components/webcrypto/algorithms/aes_ctr.cc
[modify] https://crrev.com/23a4b06b83fe4b11f6d5a48078eb543ea88c747b/content/renderer/pepper/pepper_audio_encoder_host.cc
[modify] https://crrev.com/23a4b06b83fe4b11f6d5a48078eb543ea88c747b/content/renderer/pepper/pepper_media_stream_track_host_base.cc
[modify] https://crrev.com/23a4b06b83fe4b11f6d5a48078eb543ea88c747b/device/hid/hid_connection_mac.cc
[modify] https://crrev.com/23a4b06b83fe4b11f6d5a48078eb543ea88c747b/gpu/command_buffer/client/gles2_implementation.cc
[modify] https://crrev.com/23a4b06b83fe4b11f6d5a48078eb543ea88c747b/gpu/command_buffer/service/gles2_cmd_decoder.cc
[modify] https://crrev.com/23a4b06b83fe4b11f6d5a48078eb543ea88c747b/media/base/video_util.cc
[modify] https://crrev.com/23a4b06b83fe4b11f6d5a48078eb543ea88c747b/third_party/crashpad/README.chromium
[modify] https://crrev.com/23a4b06b83fe4b11f6d5a48078eb543ea88c747b/third_party/crashpad/crashpad/util/file/string_file.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Dec 7 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/crashpad/crashpad.git/+/6b09b08a22028c4e699f4afdc6c00eef7de8e3fe

commit 6b09b08a22028c4e699f4afdc6c00eef7de8e3fe
Author: Scott Graham <scottmg@chromium.org>
Date: Wed Dec 07 22:53:16 2016

Update util/file/string_file.cc for new base/numerics API

The code was not incorrect before, but this expression is simpler.
Upstream of change made at https://codereview.chromium.org/2528243002.

R=mark@chromium.org
BUG= chromium:668713 

Change-Id: Idae36bd8312666a3254eda02713869776fec0248
Reviewed-on: https://chromium-review.googlesource.com/417981
Reviewed-by: Mark Mentovai <mark@chromium.org>

[modify] https://crrev.com/6b09b08a22028c4e699f4afdc6c00eef7de8e3fe/util/file/string_file.cc

Project Member

Comment 16 by bugdroid1@chromium.org, Dec 8 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1bb44b3c5f9bc262640a686ad8bbfec284902142

commit 1bb44b3c5f9bc262640a686ad8bbfec284902142
Author: scottmg <scottmg@chromium.org>
Date: Thu Dec 08 14:09:15 2016

Remove unused cassert include in base/numerics/safe_conversions.h

assert() does not appear in base/numerics.

Noticed while trying to roll Chromium numerics into mini_chromium to
roll mini_chromium into Crashpad to upstream
https://codereview.chromium.org/2528243002 into Crashpad to roll
Crashpad back into Chromium. Sigh.

BUG= 668713 
R=jschuh@chromium.org

Review-Url: https://codereview.chromium.org/2557743005
Cr-Commit-Position: refs/heads/master@{#437240}

[modify] https://crrev.com/1bb44b3c5f9bc262640a686ad8bbfec284902142/base/numerics/safe_conversions.h

Project Member

Comment 17 by bugdroid1@chromium.org, Dec 8 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ef3f35d7e604af1c77304a8692fc2d936fc024df

commit ef3f35d7e604af1c77304a8692fc2d936fc024df
Author: scottmg <scottmg@chromium.org>
Date: Thu Dec 08 17:44:02 2016

Remove more unnecessary includes, limits.h and climits in base/numerics

BUG= 668713 
R=jschuh@chromium.org

Review-Url: https://codereview.chromium.org/2558993002
Cr-Commit-Position: refs/heads/master@{#437276}

[modify] https://crrev.com/ef3f35d7e604af1c77304a8692fc2d936fc024df/base/numerics/safe_conversions_impl.h

Status: Fixed (was: Assigned)
I'm gonna call this done. I'll open a new bug if I want to change ValueOrDefault or the results from the magic casts.

Sign in to add a comment