Thread-unsafe use of statics in CertVerifyProc::HasTooLongValidity() |
|||
Issue description
CertVerifyProc::HasTooLongValidity() [1] initializes static variables:
static const base::Time time_2012_07_01 =
base::Time::FromUTCExploded({2012, 7, 0, 1, 0, 0, 0, 0});
static const base::Time time_2015_04_01 =
base::Time::FromUTCExploded({2015, 4, 0, 1, 0, 0, 0, 0});
static const base::Time time_2019_07_01 =
base::Time::FromUTCExploded({2019, 7, 0, 1, 0, 0, 0, 0});
This is problematic:
(a) Initialization of local statics cannot be relied on as thread-safe in Chrome [2]
(b) Runs destructors at shutdown (dangerous for code running in worker pool, although probably a non-issue for base::Time).
A more appropriate pattern would probably be to use a leaky singleton.
Or in this case can probably remove the code all together and compare exploded times directly.
[1] https://cs.chromium.org/chromium/src/net/cert/cert_verify_proc.cc?sq=package:chromium&dr=C&rcl=1476847656&l=696
[2] Despite C++11 requiring threadsafe initialization, Chromium code historically hasn't supported this, and to this day compiles with -fno-threadsafe-statics
,
Oct 20 2016
No need for TimeDelta -- If you don't care about readability, can just use base::Time::FromInternalValue(), and construct the comparison times that way. Or could keep the exploded representation and then comapare via exploded_start and exploded_expiry
,
Oct 20 2016
,
Oct 20 2016
If it were up to me, I'd just go with FromInternalValue, and then have unittests make sure that the correct ranges are actually enforced.
,
Oct 20 2016
@mmenke: Yep, that is what I proposed in https://codereview.chromium.org/2441713002/. It also matches other locations (in that file) which rely on hardcoded time stamps.
,
Oct 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/241b859db909a92795cf74a34ea1aa313f8ce64d commit 241b859db909a92795cf74a34ea1aa313f8ce64d Author: eroman <eroman@chromium.org> Date: Thu Oct 20 22:15:07 2016 Remove some thread-unsafe statics in CertVerifyProc::HasTooLongValidity(). BUG= 657903 Review-Url: https://chromiumcodereview.appspot.com/2441713002 Cr-Commit-Position: refs/heads/master@{#426616} [modify] https://crrev.com/241b859db909a92795cf74a34ea1aa313f8ce64d/net/cert/cert_verify_proc.cc
,
Oct 20 2016
,
Oct 20 2016
For what it's worth, TimeDelta makes it clearer that it's not an internal function - the TimeDelta will be converted into the FromInternalValue by the compiler, so it ends up optimized the same, but slightly clearer. But the consistency w/in the file is fine - but for future concerns :) |
|||
►
Sign in to add a comment |
|||
Comment 1 by rsleevi@chromium.org
, Oct 20 2016