New issue
Advanced search Search tips

Issue 601900 link

Starred by 3 users
Status: Fixed
Owner: ----
Closed: Dec 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Chrome , Mac
Pri: 3
Type: Bug

Blocked on:
issue 601903
issue 601905
issue 601908
issue 641096


Sign in to add a comment

base::Time::FromUTCExploded() and base::Time::FromLocalExplode() hide failures, leading to bugs in callers

Project Member Reported by eroman@chromium.org, Apr 8 2016 
base::Time::FromUTCExploded() and base::Time::FromLocalExplode() can fail.

Callers do not seem to be aware of this, nor is it clear from the API how to test for the failure.

This has led to a number of broken callsites that use a pattern similar to:

----------------
if (!exploded.HasValidValues())
  return false;

time = base::Time::FromUTCExploded(exploded);
return true
----------------


HasValidValues() does not guarantee success of FromUTCExploded(), and this pattern can lead to consuming an ill-defined time.

On failure, the Windows implementation may return Time(0).

Comment 1 by eroman@chromium.org, Apr 8 2016

Blockedon: 601903

Comment 2 by eroman@chromium.org, Apr 8 2016

Blockedon: 601905

Comment 3 by eroman@chromium.org, Apr 8 2016

Blockedon: 601908

Comment 4 by maksim.s...@intel.com, Apr 20 2016 

Well, I checked that are a lot of places in the code calling whether base::Time::FromUTCExploded() or base::Time::FromLocalExplode().

It does not seem like it is reasonable to change the implementation of the functions.

About the blockedons. Can we do it like was suggested here - https://bugs.chromium.org/p/chromium/issues/detail?id=601905#c3 ?

If yes, I can do it.

Comment 5 Deleted

Comment 6 by maksim.s...@intel.com, Apr 21 2016 

Or let me think. It might be better to change the APIs rather than doing a workaround..
Project Member

Comment 7 by bugdroid1@chromium.org, May 28 2016 

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

commit ceba9fb480269695775191d14e98ab23b5918382
Author: maksim.sisov <maksim.sisov@intel.com>
Date: Sat May 28 09:20:13 2016

Add: check exploded time is properly converted.

This cl introduces time checking in posix-like and mac systems.
base::Time::FromUTCExploded() and
base::Time::FromLocalExplode() can fail without returning
a proper error.

This fix does the following:
1) After calculations are done, create UTC or local time.
2) Convert UTC or local time back to exploded
3) Compare original exploded with converted one
4) If times are not equal, then return Time(0) indicating
an error.

Windows implementation already returns Time(0) on error.

BUG= 601900 

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

[modify] https://crrev.com/ceba9fb480269695775191d14e98ab23b5918382/base/time/time.cc
[modify] https://crrev.com/ceba9fb480269695775191d14e98ab23b5918382/base/time/time.h
[modify] https://crrev.com/ceba9fb480269695775191d14e98ab23b5918382/base/time/time_mac.cc
[modify] https://crrev.com/ceba9fb480269695775191d14e98ab23b5918382/base/time/time_posix.cc
[modify] https://crrev.com/ceba9fb480269695775191d14e98ab23b5918382/base/time/time_unittest.cc
[modify] https://crrev.com/ceba9fb480269695775191d14e98ab23b5918382/base/time/time_win.cc
Project Member

Comment 8 by bugdroid1@chromium.org, May 31 2016 

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

commit 8c687a70b52123f35c0504a5b3c5c84a639cebaf
Author: guidou <guidou@chromium.org>
Date: Tue May 31 10:21:27 2016

Revert of Add: check exploded time is properly converted (patchset #7 id:340001 of https://codereview.chromium.org/1988663002/ )

Reason for revert:
Suspect of breking Linux Tests bot. https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%29%281%29%2832%29?numbuilds=200

I will reland if the revert does not fix the bot.

Original issue's description:
> Add: check exploded time is properly converted.
>
> This cl introduces time checking in posix-like and mac systems.
> base::Time::FromUTCExploded() and
> base::Time::FromLocalExplode() can fail without returning
> a proper error.
>
> This fix does the following:
> 1) After calculations are done, create UTC or local time.
> 2) Convert UTC or local time back to exploded
> 3) Compare original exploded with converted one
> 4) If times are not equal, then return Time(0) indicating
> an error.
>
> Windows implementation already returns Time(0) on error.
>
> BUG= 601900 
>
> Committed: https://crrev.com/ceba9fb480269695775191d14e98ab23b5918382
> Cr-Commit-Position: refs/heads/master@{#396638}

TBR=mark@chromium.org,mmenke@chromium.org,eroman@chromium.org,maksim.sisov@intel.com
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 601900 

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

[modify] https://crrev.com/8c687a70b52123f35c0504a5b3c5c84a639cebaf/base/time/time.cc
[modify] https://crrev.com/8c687a70b52123f35c0504a5b3c5c84a639cebaf/base/time/time.h
[modify] https://crrev.com/8c687a70b52123f35c0504a5b3c5c84a639cebaf/base/time/time_mac.cc
[modify] https://crrev.com/8c687a70b52123f35c0504a5b3c5c84a639cebaf/base/time/time_posix.cc
[modify] https://crrev.com/8c687a70b52123f35c0504a5b3c5c84a639cebaf/base/time/time_unittest.cc
[modify] https://crrev.com/8c687a70b52123f35c0504a5b3c5c84a639cebaf/base/time/time_win.cc
Project Member

Comment 9 by bugdroid1@chromium.org, Jun 14 2016 

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

commit 558f168054566ecbc611918093398add04dc13a4
Author: maksim.sisov <maksim.sisov@intel.com>
Date: Tue Jun 14 21:57:05 2016

Add: check exploded time is properly converted.

This cl introduces time checking in posix-like and mac systems.
base::Time::FromUTCExploded() and
base::Time::FromLocalExplode() can fail without returning
a proper error.

This fix does the following:
1) After calculations are done, create UTC or local time.
2) Convert UTC or local time back to exploded
3) Compare original exploded with converted one
4) If times are not equal, then return Time(0) indicating
an error.

Windows implementation already returns Time(0) on error.

BUG= 601900 

Committed: https://crrev.com/ceba9fb480269695775191d14e98ab23b5918382
Review-Url: https://codereview.chromium.org/1988663002
Cr-Original-Commit-Position: refs/heads/master@{#396638}
Cr-Commit-Position: refs/heads/master@{#399794}

[modify] https://crrev.com/558f168054566ecbc611918093398add04dc13a4/base/time/time.cc
[modify] https://crrev.com/558f168054566ecbc611918093398add04dc13a4/base/time/time.h
[modify] https://crrev.com/558f168054566ecbc611918093398add04dc13a4/base/time/time_mac.cc
[modify] https://crrev.com/558f168054566ecbc611918093398add04dc13a4/base/time/time_posix.cc
[modify] https://crrev.com/558f168054566ecbc611918093398add04dc13a4/base/time/time_unittest.cc
[modify] https://crrev.com/558f168054566ecbc611918093398add04dc13a4/base/time/time_win.cc
Project Member

Comment 10 by bugdroid1@chromium.org, Jun 15 2016 

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

commit 558f168054566ecbc611918093398add04dc13a4
Author: maksim.sisov <maksim.sisov@intel.com>
Date: Tue Jun 14 21:57:05 2016

Add: check exploded time is properly converted.

This cl introduces time checking in posix-like and mac systems.
base::Time::FromUTCExploded() and
base::Time::FromLocalExplode() can fail without returning
a proper error.

This fix does the following:
1) After calculations are done, create UTC or local time.
2) Convert UTC or local time back to exploded
3) Compare original exploded with converted one
4) If times are not equal, then return Time(0) indicating
an error.

Windows implementation already returns Time(0) on error.

BUG= 601900 

Committed: https://crrev.com/ceba9fb480269695775191d14e98ab23b5918382
Review-Url: https://codereview.chromium.org/1988663002
Cr-Original-Commit-Position: refs/heads/master@{#396638}
Cr-Commit-Position: refs/heads/master@{#399794}

[modify] https://crrev.com/558f168054566ecbc611918093398add04dc13a4/base/time/time.cc
[modify] https://crrev.com/558f168054566ecbc611918093398add04dc13a4/base/time/time.h
[modify] https://crrev.com/558f168054566ecbc611918093398add04dc13a4/base/time/time_mac.cc
[modify] https://crrev.com/558f168054566ecbc611918093398add04dc13a4/base/time/time_posix.cc
[modify] https://crrev.com/558f168054566ecbc611918093398add04dc13a4/base/time/time_unittest.cc
[modify] https://crrev.com/558f168054566ecbc611918093398add04dc13a4/base/time/time_win.cc
Project Member

Comment 13 by bugdroid1@chromium.org, Jun 29 2016 

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

commit 5ae45866acc94635f19188f535d813a7c9c9488d
Author: maksim.sisov <maksim.sisov@intel.com>
Date: Wed Jun 29 05:14:29 2016

Make callers of FromUTC(Local)Exploded in crypto/ use new time API.

Use new time conversion API in accordance with
https://codereview.chromium.org/1988663002/

BUG= 601900 

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

[modify] https://crrev.com/5ae45866acc94635f19188f535d813a7c9c9488d/crypto/nss_util_unittest.cc
Project Member

Comment 14 by bugdroid1@chromium.org, Jul 7 2016 

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

commit 1101de3fbaecd6c38d16497306c87fbff5f31599
Author: maksim.sisov <maksim.sisov@intel.com>
Date: Thu Jul 07 06:14:26 2016

Make callers of FromUTC(Local)Exploded in ppapi/ use new time API.

Use new time conversion API in accordance with
https://codereview.chromium.org/1988663002/

BUG= 601900 

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

[modify] https://crrev.com/1101de3fbaecd6c38d16497306c87fbff5f31599/ppapi/shared_impl/time_conversion.cc
Project Member

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

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

commit 040af55f34835e31de7a9b01ed9689977fcd7011
Author: maksim.sisov <maksim.sisov@intel.com>
Date: Thu Jul 07 06:56:54 2016

Make callers of FromUTC(Local)Exploded in components/ use new time API.

Use new time conversion API in accordance with
https://codereview.chromium.org/1988663002/

BUG= 601900 

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

[modify] https://crrev.com/040af55f34835e31de7a9b01ed9689977fcd7011/components/autofill/content/browser/risk/fingerprint.cc
[modify] https://crrev.com/040af55f34835e31de7a9b01ed9689977fcd7011/components/data_reduction_proxy/core/browser/data_usage_store.cc
[modify] https://crrev.com/040af55f34835e31de7a9b01ed9689977fcd7011/components/data_reduction_proxy/core/browser/data_usage_store_unittest.cc
[modify] https://crrev.com/040af55f34835e31de7a9b01ed9689977fcd7011/components/drive/file_system/touch_operation_unittest.cc
[modify] https://crrev.com/040af55f34835e31de7a9b01ed9689977fcd7011/components/drive/resource_entry_conversion_unittest.cc
[modify] https://crrev.com/040af55f34835e31de7a9b01ed9689977fcd7011/components/drive/service/fake_drive_service_unittest.cc
[modify] https://crrev.com/040af55f34835e31de7a9b01ed9689977fcd7011/components/ntp_snippets/ntp_snippets_service.cc
[modify] https://crrev.com/040af55f34835e31de7a9b01ed9689977fcd7011/components/ntp_snippets/ntp_snippets_service_unittest.cc
[modify] https://crrev.com/040af55f34835e31de7a9b01ed9689977fcd7011/components/password_manager/core/browser/password_store.cc
[modify] https://crrev.com/040af55f34835e31de7a9b01ed9689977fcd7011/components/variations/variations_seed_store.cc
Project Member

Comment 16 by bugdroid1@chromium.org, Jul 7 2016 

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

commit b4dbc73df4c9c1c9bc3439d4c0f79c79b64f18f4
Author: maksim.sisov <maksim.sisov@intel.com>
Date: Thu Jul 07 09:17:27 2016

Make callers of FromUTC(Local)Exploded in chrome/ use new time API.

Use new time conversion API in accordance with
https://codereview.chromium.org/1988663002/

BUG= 601900 

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

[modify] https://crrev.com/b4dbc73df4c9c1c9bc3439d4c0f79c79b64f18f4/chrome/browser/banners/app_banner_settings_helper_unittest.cc
[modify] https://crrev.com/b4dbc73df4c9c1c9bc3439d4c0f79c79b64f18f4/chrome/browser/chromeos/policy/device_status_collector.cc
[modify] https://crrev.com/b4dbc73df4c9c1c9bc3439d4c0f79c79b64f18f4/chrome/browser/engagement/site_engagement_score_unittest.cc
[modify] https://crrev.com/b4dbc73df4c9c1c9bc3439d4c0f79c79b64f18f4/chrome/browser/engagement/site_engagement_service_unittest.cc
[modify] https://crrev.com/b4dbc73df4c9c1c9bc3439d4c0f79c79b64f18f4/chrome/browser/extensions/convert_web_app_unittest.cc
[modify] https://crrev.com/b4dbc73df4c9c1c9bc3439d4c0f79c79b64f18f4/chrome/browser/media_galleries/fileapi/picasa_file_util_unittest.cc
[modify] https://crrev.com/b4dbc73df4c9c1c9bc3439d4c0f79c79b64f18f4/chrome/browser/password_manager/password_store_mac.cc
[modify] https://crrev.com/b4dbc73df4c9c1c9bc3439d4c0f79c79b64f18f4/chrome/browser/ui/webui/browsing_history_handler.cc
[modify] https://crrev.com/b4dbc73df4c9c1c9bc3439d4c0f79c79b64f18f4/chrome/browser/ui/webui/media/webrtc_logs_ui.cc
[modify] https://crrev.com/b4dbc73df4c9c1c9bc3439d4c0f79c79b64f18f4/chrome/installer/gcapi/gcapi_omaha_experiment.cc
[modify] https://crrev.com/b4dbc73df4c9c1c9bc3439d4c0f79c79b64f18f4/chrome/utility/media_galleries/picasa_album_table_reader.cc
[modify] https://crrev.com/b4dbc73df4c9c1c9bc3439d4c0f79c79b64f18f4/ios/chrome/browser/ui/webui/history/browsing_history_handler.cc

Comment 17 by mark@chromium.org, Aug 25 2016

Blockedon: 641096
Project Member

Comment 19 by bugdroid1@chromium.org, Dec 20 2016 

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

commit c8c6cd130d570c7b269e6c10e6716631afd3a648
Author: maksim.sisov <maksim.sisov@intel.com>
Date: Tue Dec 20 13:36:00 2016

Make callers of FromUTC(Local)Exploded in net/ use new time API.

Use new time conversion API in accordance with
https://codereview.chromium.org/1988663002/

BUG= 601905 , 601903 , 601900 

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

[modify] https://crrev.com/c8c6cd130d570c7b269e6c10e6716631afd3a648/net/cert/ct_policy_enforcer_unittest.cc
[modify] https://crrev.com/c8c6cd130d570c7b269e6c10e6716631afd3a648/net/cert/x509_cert_types.cc
[modify] https://crrev.com/c8c6cd130d570c7b269e6c10e6716631afd3a648/net/cert/x509_cert_types_unittest.cc
[modify] https://crrev.com/c8c6cd130d570c7b269e6c10e6716631afd3a648/net/disk_cache/blockfile/eviction.cc
[modify] https://crrev.com/c8c6cd130d570c7b269e6c10e6716631afd3a648/net/extras/sqlite/sqlite_channel_id_store_unittest.cc
[modify] https://crrev.com/c8c6cd130d570c7b269e6c10e6716631afd3a648/net/ftp/ftp_directory_listing_parser_ls.cc
[modify] https://crrev.com/c8c6cd130d570c7b269e6c10e6716631afd3a648/net/ftp/ftp_directory_listing_parser_unittest.cc
[modify] https://crrev.com/c8c6cd130d570c7b269e6c10e6716631afd3a648/net/ftp/ftp_directory_listing_parser_unittest.h
[modify] https://crrev.com/c8c6cd130d570c7b269e6c10e6716631afd3a648/net/ftp/ftp_directory_listing_parser_vms.cc
[modify] https://crrev.com/c8c6cd130d570c7b269e6c10e6716631afd3a648/net/ftp/ftp_util.cc
[modify] https://crrev.com/c8c6cd130d570c7b269e6c10e6716631afd3a648/net/http/http_util_unittest.cc

Comment 20 by maksim.s...@intel.com, Dec 21 2016

Labels: OS-Chrome OS-Linux OS-Mac
Status: Started (was: Untriaged)

Comment 22 by maksim.s...@intel.com, Dec 21 2016

Status: Fixed (was: Started)
►

Sign in to add a comment