|
|
Created:
4 years, 2 months ago by maksims (do not use this acc) Modified:
4 years, 1 month ago CC:
anjitkan_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix 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 #
Dependent Patchsets: Messages
Total messages: 132 (98 generated)
The CQ bit was checked by maksim.sisov@intel.com 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...
Description was changed from ========== time BUG= ========== to ========== Fix Integer-overflow in base::Time::FromExploded. The old implementation doesn't handle possible overflows, when year is too large, for example. It leads for msecs to be larger than 2^63 - 1, which results in overflow. Fix: check by precondition both multiplication and addition for a possible overflow before doing actual operation. BUG=653445 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) 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 maksim.sisov@intel.com 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...
maksim.sisov@intel.com changed reviewers: + miu@chromium.org, mmenke@chromium.org
please take a look. mmenke@, basically that wasn't an issue from my side. I was just selected because the last who touched these files was me.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
On 2016/10/07 13:28:55, maksims wrote: > please take a look. > > mmenke@, basically that wasn't an issue from my side. I was just selected > because the last who touched these files was me. Please, don't review yet.
On 2016/10/07 15:12:51, maksims_ wrote: > On 2016/10/07 13:28:55, maksims wrote: > > please take a look. > > > > mmenke@, basically that wasn't an issue from my side. I was just selected > > because the last who touched these files was me. > > Please, don't review yet. Rather than rolling your own solution, see safe_math.h
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by maksim.sisov@intel.com 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 maksim.sisov@intel.com to run a CQ dry run
Description was changed from ========== Fix Integer-overflow in base::Time::FromExploded. The old implementation doesn't handle possible overflows, when year is too large, for example. It leads for msecs to be larger than 2^63 - 1, which results in overflow. Fix: check by precondition both multiplication and addition for a possible overflow before doing actual operation. BUG=653445 ========== to ========== Fix Integer-overflow in base::Time::FromExploded. The old implementation doesn't handle possible overflows, when year is too large, for example. It leads for microsecs to be larger than 2^63 - 1, which results in overflow. Fix: check by precondition both multiplication and addition for a possible overflow before doing actual operation. BUG=653445 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix Integer-overflow in base::Time::FromExploded. The old implementation doesn't handle possible overflows, when year is too large, for example. It leads for microsecs to be larger than 2^63 - 1, which results in overflow. Fix: check by precondition both multiplication and addition for a possible overflow before doing actual operation. BUG=653445 ========== to ========== Fix Integer-overflow in base::Time::FromExploded. The old implementation doesn't handle possible overflows, when year is too large, for example. It makes result to be larger than 2^63 - 1, which results in overflow. Fix: check by precondition both multiplication and addition for a possible overflow before doing actual operation. BUG=653445 ==========
Description was changed from ========== Fix Integer-overflow in base::Time::FromExploded. The old implementation doesn't handle possible overflows, when year is too large, for example. It makes result to be larger than 2^63 - 1, which results in overflow. Fix: check by precondition both multiplication and addition for a possible overflow before doing actual operation. BUG=653445 ========== to ========== 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: check by precondition both multiplication and addition for a possible overflow before doing actual operation. BUG=653445 ==========
Description was changed from ========== 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: check by precondition both multiplication and addition for a possible overflow before doing actual operation. BUG=653445 ========== to ========== 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: use safe_math.h for multiplication and addition. If overflow occurs, return possibly maximum platform dependent value. BUG=653445 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by maksim.sisov@intel.com 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...
Patchset #3 (id:60001) has been deleted
Patchset #2 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by maksim.sisov@intel.com 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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by maksim.sisov@intel.com 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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by maksim.sisov@intel.com 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_...)
On 2016/10/07 16:09:28, mmenke wrote: > On 2016/10/07 15:12:51, maksims_ wrote: > > On 2016/10/07 13:28:55, maksims wrote: > > > please take a look. > > > > > > mmenke@, basically that wasn't an issue from my side. I was just selected > > > because the last who touched these files was me. > > > > Please, don't review yet. > > Rather than rolling your own solution, see safe_math.h There is an overflow in windows as well, but those windows system functions just succeed with what ever value there is. Mac behaves very strange. It just doesn't have overflows. Any suggestions?
On 2016/10/10 10:51:22, maksims wrote: > On 2016/10/07 16:09:28, mmenke wrote: > > On 2016/10/07 15:12:51, maksims_ wrote: > > > On 2016/10/07 13:28:55, maksims wrote: > > > > please take a look. > > > > > > > > mmenke@, basically that wasn't an issue from my side. I was just selected > > > > because the last who touched these files was me. > > > > > > Please, don't review yet. > > > > Rather than rolling your own solution, see safe_math.h > > There is an overflow in windows as well, but those windows system functions just > succeed with what ever value there is. > > Mac behaves very strange. It just doesn't have overflows. Any suggestions? I'll play with your patch set on windows and see what's going on, but may not have time to do so in the next day or two.
On 2016/10/10 14:14:17, mmenke wrote: > On 2016/10/10 10:51:22, maksims wrote: > > On 2016/10/07 16:09:28, mmenke wrote: > > > On 2016/10/07 15:12:51, maksims_ wrote: > > > > On 2016/10/07 13:28:55, maksims wrote: > > > > > please take a look. > > > > > > > > > > mmenke@, basically that wasn't an issue from my side. I was just > selected > > > > > because the last who touched these files was me. > > > > > > > > Please, don't review yet. > > > > > > Rather than rolling your own solution, see safe_math.h > > > > There is an overflow in windows as well, but those windows system functions > just > > succeed with what ever value there is. > > > > Mac behaves very strange. It just doesn't have overflows. Any suggestions? > > I'll play with your patch set on windows and see what's going on, but may not > have time to do so in the next day or two. The issue on windows is that Time::FromeExploded uses static_cast<WORD> on the input data (A word is an unsigned 16 bit int). So for windows, need to make all those casts safe. Looks like you got the tests passing on Mac?
On 2016/10/10 22:05:03, mmenke wrote: > On 2016/10/10 14:14:17, mmenke wrote: > > On 2016/10/10 10:51:22, maksims wrote: > > > On 2016/10/07 16:09:28, mmenke wrote: > > > > On 2016/10/07 15:12:51, maksims_ wrote: > > > > > On 2016/10/07 13:28:55, maksims wrote: > > > > > > please take a look. > > > > > > > > > > > > mmenke@, basically that wasn't an issue from my side. I was just > > selected > > > > > > because the last who touched these files was me. > > > > > > > > > > Please, don't review yet. > > > > > > > > Rather than rolling your own solution, see safe_math.h > > > > > > There is an overflow in windows as well, but those windows system functions > > just > > > succeed with what ever value there is. > > > > > > Mac behaves very strange. It just doesn't have overflows. Any suggestions? > > > > I'll play with your patch set on windows and see what's going on, but may not > > have time to do so in the next day or two. > > The issue on windows is that Time::FromeExploded uses static_cast<WORD> on the > input data (A word is an unsigned 16 bit int). So for windows, need to make all > those casts safe. > > Looks like you got the tests passing on Mac? Internal Mac's methods seem to handle overflows. But there is one place where overflow can occur still (its when base::Time is created). About the Windows platform. So, maximum year it can take is 30287. How about calculating microseconds from the remainder years? For example, if the date is 1-2-30999. reminder = 30999-30287. ... do stuff with windows functions using other dates + year 30287 ... calculate microsecs of the remainder counting leap years as well and then sum up. What do you think?
On 2016/10/12 13:39:06, maksims wrote: > On 2016/10/10 22:05:03, mmenke wrote: > > On 2016/10/10 14:14:17, mmenke wrote: > > > On 2016/10/10 10:51:22, maksims wrote: > > > > On 2016/10/07 16:09:28, mmenke wrote: > > > > > On 2016/10/07 15:12:51, maksims_ wrote: > > > > > > On 2016/10/07 13:28:55, maksims wrote: > > > > > > > please take a look. > > > > > > > > > > > > > > mmenke@, basically that wasn't an issue from my side. I was just > > > selected > > > > > > > because the last who touched these files was me. > > > > > > > > > > > > Please, don't review yet. > > > > > > > > > > Rather than rolling your own solution, see safe_math.h > > > > > > > > There is an overflow in windows as well, but those windows system > functions > > > just > > > > succeed with what ever value there is. > > > > > > > > Mac behaves very strange. It just doesn't have overflows. Any suggestions? > > > > > > I'll play with your patch set on windows and see what's going on, but may > not > > > have time to do so in the next day or two. > > > > The issue on windows is that Time::FromeExploded uses static_cast<WORD> on the > > input data (A word is an unsigned 16 bit int). So for windows, need to make > all > > those casts safe. > > > > Looks like you got the tests passing on Mac? > > Internal Mac's methods seem to handle overflows. But there is one place where > overflow can occur still (its when base::Time is created). > > About the Windows platform. So, maximum year it can take is 30287. How about > calculating microseconds from the remainder years? > For example, if the date is 1-2-30999. > reminder = 30999-30287. > ... do stuff with windows functions using other dates + year 30287 > ... > calculate microsecs of the remainder counting leap years as well and then sum > up. > > What do you think? I suggested that on https://crbug.com/653445 because it's a real problem we're currently running into. I don't think it's worth the extra complexity here, since it's only an approximation and the current code already handles all the way up until year 30287.
On 2016/10/12 14:44:41, mmenke wrote: > On 2016/10/12 13:39:06, maksims wrote: > > On 2016/10/10 22:05:03, mmenke wrote: > > > On 2016/10/10 14:14:17, mmenke wrote: > > > > On 2016/10/10 10:51:22, maksims wrote: > > > > > On 2016/10/07 16:09:28, mmenke wrote: > > > > > > On 2016/10/07 15:12:51, maksims_ wrote: > > > > > > > On 2016/10/07 13:28:55, maksims wrote: > > > > > > > > please take a look. > > > > > > > > > > > > > > > > mmenke@, basically that wasn't an issue from my side. I was just > > > > selected > > > > > > > > because the last who touched these files was me. > > > > > > > > > > > > > > Please, don't review yet. > > > > > > > > > > > > Rather than rolling your own solution, see safe_math.h > > > > > > > > > > There is an overflow in windows as well, but those windows system > > functions > > > > just > > > > > succeed with what ever value there is. > > > > > > > > > > Mac behaves very strange. It just doesn't have overflows. Any > suggestions? > > > > > > > > I'll play with your patch set on windows and see what's going on, but may > > not > > > > have time to do so in the next day or two. > > > > > > The issue on windows is that Time::FromeExploded uses static_cast<WORD> on > the > > > input data (A word is an unsigned 16 bit int). So for windows, need to make > > all > > > those casts safe. > > > > > > Looks like you got the tests passing on Mac? > > > > Internal Mac's methods seem to handle overflows. But there is one place where > > overflow can occur still (its when base::Time is created). > > > > About the Windows platform. So, maximum year it can take is 30287. How about > > calculating microseconds from the remainder years? > > For example, if the date is 1-2-30999. > > reminder = 30999-30287. > > ... do stuff with windows functions using other dates + year 30287 > > ... > > calculate microsecs of the remainder counting leap years as well and then sum > > up. > > > > What do you think? > > I suggested that on https://crbug.com/653445 because it's a real problem we're > currently running into. I don't think it's worth the extra complexity here, > since it's only an approximation and the current code already handles all the > way up until year 30287. Also, words are actually unsigned 16-bit ints, which should mean up to year 65535.
On 2016/10/12 14:46:15, mmenke wrote: > On 2016/10/12 14:44:41, mmenke wrote: > > On 2016/10/12 13:39:06, maksims wrote: > > > On 2016/10/10 22:05:03, mmenke wrote: > > > > On 2016/10/10 14:14:17, mmenke wrote: > > > > > On 2016/10/10 10:51:22, maksims wrote: > > > > > > On 2016/10/07 16:09:28, mmenke wrote: > > > > > > > On 2016/10/07 15:12:51, maksims_ wrote: > > > > > > > > On 2016/10/07 13:28:55, maksims wrote: > > > > > > > > > please take a look. > > > > > > > > > > > > > > > > > > mmenke@, basically that wasn't an issue from my side. I was just > > > > > selected > > > > > > > > > because the last who touched these files was me. > > > > > > > > > > > > > > > > Please, don't review yet. > > > > > > > > > > > > > > Rather than rolling your own solution, see safe_math.h > > > > > > > > > > > > There is an overflow in windows as well, but those windows system > > > functions > > > > > just > > > > > > succeed with what ever value there is. > > > > > > > > > > > > Mac behaves very strange. It just doesn't have overflows. Any > > suggestions? > > > > > > > > > > I'll play with your patch set on windows and see what's going on, but > may > > > not > > > > > have time to do so in the next day or two. > > > > > > > > The issue on windows is that Time::FromeExploded uses static_cast<WORD> on > > the > > > > input data (A word is an unsigned 16 bit int). So for windows, need to > make > > > all > > > > those casts safe. > > > > > > > > Looks like you got the tests passing on Mac? > > > > > > Internal Mac's methods seem to handle overflows. But there is one place > where > > > overflow can occur still (its when base::Time is created). > > > > > > About the Windows platform. So, maximum year it can take is 30287. How about > > > calculating microseconds from the remainder years? > > > For example, if the date is 1-2-30999. > > > reminder = 30999-30287. > > > ... do stuff with windows functions using other dates + year 30287 > > > ... > > > calculate microsecs of the remainder counting leap years as well and then > sum > > > up. > > > > > > What do you think? > > > > I suggested that on https://crbug.com/653445 because it's a real problem we're > > currently running into. I don't think it's worth the extra complexity here, > > since it's only an approximation and the current code already handles all the > > way up until year 30287. > > Also, words are actually unsigned 16-bit ints, which should mean up to year > 65535. Systemtime limits year from 1601 to 30287
On 2016/10/12 15:21:30, maksims_ wrote: > On 2016/10/12 14:46:15, mmenke wrote: > > On 2016/10/12 14:44:41, mmenke wrote: > > > On 2016/10/12 13:39:06, maksims wrote: > > > > On 2016/10/10 22:05:03, mmenke wrote: > > > > > On 2016/10/10 14:14:17, mmenke wrote: > > > > > > On 2016/10/10 10:51:22, maksims wrote: > > > > > > > On 2016/10/07 16:09:28, mmenke wrote: > > > > > > > > On 2016/10/07 15:12:51, maksims_ wrote: > > > > > > > > > On 2016/10/07 13:28:55, maksims wrote: > > > > > > > > > > please take a look. > > > > > > > > > > > > > > > > > > > > mmenke@, basically that wasn't an issue from my side. I was > just > > > > > > selected > > > > > > > > > > because the last who touched these files was me. > > > > > > > > > > > > > > > > > > Please, don't review yet. > > > > > > > > > > > > > > > > Rather than rolling your own solution, see safe_math.h > > > > > > > > > > > > > > There is an overflow in windows as well, but those windows system > > > > functions > > > > > > just > > > > > > > succeed with what ever value there is. > > > > > > > > > > > > > > Mac behaves very strange. It just doesn't have overflows. Any > > > suggestions? > > > > > > > > > > > > I'll play with your patch set on windows and see what's going on, but > > may > > > > not > > > > > > have time to do so in the next day or two. > > > > > > > > > > The issue on windows is that Time::FromeExploded uses static_cast<WORD> > on > > > the > > > > > input data (A word is an unsigned 16 bit int). So for windows, need to > > make > > > > all > > > > > those casts safe. > > > > > > > > > > Looks like you got the tests passing on Mac? > > > > > > > > Internal Mac's methods seem to handle overflows. But there is one place > > where > > > > overflow can occur still (its when base::Time is created). > > > > > > > > About the Windows platform. So, maximum year it can take is 30287. How > about > > > > calculating microseconds from the remainder years? > > > > For example, if the date is 1-2-30999. > > > > reminder = 30999-30287. > > > > ... do stuff with windows functions using other dates + year 30287 > > > > ... > > > > calculate microsecs of the remainder counting leap years as well and then > > sum > > > > up. > > > > > > > > What do you think? > > > > > > I suggested that on https://crbug.com/653445 because it's a real problem > we're > > > currently running into. I don't think it's worth the extra complexity here, > > > since it's only an approximation and the current code already handles all > the > > > way up until year 30287. > > > > Also, words are actually unsigned 16-bit ints, which should mean up to year > > 65535. > > Systemtime limits year from 1601 to 30287 But if some really large year is used, overflow occurs. That is why tests on windows fail
On 2016/10/12 15:22:50, maksims_ wrote: > On 2016/10/12 15:21:30, maksims_ wrote: > > On 2016/10/12 14:46:15, mmenke wrote: > > > On 2016/10/12 14:44:41, mmenke wrote: > > > > On 2016/10/12 13:39:06, maksims wrote: > > > > > On 2016/10/10 22:05:03, mmenke wrote: > > > > > > On 2016/10/10 14:14:17, mmenke wrote: > > > > > > > On 2016/10/10 10:51:22, maksims wrote: > > > > > > > > On 2016/10/07 16:09:28, mmenke wrote: > > > > > > > > > On 2016/10/07 15:12:51, maksims_ wrote: > > > > > > > > > > On 2016/10/07 13:28:55, maksims wrote: > > > > > > > > > > > please take a look. > > > > > > > > > > > > > > > > > > > > > > mmenke@, basically that wasn't an issue from my side. I was > > just > > > > > > > selected > > > > > > > > > > > because the last who touched these files was me. > > > > > > > > > > > > > > > > > > > > Please, don't review yet. > > > > > > > > > > > > > > > > > > Rather than rolling your own solution, see safe_math.h > > > > > > > > > > > > > > > > There is an overflow in windows as well, but those windows system > > > > > functions > > > > > > > just > > > > > > > > succeed with what ever value there is. > > > > > > > > > > > > > > > > Mac behaves very strange. It just doesn't have overflows. Any > > > > suggestions? > > > > > > > > > > > > > > I'll play with your patch set on windows and see what's going on, > but > > > may > > > > > not > > > > > > > have time to do so in the next day or two. > > > > > > > > > > > > The issue on windows is that Time::FromeExploded uses > static_cast<WORD> > > on > > > > the > > > > > > input data (A word is an unsigned 16 bit int). So for windows, need > to > > > make > > > > > all > > > > > > those casts safe. > > > > > > > > > > > > Looks like you got the tests passing on Mac? > > > > > > > > > > Internal Mac's methods seem to handle overflows. But there is one place > > > where > > > > > overflow can occur still (its when base::Time is created). > > > > > > > > > > About the Windows platform. So, maximum year it can take is 30287. How > > about > > > > > calculating microseconds from the remainder years? > > > > > For example, if the date is 1-2-30999. > > > > > reminder = 30999-30287. > > > > > ... do stuff with windows functions using other dates + year 30287 > > > > > ... > > > > > calculate microsecs of the remainder counting leap years as well and > then > > > sum > > > > > up. > > > > > > > > > > What do you think? > > > > > > > > I suggested that on https://crbug.com/653445 because it's a real problem > > we're > > > > currently running into. I don't think it's worth the extra complexity > here, > > > > since it's only an approximation and the current code already handles all > > the > > > > way up until year 30287. > > > > > > Also, words are actually unsigned 16-bit ints, which should mean up to year > > > 65535. > > > > Systemtime limits year from 1601 to 30287 > > But if some really large year is used, overflow occurs. That is why tests on > windows fail The test fail because the overflow isn't detected. If we detected overflow, and returned false, the test would pass (That's what the test is checking for - if you added support for returning the actual base::Time for year 9840633 (Which is too large for a base::Time, anyways, I believe), the test would actually fail.
On 2016/10/12 15:27:35, mmenke wrote: > On 2016/10/12 15:22:50, maksims_ wrote: > > On 2016/10/12 15:21:30, maksims_ wrote: > > > On 2016/10/12 14:46:15, mmenke wrote: > > > > On 2016/10/12 14:44:41, mmenke wrote: > > > > > On 2016/10/12 13:39:06, maksims wrote: > > > > > > On 2016/10/10 22:05:03, mmenke wrote: > > > > > > > On 2016/10/10 14:14:17, mmenke wrote: > > > > > > > > On 2016/10/10 10:51:22, maksims wrote: > > > > > > > > > On 2016/10/07 16:09:28, mmenke wrote: > > > > > > > > > > On 2016/10/07 15:12:51, maksims_ wrote: > > > > > > > > > > > On 2016/10/07 13:28:55, maksims wrote: > > > > > > > > > > > > please take a look. > > > > > > > > > > > > > > > > > > > > > > > > mmenke@, basically that wasn't an issue from my side. I > was > > > just > > > > > > > > selected > > > > > > > > > > > > because the last who touched these files was me. > > > > > > > > > > > > > > > > > > > > > > Please, don't review yet. > > > > > > > > > > > > > > > > > > > > Rather than rolling your own solution, see safe_math.h > > > > > > > > > > > > > > > > > > There is an overflow in windows as well, but those windows > system > > > > > > functions > > > > > > > > just > > > > > > > > > succeed with what ever value there is. > > > > > > > > > > > > > > > > > > Mac behaves very strange. It just doesn't have overflows. Any > > > > > suggestions? > > > > > > > > > > > > > > > > I'll play with your patch set on windows and see what's going on, > > but > > > > may > > > > > > not > > > > > > > > have time to do so in the next day or two. > > > > > > > > > > > > > > The issue on windows is that Time::FromeExploded uses > > static_cast<WORD> > > > on > > > > > the > > > > > > > input data (A word is an unsigned 16 bit int). So for windows, need > > to > > > > make > > > > > > all > > > > > > > those casts safe. > > > > > > > > > > > > > > Looks like you got the tests passing on Mac? > > > > > > > > > > > > Internal Mac's methods seem to handle overflows. But there is one > place > > > > where > > > > > > overflow can occur still (its when base::Time is created). > > > > > > > > > > > > About the Windows platform. So, maximum year it can take is 30287. How > > > about > > > > > > calculating microseconds from the remainder years? > > > > > > For example, if the date is 1-2-30999. > > > > > > reminder = 30999-30287. > > > > > > ... do stuff with windows functions using other dates + year 30287 > > > > > > ... > > > > > > calculate microsecs of the remainder counting leap years as well and > > then > > > > sum > > > > > > up. > > > > > > > > > > > > What do you think? > > > > > > > > > > I suggested that on https://crbug.com/653445 because it's a real problem > > > we're > > > > > currently running into. I don't think it's worth the extra complexity > > here, > > > > > since it's only an approximation and the current code already handles > all > > > the > > > > > way up until year 30287. > > > > > > > > Also, words are actually unsigned 16-bit ints, which should mean up to > year > > > > 65535. > > > > > > Systemtime limits year from 1601 to 30287 > > > > But if some really large year is used, overflow occurs. That is why tests on > > windows fail > > The test fail because the overflow isn't detected. If we detected overflow, and > returned false, the test would pass (That's what the test is checking for - if > you added support for returning the actual base::Time for year 9840633 (Which is > too large for a base::Time, anyways, I believe), the test would actually fail. Yes, that is what I am trying to say. So, there are two alternatives: limit year to 30287 or handle that manually limiting the time by int64 as it is done in linux
On 2016/10/12 15:39:24, maksims_ wrote: > On 2016/10/12 15:27:35, mmenke wrote: > > On 2016/10/12 15:22:50, maksims_ wrote: > > > On 2016/10/12 15:21:30, maksims_ wrote: > > > > On 2016/10/12 14:46:15, mmenke wrote: > > > > > On 2016/10/12 14:44:41, mmenke wrote: > > > > > > On 2016/10/12 13:39:06, maksims wrote: > > > > > > > On 2016/10/10 22:05:03, mmenke wrote: > > > > > > > > On 2016/10/10 14:14:17, mmenke wrote: > > > > > > > > > On 2016/10/10 10:51:22, maksims wrote: > > > > > > > > > > On 2016/10/07 16:09:28, mmenke wrote: > > > > > > > > > > > On 2016/10/07 15:12:51, maksims_ wrote: > > > > > > > > > > > > On 2016/10/07 13:28:55, maksims wrote: > > > > > > > > > > > > > please take a look. > > > > > > > > > > > > > > > > > > > > > > > > > > mmenke@, basically that wasn't an issue from my side. I > > was > > > > just > > > > > > > > > selected > > > > > > > > > > > > > because the last who touched these files was me. > > > > > > > > > > > > > > > > > > > > > > > > Please, don't review yet. > > > > > > > > > > > > > > > > > > > > > > Rather than rolling your own solution, see safe_math.h > > > > > > > > > > > > > > > > > > > > There is an overflow in windows as well, but those windows > > system > > > > > > > functions > > > > > > > > > just > > > > > > > > > > succeed with what ever value there is. > > > > > > > > > > > > > > > > > > > > Mac behaves very strange. It just doesn't have overflows. Any > > > > > > suggestions? > > > > > > > > > > > > > > > > > > I'll play with your patch set on windows and see what's going > on, > > > but > > > > > may > > > > > > > not > > > > > > > > > have time to do so in the next day or two. > > > > > > > > > > > > > > > > The issue on windows is that Time::FromeExploded uses > > > static_cast<WORD> > > > > on > > > > > > the > > > > > > > > input data (A word is an unsigned 16 bit int). So for windows, > need > > > to > > > > > make > > > > > > > all > > > > > > > > those casts safe. > > > > > > > > > > > > > > > > Looks like you got the tests passing on Mac? > > > > > > > > > > > > > > Internal Mac's methods seem to handle overflows. But there is one > > place > > > > > where > > > > > > > overflow can occur still (its when base::Time is created). > > > > > > > > > > > > > > About the Windows platform. So, maximum year it can take is 30287. > How > > > > about > > > > > > > calculating microseconds from the remainder years? > > > > > > > For example, if the date is 1-2-30999. > > > > > > > reminder = 30999-30287. > > > > > > > ... do stuff with windows functions using other dates + year 30287 > > > > > > > ... > > > > > > > calculate microsecs of the remainder counting leap years as well and > > > then > > > > > sum > > > > > > > up. > > > > > > > > > > > > > > What do you think? > > > > > > > > > > > > I suggested that on https://crbug.com/653445 because it's a real > problem > > > > we're > > > > > > currently running into. I don't think it's worth the extra complexity > > > here, > > > > > > since it's only an approximation and the current code already handles > > all > > > > the > > > > > > way up until year 30287. > > > > > > > > > > Also, words are actually unsigned 16-bit ints, which should mean up to > > year > > > > > 65535. > > > > > > > > Systemtime limits year from 1601 to 30287 > > > > > > But if some really large year is used, overflow occurs. That is why tests on > > > windows fail > > > > The test fail because the overflow isn't detected. If we detected overflow, > and > > returned false, the test would pass (That's what the test is checking for - if > > you added support for returning the actual base::Time for year 9840633 (Which > is > > too large for a base::Time, anyways, I believe), the test would actually fail. > > Yes, that is what I am trying to say. So, there are two alternatives: limit year > to 30287 or handle that manually limiting the time by int64 as it is done in > linux I don't think we actually need to limit year to 30287, we just need to detect if any of the WORD casts overflow. If the year is 30288, the cast won't overflop, but SystemTimeToFileTime will fail, so we're still fine.
On 2016/10/12 15:43:04, mmenke wrote: > On 2016/10/12 15:39:24, maksims_ wrote: > > On 2016/10/12 15:27:35, mmenke wrote: > > > On 2016/10/12 15:22:50, maksims_ wrote: > > > > On 2016/10/12 15:21:30, maksims_ wrote: > > > > > On 2016/10/12 14:46:15, mmenke wrote: > > > > > > On 2016/10/12 14:44:41, mmenke wrote: > > > > > > > On 2016/10/12 13:39:06, maksims wrote: > > > > > > > > On 2016/10/10 22:05:03, mmenke wrote: > > > > > > > > > On 2016/10/10 14:14:17, mmenke wrote: > > > > > > > > > > On 2016/10/10 10:51:22, maksims wrote: > > > > > > > > > > > On 2016/10/07 16:09:28, mmenke wrote: > > > > > > > > > > > > On 2016/10/07 15:12:51, maksims_ wrote: > > > > > > > > > > > > > On 2016/10/07 13:28:55, maksims wrote: > > > > > > > > > > > > > > please take a look. > > > > > > > > > > > > > > > > > > > > > > > > > > > > mmenke@, basically that wasn't an issue from my side. > I > > > was > > > > > just > > > > > > > > > > selected > > > > > > > > > > > > > > because the last who touched these files was me. > > > > > > > > > > > > > > > > > > > > > > > > > > Please, don't review yet. > > > > > > > > > > > > > > > > > > > > > > > > Rather than rolling your own solution, see safe_math.h > > > > > > > > > > > > > > > > > > > > > > There is an overflow in windows as well, but those windows > > > system > > > > > > > > functions > > > > > > > > > > just > > > > > > > > > > > succeed with what ever value there is. > > > > > > > > > > > > > > > > > > > > > > Mac behaves very strange. It just doesn't have overflows. > Any > > > > > > > suggestions? > > > > > > > > > > > > > > > > > > > > I'll play with your patch set on windows and see what's going > > on, > > > > but > > > > > > may > > > > > > > > not > > > > > > > > > > have time to do so in the next day or two. > > > > > > > > > > > > > > > > > > The issue on windows is that Time::FromeExploded uses > > > > static_cast<WORD> > > > > > on > > > > > > > the > > > > > > > > > input data (A word is an unsigned 16 bit int). So for windows, > > need > > > > to > > > > > > make > > > > > > > > all > > > > > > > > > those casts safe. > > > > > > > > > > > > > > > > > > Looks like you got the tests passing on Mac? > > > > > > > > > > > > > > > > Internal Mac's methods seem to handle overflows. But there is one > > > place > > > > > > where > > > > > > > > overflow can occur still (its when base::Time is created). > > > > > > > > > > > > > > > > About the Windows platform. So, maximum year it can take is 30287. > > How > > > > > about > > > > > > > > calculating microseconds from the remainder years? > > > > > > > > For example, if the date is 1-2-30999. > > > > > > > > reminder = 30999-30287. > > > > > > > > ... do stuff with windows functions using other dates + year 30287 > > > > > > > > ... > > > > > > > > calculate microsecs of the remainder counting leap years as well > and > > > > then > > > > > > sum > > > > > > > > up. > > > > > > > > > > > > > > > > What do you think? > > > > > > > > > > > > > > I suggested that on https://crbug.com/653445 because it's a real > > problem > > > > > we're > > > > > > > currently running into. I don't think it's worth the extra > complexity > > > > here, > > > > > > > since it's only an approximation and the current code already > handles > > > all > > > > > the > > > > > > > way up until year 30287. > > > > > > > > > > > > Also, words are actually unsigned 16-bit ints, which should mean up to > > > year > > > > > > 65535. > > > > > > > > > > Systemtime limits year from 1601 to 30287 > > > > > > > > But if some really large year is used, overflow occurs. That is why tests > on > > > > windows fail > > > > > > The test fail because the overflow isn't detected. If we detected overflow, > > and > > > returned false, the test would pass (That's what the test is checking for - > if > > > you added support for returning the actual base::Time for year 9840633 > (Which > > is > > > too large for a base::Time, anyways, I believe), the test would actually > fail. > > > > Yes, that is what I am trying to say. So, there are two alternatives: limit > year > > to 30287 or handle that manually limiting the time by int64 as it is done in > > linux > > I don't think we actually need to limit year to 30287, we just need to detect if > any of the WORD casts overflow. If the year is 30288, the cast won't overflop, > but SystemTimeToFileTime will fail, so we're still fine. Overflow, rather...Though I do rather prefer the term overflop, now that I think about it. :)
On 2016/10/12 15:45:02, mmenke wrote: > On 2016/10/12 15:43:04, mmenke wrote: > > On 2016/10/12 15:39:24, maksims_ wrote: > > > On 2016/10/12 15:27:35, mmenke wrote: > > > > On 2016/10/12 15:22:50, maksims_ wrote: > > > > > On 2016/10/12 15:21:30, maksims_ wrote: > > > > > > On 2016/10/12 14:46:15, mmenke wrote: > > > > > > > On 2016/10/12 14:44:41, mmenke wrote: > > > > > > > > On 2016/10/12 13:39:06, maksims wrote: > > > > > > > > > On 2016/10/10 22:05:03, mmenke wrote: > > > > > > > > > > On 2016/10/10 14:14:17, mmenke wrote: > > > > > > > > > > > On 2016/10/10 10:51:22, maksims wrote: > > > > > > > > > > > > On 2016/10/07 16:09:28, mmenke wrote: > > > > > > > > > > > > > On 2016/10/07 15:12:51, maksims_ wrote: > > > > > > > > > > > > > > On 2016/10/07 13:28:55, maksims wrote: > > > > > > > > > > > > > > > please take a look. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > mmenke@, basically that wasn't an issue from my > side. > > I > > > > was > > > > > > just > > > > > > > > > > > selected > > > > > > > > > > > > > > > because the last who touched these files was me. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Please, don't review yet. > > > > > > > > > > > > > > > > > > > > > > > > > > Rather than rolling your own solution, see safe_math.h > > > > > > > > > > > > > > > > > > > > > > > > There is an overflow in windows as well, but those windows > > > > system > > > > > > > > > functions > > > > > > > > > > > just > > > > > > > > > > > > succeed with what ever value there is. > > > > > > > > > > > > > > > > > > > > > > > > Mac behaves very strange. It just doesn't have overflows. > > Any > > > > > > > > suggestions? > > > > > > > > > > > > > > > > > > > > > > I'll play with your patch set on windows and see what's > going > > > on, > > > > > but > > > > > > > may > > > > > > > > > not > > > > > > > > > > > have time to do so in the next day or two. > > > > > > > > > > > > > > > > > > > > The issue on windows is that Time::FromeExploded uses > > > > > static_cast<WORD> > > > > > > on > > > > > > > > the > > > > > > > > > > input data (A word is an unsigned 16 bit int). So for > windows, > > > need > > > > > to > > > > > > > make > > > > > > > > > all > > > > > > > > > > those casts safe. > > > > > > > > > > > > > > > > > > > > Looks like you got the tests passing on Mac? > > > > > > > > > > > > > > > > > > Internal Mac's methods seem to handle overflows. But there is > one > > > > place > > > > > > > where > > > > > > > > > overflow can occur still (its when base::Time is created). > > > > > > > > > > > > > > > > > > About the Windows platform. So, maximum year it can take is > 30287. > > > How > > > > > > about > > > > > > > > > calculating microseconds from the remainder years? > > > > > > > > > For example, if the date is 1-2-30999. > > > > > > > > > reminder = 30999-30287. > > > > > > > > > ... do stuff with windows functions using other dates + year > 30287 > > > > > > > > > ... > > > > > > > > > calculate microsecs of the remainder counting leap years as well > > and > > > > > then > > > > > > > sum > > > > > > > > > up. > > > > > > > > > > > > > > > > > > What do you think? > > > > > > > > > > > > > > > > I suggested that on https://crbug.com/653445 because it's a real > > > problem > > > > > > we're > > > > > > > > currently running into. I don't think it's worth the extra > > complexity > > > > > here, > > > > > > > > since it's only an approximation and the current code already > > handles > > > > all > > > > > > the > > > > > > > > way up until year 30287. > > > > > > > > > > > > > > Also, words are actually unsigned 16-bit ints, which should mean up > to > > > > year > > > > > > > 65535. > > > > > > > > > > > > Systemtime limits year from 1601 to 30287 > > > > > > > > > > But if some really large year is used, overflow occurs. That is why > tests > > on > > > > > windows fail > > > > > > > > The test fail because the overflow isn't detected. If we detected > overflow, > > > and > > > > returned false, the test would pass (That's what the test is checking for > - > > if > > > > you added support for returning the actual base::Time for year 9840633 > > (Which > > > is > > > > too large for a base::Time, anyways, I believe), the test would actually > > fail. > > > > > > Yes, that is what I am trying to say. So, there are two alternatives: limit > > year > > > to 30287 or handle that manually limiting the time by int64 as it is done in > > > linux > > > > I don't think we actually need to limit year to 30287, we just need to detect > if > > any of the WORD casts overflow. If the year is 30288, the cast won't > overflop, > > but SystemTimeToFileTime will fail, so we're still fine. > > Overflow, rather...Though I do rather prefer the term overflop, now that I think > about it. :) If there might be an overflow, then we fail as well, right?
On 2016/10/12 15:51:50, maksims_ wrote: > On 2016/10/12 15:45:02, mmenke wrote: > > On 2016/10/12 15:43:04, mmenke wrote: > > > On 2016/10/12 15:39:24, maksims_ wrote: > > > > On 2016/10/12 15:27:35, mmenke wrote: > > > > > On 2016/10/12 15:22:50, maksims_ wrote: > > > > > > On 2016/10/12 15:21:30, maksims_ wrote: > > > > > > > On 2016/10/12 14:46:15, mmenke wrote: > > > > > > > > On 2016/10/12 14:44:41, mmenke wrote: > > > > > > > > > On 2016/10/12 13:39:06, maksims wrote: > > > > > > > > > > On 2016/10/10 22:05:03, mmenke wrote: > > > > > > > > > > > On 2016/10/10 14:14:17, mmenke wrote: > > > > > > > > > > > > On 2016/10/10 10:51:22, maksims wrote: > > > > > > > > > > > > > On 2016/10/07 16:09:28, mmenke wrote: > > > > > > > > > > > > > > On 2016/10/07 15:12:51, maksims_ wrote: > > > > > > > > > > > > > > > On 2016/10/07 13:28:55, maksims wrote: > > > > > > > > > > > > > > > > please take a look. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > mmenke@, basically that wasn't an issue from my > > side. > > > I > > > > > was > > > > > > > just > > > > > > > > > > > > selected > > > > > > > > > > > > > > > > because the last who touched these files was me. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Please, don't review yet. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Rather than rolling your own solution, see safe_math.h > > > > > > > > > > > > > > > > > > > > > > > > > > There is an overflow in windows as well, but those > windows > > > > > system > > > > > > > > > > functions > > > > > > > > > > > > just > > > > > > > > > > > > > succeed with what ever value there is. > > > > > > > > > > > > > > > > > > > > > > > > > > Mac behaves very strange. It just doesn't have > overflows. > > > Any > > > > > > > > > suggestions? > > > > > > > > > > > > > > > > > > > > > > > > I'll play with your patch set on windows and see what's > > going > > > > on, > > > > > > but > > > > > > > > may > > > > > > > > > > not > > > > > > > > > > > > have time to do so in the next day or two. > > > > > > > > > > > > > > > > > > > > > > The issue on windows is that Time::FromeExploded uses > > > > > > static_cast<WORD> > > > > > > > on > > > > > > > > > the > > > > > > > > > > > input data (A word is an unsigned 16 bit int). So for > > windows, > > > > need > > > > > > to > > > > > > > > make > > > > > > > > > > all > > > > > > > > > > > those casts safe. > > > > > > > > > > > > > > > > > > > > > > Looks like you got the tests passing on Mac? > > > > > > > > > > > > > > > > > > > > Internal Mac's methods seem to handle overflows. But there is > > one > > > > > place > > > > > > > > where > > > > > > > > > > overflow can occur still (its when base::Time is created). > > > > > > > > > > > > > > > > > > > > About the Windows platform. So, maximum year it can take is > > 30287. > > > > How > > > > > > > about > > > > > > > > > > calculating microseconds from the remainder years? > > > > > > > > > > For example, if the date is 1-2-30999. > > > > > > > > > > reminder = 30999-30287. > > > > > > > > > > ... do stuff with windows functions using other dates + year > > 30287 > > > > > > > > > > ... > > > > > > > > > > calculate microsecs of the remainder counting leap years as > well > > > and > > > > > > then > > > > > > > > sum > > > > > > > > > > up. > > > > > > > > > > > > > > > > > > > > What do you think? > > > > > > > > > > > > > > > > > > I suggested that on https://crbug.com/653445 because it's a real > > > > problem > > > > > > > we're > > > > > > > > > currently running into. I don't think it's worth the extra > > > complexity > > > > > > here, > > > > > > > > > since it's only an approximation and the current code already > > > handles > > > > > all > > > > > > > the > > > > > > > > > way up until year 30287. > > > > > > > > > > > > > > > > Also, words are actually unsigned 16-bit ints, which should mean > up > > to > > > > > year > > > > > > > > 65535. > > > > > > > > > > > > > > Systemtime limits year from 1601 to 30287 > > > > > > > > > > > > But if some really large year is used, overflow occurs. That is why > > tests > > > on > > > > > > windows fail > > > > > > > > > > The test fail because the overflow isn't detected. If we detected > > overflow, > > > > and > > > > > returned false, the test would pass (That's what the test is checking > for > > - > > > if > > > > > you added support for returning the actual base::Time for year 9840633 > > > (Which > > > > is > > > > > too large for a base::Time, anyways, I believe), the test would actually > > > fail. > > > > > > > > Yes, that is what I am trying to say. So, there are two alternatives: > limit > > > year > > > > to 30287 or handle that manually limiting the time by int64 as it is done > in > > > > linux > > > > > > I don't think we actually need to limit year to 30287, we just need to > detect > > if > > > any of the WORD casts overflow. If the year is 30288, the cast won't > > overflop, > > > but SystemTimeToFileTime will fail, so we're still fine. > > > > Overflow, rather...Though I do rather prefer the term overflop, now that I > think > > about it. :) > If there might be an overflow, then we fail as well, right? Right, if we can't convert one of the values to a WORD withotu an overflow, we should return false and set the time to base::Time().
The CQ bit was checked by maksim.sisov@intel.com 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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by maksim.sisov@intel.com 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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by maksim.sisov@intel.com 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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by maksim.sisov@intel.com 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...
Patchset #10 (id:240001) has been deleted
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Patchset #9 (id:220001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #8 (id:200001) has been deleted
Patchset #7 (id:180001) has been deleted
Patchset #5 (id:140001) has been deleted
Patchset #4 (id:120001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
The CQ bit was checked by maksim.sisov@intel.com 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...
please review
Description was changed from ========== 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: use safe_math.h for multiplication and addition. If overflow occurs, return possibly maximum platform dependent value. BUG=653445 ========== to ========== 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). BUG=653445 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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#... base/time/time_win.cc:113: const int kMaxWinWordSize = 65535; This should just be std::numeric_limits<WORD>::max() https://codereview.chromium.org/2405453002/diff/280001/base/time/time_win.cc#... base/time/time_win.cc:260: st.wMilliseconds = static_cast<WORD>(exploded.millisecond); Seems like this should be: if (!SafeConvertToWord(exploded.year, &st.wYear) || !SafeConvertToWord(exploded.month, &st.wMonth) || ....) { *time = Time(0); return false; } And then something like: bool SafeConvertToWord(int in, WORD* out) { CheckedNumeric<WORD> result = in; *out = result.ValueOrDefault(std::numeric_limits<WORD>::max()); return result.IsValid(); } In an anonymous namespace up top. The advantage is it covers all parameters, which is good, since we want to reject invalid times, and is more general - it doesn't depend on WORD being an unsigned int16. If that doesn't work for some reason, there's also IsValueInRangeForNumericType.
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#newco... base/time/time.h:548: // month is set to 31 on a 28-30 day month. Returns maximum possible IMO, if there is any overflow, this function should return false instead of just returning "max possible time" because the conversion is not possible. Or, can you point at specific code that requires this behavior? https://codereview.chromium.org/2405453002/diff/280001/base/time/time_mac.cc File base/time/time_mac.cc (right): https://codereview.chromium.org/2405453002/diff/280001/base/time/time_mac.cc#... base/time/time_mac.cc:196: CFAbsoluteTime microseconds = Even though we know CFAbsoluteTime is a double, it's not proper to use it here because: 1) It is a data type whose timebase is in seconds; and 2) There's no guarantee it won't be typedef'ed to something else in the future (though, admittedly this is highly unlikely). So, please just use double here. https://codereview.chromium.org/2405453002/diff/280001/base/time/time_posix.cc File base/time/time_posix.cc (right): https://codereview.chromium.org/2405453002/diff/280001/base/time/time_posix.c... base/time/time_posix.cc:119: int64_t MultiplyAndAdd(int64_t init_value, This helper hides whether overflow occurs (i.e., it will always return zero whether a non-overflowing result is zero or an overflow occurred). I suggest getting rid of this method (see comments below). https://codereview.chromium.org/2405453002/diff/280001/base/time/time_posix.c... base/time/time_posix.cc:257: int64_t milliseconds = 0; nit: Please move the declaration of |milliseconds| down to around where it is first used (approx. line 294). https://codereview.chromium.org/2405453002/diff/280001/base/time/time_posix.c... base/time/time_posix.cc:327: milliseconds = Suggestion (to return false if overflow occurs): base::CheckedNumeric<int64_t> checked_millis = seconds; checked_millis *= kMillisecondsPerSecond; checked_millis += exploded.millisecond; if (!checked_millis.is_valid()) { *time = base::Time(0); return false; } milliseconds = checked_millis.value(); https://codereview.chromium.org/2405453002/diff/280001/base/time/time_posix.c... base/time/time_posix.cc:332: int64_t microseconds_win_epoch = Similar suggestion here: Declare |microseconds_win_epoch| as a base::CheckedNumeric<int64_t> rather than a plain int64_t, and return false if !is_valid() after the math. 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#... base/time/time_win.cc:245: if (exploded.year > kMaxWinWordSize) { Suggestion: if (exploded.year > std::numeric_limits<WORD>::max()) { ...then you don't need the global constant. Or, given mmenke's comment for the code on lines 252-260, maybe the code here just goes away?
The CQ bit was checked by maksim.sisov@intel.com 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_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Patchset #7 (id:300001) has been deleted
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_...)
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.... base/time/time.h:548: // month is set to 31 on a 28-30 day month. Returns maximum possible On 2016/10/18 19:55:13, miu wrote: > IMO, if there is any overflow, this function should return false instead of just > returning "max possible time" because the conversion is not possible. > > Or, can you point at specific code that requires this behavior? That's a typo. At first I thought it would be like this. It returns Time(0), of course. https://chromiumcodereview.appspot.com/2405453002/diff/280001/base/time/time_... File base/time/time_mac.cc (right): https://chromiumcodereview.appspot.com/2405453002/diff/280001/base/time/time_... base/time/time_mac.cc:196: CFAbsoluteTime microseconds = On 2016/10/18 19:55:13, miu wrote: > Even though we know CFAbsoluteTime is a double, it's not proper to use it here > because: 1) It is a data type whose timebase is in seconds; and 2) There's no > guarantee it won't be typedef'ed to something else in the future (though, > admittedly this is highly unlikely). So, please just use double here. Done. https://chromiumcodereview.appspot.com/2405453002/diff/280001/base/time/time_... File base/time/time_posix.cc (right): https://chromiumcodereview.appspot.com/2405453002/diff/280001/base/time/time_... base/time/time_posix.cc:119: int64_t MultiplyAndAdd(int64_t init_value, On 2016/10/18 19:55:13, miu wrote: > This helper hides whether overflow occurs (i.e., it will always return zero > whether a non-overflowing result is zero or an overflow occurred). I suggest > getting rid of this method (see comments below). Removed https://chromiumcodereview.appspot.com/2405453002/diff/280001/base/time/time_... base/time/time_posix.cc:257: int64_t milliseconds = 0; On 2016/10/18 19:55:13, miu wrote: > nit: Please move the declaration of |milliseconds| down to around where it is > first used (approx. line 294). Done. https://chromiumcodereview.appspot.com/2405453002/diff/280001/base/time/time_... base/time/time_posix.cc:327: milliseconds = On 2016/10/18 19:55:13, miu wrote: > Suggestion (to return false if overflow occurs): > > base::CheckedNumeric<int64_t> checked_millis = seconds; > checked_millis *= kMillisecondsPerSecond; > checked_millis += exploded.millisecond; > if (!checked_millis.is_valid()) { > *time = base::Time(0); > return false; > } > milliseconds = checked_millis.value(); Done. https://chromiumcodereview.appspot.com/2405453002/diff/280001/base/time/time_... base/time/time_posix.cc:332: int64_t microseconds_win_epoch = On 2016/10/18 19:55:13, miu wrote: > Similar suggestion here: Declare |microseconds_win_epoch| as a > base::CheckedNumeric<int64_t> rather than a plain int64_t, and return false if > !is_valid() after the math. Done. https://chromiumcodereview.appspot.com/2405453002/diff/280001/base/time/time_... File base/time/time_win.cc (right): https://chromiumcodereview.appspot.com/2405453002/diff/280001/base/time/time_... base/time/time_win.cc:245: if (exploded.year > kMaxWinWordSize) { On 2016/10/18 19:55:13, miu wrote: > Suggestion: > > if (exploded.year > std::numeric_limits<WORD>::max()) { > > ...then you don't need the global constant. > > Or, given mmenke's comment for the code on lines 252-260, maybe the code here > just goes away? Yes. Removed https://chromiumcodereview.appspot.com/2405453002/diff/280001/base/time/time_... base/time/time_win.cc:260: st.wMilliseconds = static_cast<WORD>(exploded.millisecond); On 2016/10/18 15:04:50, mmenke wrote: > Seems like this should be: > > if (!SafeConvertToWord(exploded.year, &st.wYear) || > !SafeConvertToWord(exploded.month, &st.wMonth) || > ....) { > *time = Time(0); > return false; > } > > And then something like: > > bool SafeConvertToWord(int in, WORD* out) { > CheckedNumeric<WORD> result = in; > *out = result.ValueOrDefault(std::numeric_limits<WORD>::max()); > return result.IsValid(); > } > > In an anonymous namespace up top. > > The advantage is it covers all parameters, which is good, since we want to > reject invalid times, and is more general - it doesn't depend on WORD being an > unsigned int16. If that doesn't work for some reason, there's also > IsValueInRangeForNumericType. Done.
LGTM
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.... base/time/time.h:548: // month is set to 31 on a 28-30 day month. Returns Time(0) on overflow. nit: "Returns false and sets |time| to Time(0) on overflow."
The CQ bit was checked by maksim.sisov@intel.com 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...
Patchset #9 (id:360001) has been deleted
Patchset #8 (id:340001) has been deleted
maksim.sisov@intel.com changed reviewers: + blundell@chromium.org, liberato@chromium.org
liberato@chromium.org: Please review changes in components blundell@chromium.org: Please review changes in media
Description was changed from ========== 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). BUG=653445 ========== to ========== 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 ==========
The CQ bit was checked by maksim.sisov@intel.com 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_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2016/10/20 12:00:54, maksims wrote: > mailto:liberato@chromium.org: Please review changes in > components > > mailto:blundell@chromium.org: Please review changes in > media seems like there are quite many places in media, where Exploded's member day_of_week is not used. I need to fix my windows build and build media_unittests separately.
media/ lgtm. out of curiosity, which platform cares about the day of the week that was causing undefined behavior?
media/ lgtm. out of curiosity, which platform cares about the day of the week that was causing undefined behavior?
On 2016/10/20 15:38:51, liberato wrote: > media/ lgtm. > > out of curiosity, which platform cares about the day of the week that was > causing undefined behavior? Windows. I assume the value isn't being initialized. The code added in this CL safely converts the day to a WORD, which is the datatype the corresponding Windows function takes. More generally, passing an uninitialized value to a platform API just seems like a bad idea, even if it's most likely going to be ignored.
On 2016/10/20 15:42:33, mmenke wrote: > On 2016/10/20 15:38:51, liberato wrote: > > media/ lgtm. > > > > out of curiosity, which platform cares about the day of the week that was > > causing undefined behavior? > > Windows. I assume the value isn't being initialized. The code added in this CL > safely converts the day to a WORD, which is the datatype the corresponding > Windows function takes. More generally, passing an uninitialized value to a > platform API just seems like a bad idea, even if it's most likely going to be > ignored. That having been said, the fix may just be not to worry about overflow for that one value, and just use a static_cast like the old code did. Not happy with that solution, but not sure something better is worth the effort.
On 2016/10/20 15:43:47, mmenke wrote: > On 2016/10/20 15:42:33, mmenke wrote: > > On 2016/10/20 15:38:51, liberato wrote: > > > media/ lgtm. > > > > > > out of curiosity, which platform cares about the day of the week that was > > > causing undefined behavior? > > > > Windows. I assume the value isn't being initialized. The code added in this > CL > > safely converts the day to a WORD, which is the datatype the corresponding > > Windows function takes. More generally, passing an uninitialized value to a > > platform API just seems like a bad idea, even if it's most likely going to be > > ignored. > > That having been said, the fix may just be not to worry about overflow for that > one value, and just use a static_cast like the old code did. Not happy with > that solution, but not sure something better is worth the effort. it's odd that windows uses the value for anything. as in, if one fills in the DoW differently, does the result of whatever windows calls mktime() change? or does it only get mad if it's out of bounds? setting day of week on a mktime-like call isn't intuitive, and i wouldn't be surprised if folks don't do it properly everywhere.
The CQ bit was checked by maksim.sisov@intel.com 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_...)
//components lgtm
The CQ bit was checked by maksim.sisov@intel.com 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...
Patchset #11 (id:440001) has been deleted
The CQ bit was checked by maksim.sisov@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org, mmenke@chromium.org, liberato@chromium.org, blundell@chromium.org Link to the patchset: https://codereview.chromium.org/2405453002/#ps460001 (title: "rebased and fixed net unittest")
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.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:460001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/eebbdecabe619cc9887152bd9371f3e2bc7711b6 Cr-Commit-Position: refs/heads/master@{#427064} |