When parsing cookie expiration times, saturate out of range dates

When parsing cookie expiration times, saturate out of range dates
rather than reject them.

Roughly this means cookie expiration times prior to 1970 on (non-OSX)
POSIX, or after 2038 on 32-bit (non-OSX) POSIX, will now be interpreted
as either very small or very large base::Time values.

BUG=649416
Committed: https://crrev.com/93cde27c80e7119c5a076ea61f59cc5bd473d219
Cr-Commit-Position: refs/heads/master@{#426213}

[mmenke, 2016-10-17 17:45:10]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 17:45:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-17 18:14:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-17 18:14:40]

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...)

[mmenke, 2016-10-17 19:37:09]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 19:37:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2016-10-17 19:53:50]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 19:54:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-17 21:52:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-17 21:52:32]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-10-17 22:05:40]

mmenke@chromium.org changed reviewers:
+ eroman@chromium.org

[mmenke, 2016-10-17 22:05:41]

Eric:  WDYT?  Saturating to base::Time::Max() seems wrong, since
base::Time::Max() is not the value which FromExploded starts returning false.

Another option would be to reduce the date to the last year where the exploded
time doesn't overflow (Explode?), and then just add the approximate time from
then until the provided year (base::TimeDelta::FromDays(365*delta)).  Would need
to replace Feb 29th with Feb 28th, but otherwise, I think that would work well
enough...  Would also provide better date validation.

[mmenke, 2016-10-18 16:37:00]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 16:37:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 18:48:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-18 18:48:07]

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_...)

[eroman, 2016-10-18 21:52:55]

https://codereview.chromium.org/2424443002/diff/60001/net/cookies/cookie_util.cc
File net/cookies/cookie_util.cc (right):

https://codereview.chromium.org/2424443002/diff/60001/net/cookies/cookie_util...
net/cookies/cookie_util.cc:57: #if defined(OS_POSIX) && !defined(OS_MACOSX)
What is the range on mac?

https://codereview.chromium.org/2424443002/diff/60001/net/cookies/cookie_util...
net/cookies/cookie_util.cc:75: if (exploded.year < 1961) {
Not sure I follow, isn't the windows epoch 1601?

https://codereview.chromium.org/2424443002/diff/60001/net/cookies/cookie_util...
net/cookies/cookie_util.cc:85: // This relies on base::Time() returning the
start of the Windows epoch.
You can use base::Time::FromInternalValue() -- that is defined in terms of the
windows epoch.

https://codereview.chromium.org/2424443002/diff/60001/net/cookies/cookie_util...
File net/cookies/cookie_util_unittest.cc (right):

https://codereview.chromium.org/2424443002/diff/60001/net/cookies/cookie_util...
net/cookies/cookie_util_unittest.cc:178: // anyway (and return a minimal
base::Time).
wrap

[eroman, 2016-10-18 22:00:36]

On Mon, Oct 17, 2016 at 3:05 PM, <mmenke@chromium.org> wrote:

> Reviewers: eroman
> CL: https://codereview.chromium.org/2424443002/
>
> Message:
> Eric: WDYT? Saturating to base::Time::Max() seems wrong, since
> base::Time::Max() is not the value which FromExploded starts returning
> false.
>

There is precedence for use base::Time::Max()... but I am fine with either
approach.


> Another option would be to reduce the date to the last year where the
> exploded
> time doesn't overflow (Explode?), and then just add the approximate time
> from
> then until the provided year (base::TimeDelta::FromDays(365*delta)).
> Would need
> to replace Feb 29th with Feb 28th, but otherwise, I think that would work
> well
> enough... Would also provide better date validation.
>

That would be fine, but honestly I don't think it is necessary.

My main motivation for wanting to keep this specific to cookie parsing (and
not a generic function for date parsing), is that the overall behavior
shouldn't matter how accurate the estimate is.

Either we use a reasonable specific date, or parsing fails, or we have some
date that is far far into the future (and will in practice never be hit).

I need to dig, but do we have a bound on the max age for a cache entry? (in
other words, is there any practical difference for setting expiration to
the year 2 zillion, vs 2038).

(If we were to expose this elsewhere then trying to be more accurate seems
reasonable, but it also could be a concern depending on the accuracy that
consumers expect)


>
> Description:
> When parsing cookie expiration times, saturate out of range dates
> rather than reject them.
>
> Roughly this means cookie expiration times prior to 1970 on (non-OSX)
> POSIX, or after 2038 on 32-bit (non-OSX) POSIX, will now be interpreted
> as either very small or very large base::Time values.
>
> BUG=649416
>
> Affected files (+220, -85 lines):
> M extensions/browser/api/web_request/web_request_api_helpers.cc
> M net/cookies/canonical_cookie.h
> M net/cookies/canonical_cookie.cc
> M net/cookies/cookie_monster_store_test.cc
> M net/cookies/cookie_util.h
> M net/cookies/cookie_util.cc
> M net/cookies/cookie_util_unittest.cc
>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mmenke, 2016-10-18 22:18:14]

Thanks for the feedback!

On 2016/10/18 22:00:36, eroman wrote:
> On Mon, Oct 17, 2016 at 3:05 PM, <mailto:mmenke@chromium.org> wrote:
> 
> > Reviewers: eroman
> > CL: https://codereview.chromium.org/2424443002/
> >
> > Message:
> > Eric: WDYT? Saturating to base::Time::Max() seems wrong, since
> > base::Time::Max() is not the value which FromExploded starts returning
> > false.
> >
> 
> There is precedence for use base::Time::Max()... but I am fine with either
> approach.

I'd be fine with MinNoNullTime(), but given sync, base::Time::Max() just scares
me, though I suppose no one setting such a time probably minds if we keep the
cookie around eternally, but it just seems not great.

> > Another option would be to reduce the date to the last year where the
> > exploded
> > time doesn't overflow (Explode?), and then just add the approximate time
> > from
> > then until the provided year (base::TimeDelta::FromDays(365*delta)).
> > Would need
> > to replace Feb 29th with Feb 28th, but otherwise, I think that would work
> > well
> > enough... Would also provide better date validation.
> >
> 
> That would be fine, but honestly I don't think it is necessary.
> 
> My main motivation for wanting to keep this specific to cookie parsing (and
> not a generic function for date parsing), is that the overall behavior
> shouldn't matter how accurate the estimate is.
> 
> Either we use a reasonable specific date, or parsing fails, or we have some
> date that is far far into the future (and will in practice never be hit).
> 
> I need to dig, but do we have a bound on the max age for a cache entry? (in
> other words, is there any practical difference for setting expiration to
> the year 2 zillion, vs 2038).
> 
> (If we were to expose this elsewhere then trying to be more accurate seems
> reasonable, but it also could be a concern depending on the accuracy that
> consumers expect)

I'm not too familiar with the cache code, but we evict entries based on when
they were last used, and I don't believe there's any effective cap.  I think
we're completely fine for now, but if people are still using 32-bit Linux in 15
years....  Though once it starts to matter, that way to estimate real expiration
time may not be good enough, anyways...So yea, doesn't seem worth it.

https://codereview.chromium.org/2424443002/diff/60001/net/cookies/cookie_util.cc
File net/cookies/cookie_util.cc (right):

https://codereview.chromium.org/2424443002/diff/60001/net/cookies/cookie_util...
net/cookies/cookie_util.cc:57: #if defined(OS_POSIX) && !defined(OS_MACOSX)
On 2016/10/18 21:52:54, eroman wrote:
> What is the range on mac?

It's unclear.  Note that it's passing the tests without issue.

The value it uses is a double, so in theory it could represent a time that's
quite large.

https://codereview.chromium.org/2424443002/diff/60001/net/cookies/cookie_util...
net/cookies/cookie_util.cc:75: if (exploded.year < 1961) {
On 2016/10/18 21:52:54, eroman wrote:
> Not sure I follow, isn't the windows epoch 1601?

You're right, fixed.  Not sure where the 1961 came from.

https://codereview.chromium.org/2424443002/diff/60001/net/cookies/cookie_util...
net/cookies/cookie_util.cc:85: // This relies on base::Time() returning the
start of the Windows epoch.
On 2016/10/18 21:52:54, eroman wrote:
> You can use base::Time::FromInternalValue() -- that is defined in terms of the
> windows epoch.

Done (I didn't use it because the method doesn't explicitly say what it
does...but then, the same is true for base::Time()).

https://codereview.chromium.org/2424443002/diff/60001/net/cookies/cookie_util...
File net/cookies/cookie_util_unittest.cc (right):

https://codereview.chromium.org/2424443002/diff/60001/net/cookies/cookie_util...
net/cookies/cookie_util_unittest.cc:178: // anyway (and return a minimal
base::Time).
On 2016/10/18 21:52:55, eroman wrote:
> wrap

Done.

[mmenke, 2016-10-18 22:23:55]

Grr....git cl upload blocked uploading a CL on an owners whine.  Uploaded now.

[mmenke, 2016-10-18 22:24:00]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 22:24:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eroman, 2016-10-18 22:55:19]

lgtm with nits

https://codereview.chromium.org/2424443002/diff/80001/net/cookies/cookie_util.cc
File net/cookies/cookie_util.cc (right):

https://codereview.chromium.org/2424443002/diff/80001/net/cookies/cookie_util...
net/cookies/cookie_util.cc:34: //  * Time::Max() as the maximum value
Update this comment.

https://codereview.chromium.org/2424443002/diff/80001/net/cookies/cookie_util.h
File net/cookies/cookie_util.h (right):

https://codereview.chromium.org/2424443002/diff/80001/net/cookies/cookie_util...
net/cookies/cookie_util.h:45: // to the min/max time FromExploded supports
rather than fail.
Could you clarify this a little bit more?

https://codereview.chromium.org/2424443002/diff/80001/net/cookies/cookie_util...
File net/cookies/cookie_util_unittest.cc (right):

https://codereview.chromium.org/2424443002/diff/80001/net/cookies/cookie_util...
net/cookies/cookie_util_unittest.cc:163: // It should either have an exact
value, or should be base::Time::Max().
Update this comment.

https://codereview.chromium.org/2424443002/diff/80001/net/cookies/cookie_util...
net/cookies/cookie_util_unittest.cc:175: // Tests parsing dates that are prior
to (or around) 1970. POSIX systems are
Maybe for precision say (non-Mac) POSIX systemss ?

[commit-bot: I haz the power, 2016-10-19 00:22:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-19 00:22:41]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mmenke, 2016-10-19 14:45:31]

Thanks for the feedback!

https://codereview.chromium.org/2424443002/diff/80001/net/cookies/cookie_util.cc
File net/cookies/cookie_util.cc (right):

https://codereview.chromium.org/2424443002/diff/80001/net/cookies/cookie_util...
net/cookies/cookie_util.cc:34: //  * Time::Max() as the maximum value
On 2016/10/18 22:55:18, eroman wrote:
> Update this comment.

Done.

https://codereview.chromium.org/2424443002/diff/80001/net/cookies/cookie_util.h
File net/cookies/cookie_util.h (right):

https://codereview.chromium.org/2424443002/diff/80001/net/cookies/cookie_util...
net/cookies/cookie_util.h:45: // to the min/max time FromExploded supports
rather than fail.
On 2016/10/18 22:55:19, eroman wrote:
> Could you clarify this a little bit more?

Done.

https://codereview.chromium.org/2424443002/diff/80001/net/cookies/cookie_util...
File net/cookies/cookie_util_unittest.cc (right):

https://codereview.chromium.org/2424443002/diff/80001/net/cookies/cookie_util...
net/cookies/cookie_util_unittest.cc:163: // It should either have an exact
value, or should be base::Time::Max().
On 2016/10/18 22:55:19, eroman wrote:
> Update this comment.

Done.

https://codereview.chromium.org/2424443002/diff/80001/net/cookies/cookie_util...
net/cookies/cookie_util_unittest.cc:175: // Tests parsing dates that are prior
to (or around) 1970. POSIX systems are
On 2016/10/18 22:55:19, eroman wrote:
> Maybe for precision say (non-Mac) POSIX systemss ?

Done.

[mmenke, 2016-10-19 14:47:44]

mmenke@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[mmenke, 2016-10-19 14:47:45]

[+rdevlin.cronin]:  TBRing you for
	extensions/browser/api/web_request/web_request_api_helpers.cc - This CL just
renames a method it calls, and gives a better behavior for out of range times.

[mmenke, 2016-10-19 14:49:33]

Description was changed from

==========
When parsing cookie expiration times, saturate out of range dates
rather than reject them.

Roughly this means cookie expiration times prior to 1970 on (non-OSX)
POSIX, or after 2038 on 32-bit (non-OSX) POSIX, will now be interpreted
as either very small or very large base::Time values.

BUG=649416
==========

to

==========
When parsing cookie expiration times, saturate out of range dates
rather than reject them.

Roughly this means cookie expiration times prior to 1970 on (non-OSX)
POSIX, or after 2038 on 32-bit (non-OSX) POSIX, will now be interpreted
as either very small or very large base::Time values.

BUG=649416
TBR=rdevlin.cronin@chromium.org
==========

[mmenke, 2016-10-19 14:49:46]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2016-10-19 14:49:46]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2424443002/#ps100001
(title: "Response to comments")

[commit-bot: I haz the power, 2016-10-19 14:50:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2016-10-19 14:50:39]

extensions lgtm

[mmenke, 2016-10-19 14:51:35]

Description was changed from

==========
When parsing cookie expiration times, saturate out of range dates
rather than reject them.

Roughly this means cookie expiration times prior to 1970 on (non-OSX)
POSIX, or after 2038 on 32-bit (non-OSX) POSIX, will now be interpreted
as either very small or very large base::Time values.

BUG=649416
TBR=rdevlin.cronin@chromium.org
==========

to

==========
When parsing cookie expiration times, saturate out of range dates
rather than reject them.

Roughly this means cookie expiration times prior to 1970 on (non-OSX)
POSIX, or after 2038 on 32-bit (non-OSX) POSIX, will now be interpreted
as either very small or very large base::Time values.

BUG=649416
==========

[commit-bot: I haz the power, 2016-10-19 16:07:47]

Description was changed from

==========
When parsing cookie expiration times, saturate out of range dates
rather than reject them.

Roughly this means cookie expiration times prior to 1970 on (non-OSX)
POSIX, or after 2038 on 32-bit (non-OSX) POSIX, will now be interpreted
as either very small or very large base::Time values.

BUG=649416
==========

to

==========
When parsing cookie expiration times, saturate out of range dates
rather than reject them.

Roughly this means cookie expiration times prior to 1970 on (non-OSX)
POSIX, or after 2038 on 32-bit (non-OSX) POSIX, will now be interpreted
as either very small or very large base::Time values.

BUG=649416
==========

[commit-bot: I haz the power, 2016-10-19 16:07:48]

Committed patchset #6 (id:100001)

[huangs, 2016-10-19 17:47:52]

The new test CookieUtilTest.ParseCookieExpirationTimeBeyond2038 is failing on
"Linux test dbg" bot:

https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2...

[huangs, 2016-10-19 17:48:58]

Going to revert this; sorry for the trouble!

[huangs, 2016-10-19 17:49:28]

A revert of this CL (patchset #6 id:100001) has been created in
https://chromiumcodereview.appspot.com/2433193002/ by huangs@chromium.org.

The reason for reverting is: The new test

CookieUtilTest.ParseCookieExpirationTimeBeyond2038

is failing on "Linux test dbg" bot..

[commit-bot: I haz the power, 2016-10-21 13:09:05]

Description was changed from

==========
When parsing cookie expiration times, saturate out of range dates
rather than reject them.

Roughly this means cookie expiration times prior to 1970 on (non-OSX)
POSIX, or after 2038 on 32-bit (non-OSX) POSIX, will now be interpreted
as either very small or very large base::Time values.

BUG=649416
==========

to

==========
When parsing cookie expiration times, saturate out of range dates
rather than reject them.

Roughly this means cookie expiration times prior to 1970 on (non-OSX)
POSIX, or after 2038 on 32-bit (non-OSX) POSIX, will now be interpreted
as either very small or very large base::Time values.

BUG=649416

Committed: https://crrev.com/93cde27c80e7119c5a076ea61f59cc5bd473d219
Cr-Commit-Position: refs/heads/master@{#426213}
==========

[commit-bot: I haz the power, 2016-10-21 13:09:06]

Patchset 6 (id:??) landed as
https://crrev.com/93cde27c80e7119c5a076ea61f59cc5bd473d219
Cr-Commit-Position: refs/heads/master@{#426213}