Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2405453002: Fix Integer-overflow in base::Time::FromExploded. (Closed)

Created:
3 years ago by maksims (do not use this acc)
Modified:
2 years, 12 months ago
CC:
anjitkan_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix Integer-overflow in base::Time::FromExploded. The old implementation doesn't handle possible overflows, when year is too large, for example. It makes a result to be larger than 2^63 - 1, which results in overflow. Fix Posix: use safe_math.h for multiplication and addition. If overflow occurs, return possibly maximum platform dependent value. Fix Mac and Win: if safe cast is impossible, return Time(0). Fix media and components: use day of week as well, as long as unused variable results in undefined behavior and overflow BUG=653445 Committed: https://crrev.com/eebbdecabe619cc9887152bd9371f3e2bc7711b6 Cr-Commit-Position: refs/heads/master@{#427064}

Patch Set 1 #

Patch Set 2 : use safe_math.h #

Patch Set 3 : remove unused var #

Patch Set 4 : fix win overflow #

Patch Set 5 : solution for mac #

Patch Set 6 : move method under ifdef #

Total comments: 17

Patch Set 7 : ... #

Total comments: 1

Patch Set 8 : fix unittests #

Patch Set 9 : one more fix #

Patch Set 10 : media unittests fix #

Patch Set 11 : rebased and fixed net unittest #

Unified diffs Side-by-side diffs Delta from patch set Stats (+63 lines, -21 lines) Patch
M base/time/time.h View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M base/time/time_mac.cc View 1 2 3 4 5 6 1 chunk +12 lines, -3 lines 0 comments Download
M base/time/time_posix.cc View 1 2 3 4 5 6 3 chunks +18 lines, -6 lines 0 comments Download
M base/time/time_unittest.cc View 1 2 3 1 chunk +5 lines, -0 lines 0 comments Download
M base/time/time_win.cc View 1 2 3 4 5 6 2 chunks +19 lines, -9 lines 0 comments Download
M components/data_reduction_proxy/core/browser/data_usage_store_unittest.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M media/ffmpeg/ffmpeg_common.cc View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
M media/formats/webm/webm_info_parser.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M media/test/pipeline_integration_test.cc View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M net/http/http_util_unittest.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -2 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 132 (98 generated)
maksims (do not use this acc)
please take a look. mmenke@, basically that wasn't an issue from my side. I was ...
3 years ago (2016-10-07 13:28:55 UTC) #9
maksims_
On 2016/10/07 13:28:55, maksims wrote: > please take a look. > > mmenke@, basically that ...
3 years ago (2016-10-07 15:12:51 UTC) #12
mmenke
On 2016/10/07 15:12:51, maksims_ wrote: > On 2016/10/07 13:28:55, maksims wrote: > > please take ...
3 years ago (2016-10-07 16:09:28 UTC) #13
maksims (do not use this acc)
On 2016/10/07 16:09:28, mmenke wrote: > On 2016/10/07 15:12:51, maksims_ wrote: > > On 2016/10/07 ...
3 years ago (2016-10-10 10:51:22 UTC) #43
mmenke
On 2016/10/10 10:51:22, maksims wrote: > On 2016/10/07 16:09:28, mmenke wrote: > > On 2016/10/07 ...
3 years ago (2016-10-10 14:14:17 UTC) #44
mmenke
On 2016/10/10 14:14:17, mmenke wrote: > On 2016/10/10 10:51:22, maksims wrote: > > On 2016/10/07 ...
3 years ago (2016-10-10 22:05:03 UTC) #45
maksims (do not use this acc)
On 2016/10/10 22:05:03, mmenke wrote: > On 2016/10/10 14:14:17, mmenke wrote: > > On 2016/10/10 ...
3 years ago (2016-10-12 13:39:06 UTC) #46
mmenke
On 2016/10/12 13:39:06, maksims wrote: > On 2016/10/10 22:05:03, mmenke wrote: > > On 2016/10/10 ...
3 years ago (2016-10-12 14:44:41 UTC) #47
mmenke
On 2016/10/12 14:44:41, mmenke wrote: > On 2016/10/12 13:39:06, maksims wrote: > > On 2016/10/10 ...
3 years ago (2016-10-12 14:46:15 UTC) #48
maksims_
On 2016/10/12 14:46:15, mmenke wrote: > On 2016/10/12 14:44:41, mmenke wrote: > > On 2016/10/12 ...
3 years ago (2016-10-12 15:21:30 UTC) #49
maksims_
On 2016/10/12 15:21:30, maksims_ wrote: > On 2016/10/12 14:46:15, mmenke wrote: > > On 2016/10/12 ...
3 years ago (2016-10-12 15:22:50 UTC) #50
mmenke
On 2016/10/12 15:22:50, maksims_ wrote: > On 2016/10/12 15:21:30, maksims_ wrote: > > On 2016/10/12 ...
3 years ago (2016-10-12 15:27:35 UTC) #51
maksims_
On 2016/10/12 15:27:35, mmenke wrote: > On 2016/10/12 15:22:50, maksims_ wrote: > > On 2016/10/12 ...
3 years ago (2016-10-12 15:39:24 UTC) #52
mmenke
On 2016/10/12 15:39:24, maksims_ wrote: > On 2016/10/12 15:27:35, mmenke wrote: > > On 2016/10/12 ...
3 years ago (2016-10-12 15:43:04 UTC) #53
mmenke
On 2016/10/12 15:43:04, mmenke wrote: > On 2016/10/12 15:39:24, maksims_ wrote: > > On 2016/10/12 ...
3 years ago (2016-10-12 15:45:02 UTC) #54
maksims_
On 2016/10/12 15:45:02, mmenke wrote: > On 2016/10/12 15:43:04, mmenke wrote: > > On 2016/10/12 ...
3 years ago (2016-10-12 15:51:50 UTC) #55
mmenke
On 2016/10/12 15:51:50, maksims_ wrote: > On 2016/10/12 15:45:02, mmenke wrote: > > On 2016/10/12 ...
3 years ago (2016-10-12 16:33:35 UTC) #56
maksims (do not use this acc)
please review
3 years ago (2016-10-18 12:59:07 UTC) #83
mmenke
https://codereview.chromium.org/2405453002/diff/280001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/2405453002/diff/280001/base/time/time_win.cc#newcode113 base/time/time_win.cc:113: const int kMaxWinWordSize = 65535; This should just be ...
3 years ago (2016-10-18 15:04:50 UTC) #87
miu
https://codereview.chromium.org/2405453002/diff/280001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/2405453002/diff/280001/base/time/time.h#newcode548 base/time/time.h:548: // month is set to 31 on a 28-30 ...
3 years ago (2016-10-18 19:55:13 UTC) #88
maksims (do not use this acc)
https://chromiumcodereview.appspot.com/2405453002/diff/280001/base/time/time.h File base/time/time.h (right): https://chromiumcodereview.appspot.com/2405453002/diff/280001/base/time/time.h#newcode548 base/time/time.h:548: // month is set to 31 on a 28-30 ...
3 years ago (2016-10-19 16:41:05 UTC) #98
mmenke
LGTM
3 years ago (2016-10-19 17:34:48 UTC) #99
miu
lgtm % nit: https://chromiumcodereview.appspot.com/2405453002/diff/320001/base/time/time.h File base/time/time.h (right): https://chromiumcodereview.appspot.com/2405453002/diff/320001/base/time/time.h#newcode548 base/time/time.h:548: // month is set to 31 ...
3 years ago (2016-10-19 19:54:33 UTC) #100
maksims (do not use this acc)
liberato@chromium.org: Please review changes in components blundell@chromium.org: Please review changes in media
3 years ago (2016-10-20 12:00:54 UTC) #106
maksims (do not use this acc)
On 2016/10/20 12:00:54, maksims wrote: > mailto:liberato@chromium.org: Please review changes in > components > > ...
2 years, 12 months ago (2016-10-20 15:27:01 UTC) #112
liberato (no reviews please)
media/ lgtm. out of curiosity, which platform cares about the day of the week that ...
2 years, 12 months ago (2016-10-20 15:38:47 UTC) #113
liberato (no reviews please)
media/ lgtm. out of curiosity, which platform cares about the day of the week that ...
2 years, 12 months ago (2016-10-20 15:38:51 UTC) #114
mmenke
On 2016/10/20 15:38:51, liberato wrote: > media/ lgtm. > > out of curiosity, which platform ...
2 years, 12 months ago (2016-10-20 15:42:33 UTC) #115
mmenke
On 2016/10/20 15:42:33, mmenke wrote: > On 2016/10/20 15:38:51, liberato wrote: > > media/ lgtm. ...
2 years, 12 months ago (2016-10-20 15:43:47 UTC) #116
liberato (no reviews please)
On 2016/10/20 15:43:47, mmenke wrote: > On 2016/10/20 15:42:33, mmenke wrote: > > On 2016/10/20 ...
2 years, 12 months ago (2016-10-20 15:51:21 UTC) #117
blundell
//components lgtm
2 years, 12 months ago (2016-10-21 16:31:27 UTC) #122
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/2405453002/460001
2 years, 12 months ago (2016-10-24 11:22:27 UTC) #128
commit-bot: I haz the power
Committed patchset #11 (id:460001)
2 years, 12 months ago (2016-10-24 14:07:54 UTC) #130
commit-bot: I haz the power
2 years, 12 months ago (2016-10-24 14:10:53 UTC) #132
Message was sent while issue was closed.
Patchset 11 (id:??) landed as
https://crrev.com/eebbdecabe619cc9887152bd9371f3e2bc7711b6
Cr-Commit-Position: refs/heads/master@{#427064}

Powered by Google App Engine
This is Rietveld 408576698