Thread safe initialization of OSCrypt cache

Thread safe initialization of OSCrypt cache

Clients of OSCrypt run in multiple threads. This creates a race
condition for the lazy initialization of OSCrypt, which currently
isn't thread safe.

Additionally, when a synchronous call to libsecret blocks waiting for
user permission, parallel calls to it do not block on the same prompt.
Rather, they fail. This means that, while the first caller will
correctly identify that encryption should be used, subsequent callers
will behave as if full encryption is not available, until the first
caller gets permission and properly initialises OSCrypt.

Fixing thread safety should fix reported cases of data loss.

BUG=631171
Committed: https://crrev.com/fc6f304827896fb27397fc2a27f9f5717eff319c
Cr-Commit-Position: refs/heads/master@{#421097}

[cfroussios, 2016-09-21 14:17:43]

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

[commit-bot: I haz the power, 2016-09-21 14:17:51]

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

[cfroussios, 2016-09-21 14:40:28]

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

[commit-bot: I haz the power, 2016-09-21 14:40:36]

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

[cfroussios, 2016-09-21 14:43:19]

Description was changed from

==========
Thread safe initilization of OSCrypt cache

Clients of OSCrypt run in multiple threads. This creates a race
condition for the lazy initialisation of the OSCrypt.

Additionally, when a synchronous call to libsecret blocks waiting for
user permission, parallel calls to it do not block on the same prompt.
Rather, they fail. This means that, while the first caller will
correctly identify that encryption should be used, subsequent callers
will behave as if full encryption is not available, until the first
caller gets permission and properly initilises OSCrypt.

BUG=495801
==========

to

==========
Thread safe initilization of OSCrypt cache

Clients of OSCrypt run in multiple threads. This creates a race
condition for the lazy initialisation of the OSCrypt.

Additionally, when a synchronous call to libsecret blocks waiting for
user permission, parallel calls to it do not block on the same prompt.
Rather, they fail. This means that, while the first caller will
correctly identify that encryption should be used, subsequent callers
will behave as if full encryption is not available, until the first
caller gets permission and properly initilises OSCrypt.

BUG=631171
==========

[cfroussios, 2016-09-21 14:45:52]

Description was changed from

==========
Thread safe initilization of OSCrypt cache

Clients of OSCrypt run in multiple threads. This creates a race
condition for the lazy initialisation of the OSCrypt.

Additionally, when a synchronous call to libsecret blocks waiting for
user permission, parallel calls to it do not block on the same prompt.
Rather, they fail. This means that, while the first caller will
correctly identify that encryption should be used, subsequent callers
will behave as if full encryption is not available, until the first
caller gets permission and properly initilises OSCrypt.

BUG=631171
==========

to

==========
Thread safe initialization of OSCrypt cache

Clients of OSCrypt run in multiple threads. This creates a race
condition for the lazy initialisation of OSCrypt, which currently
isn't thread safe.

Additionally, when a synchronous call to libsecret blocks waiting for
user permission, parallel calls to it do not block on the same prompt.
Rather, they fail. This means that, while the first caller will
correctly identify that encryption should be used, subsequent callers
will behave as if full encryption is not available, until the first
caller gets permission and properly initialises OSCrypt.

Fixing thread safety should fix reported cases of data loss.

BUG=631171
==========

[cfroussios, 2016-09-21 14:52:41]

cfroussios@chromium.org changed reviewers:
+ thestig@chromium.org

[commit-bot: I haz the power, 2016-09-21 14:56:18]

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

[commit-bot: I haz the power, 2016-09-21 14:56:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[cfroussios, 2016-09-21 15:01:04]

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

[commit-bot: I haz the power, 2016-09-21 15:01:47]

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

[cfroussios, 2016-09-21 15:14:37]

Hi! Can you review this bug fix? Thanks!

[commit-bot: I haz the power, 2016-09-21 16:21:09]

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

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

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-09-21 19:04:56]

Let's add unit tests to exercise this code from multiple threads. Then we can
run it through TSANv2.
https://www.chromium.org/developers/testing/threadsanitizer-tsan-v2

Commit msg / subject line: "initilization" is a typo.

https://codereview.chromium.org/2359803002/diff/60001/components/os_crypt/os_...
File components/os_crypt/os_crypt_linux.cc (right):

https://codereview.chromium.org/2359803002/diff/60001/components/os_crypt/os_...
components/os_crypt/os_crypt_linux.cc:64: base::Lock g_cache_lock;
Does this add a static initializer? Maybe make it part of struct Cache?

[Lei Zhang, 2016-09-21 19:10:35]

BTW, what's the current call stack from OSCrypt clients? Do they go through
OSCrypt::{En,De}cryptString()? Should we add comments saying these can be called
on any thread?

[cfroussios, 2016-09-22 09:08:57]

Description was changed from

==========
Thread safe initialization of OSCrypt cache

Clients of OSCrypt run in multiple threads. This creates a race
condition for the lazy initialisation of OSCrypt, which currently
isn't thread safe.

Additionally, when a synchronous call to libsecret blocks waiting for
user permission, parallel calls to it do not block on the same prompt.
Rather, they fail. This means that, while the first caller will
correctly identify that encryption should be used, subsequent callers
will behave as if full encryption is not available, until the first
caller gets permission and properly initialises OSCrypt.

Fixing thread safety should fix reported cases of data loss.

BUG=631171
==========

to

==========
Thread safe initialization of OSCrypt cache

Clients of OSCrypt run in multiple threads. This creates a race
condition for the lazy initialization of OSCrypt, which currently
isn't thread safe.

Additionally, when a synchronous call to libsecret blocks waiting for
user permission, parallel calls to it do not block on the same prompt.
Rather, they fail. This means that, while the first caller will
correctly identify that encryption should be used, subsequent callers
will behave as if full encryption is not available, until the first
caller gets permission and properly initialises OSCrypt.

Fixing thread safety should fix reported cases of data loss.

BUG=631171
==========

[cfroussios, 2016-09-22 17:55:22]

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

[commit-bot: I haz the power, 2016-09-22 17:55:46]

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

[cfroussios, 2016-09-22 17:56:57]

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

[commit-bot: I haz the power, 2016-09-22 17:57:22]

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

[cfroussios, 2016-09-22 18:04:53]

I added some tests. TSan would not accept the current pattern (reading the
initialization condition outside of the guarded code), so I changed to something
less efficient unfortunately.

I don't exactly follow your point about the clients' call stack. Apart from the
3 initialisation functions, and mocking for tests, encrypt/decrypt are the only
entry points to OSCrypt. Calling these methods thread-safe would be accurate for
Mac and Linux. I didn't see any locks in windows, so I don't know how the issue
is dealt there and if it's accurate to add a comment about every case.

https://codereview.chromium.org/2359803002/diff/60001/components/os_crypt/os_...
File components/os_crypt/os_crypt_linux.cc (right):

https://codereview.chromium.org/2359803002/diff/60001/components/os_crypt/os_...
components/os_crypt/os_crypt_linux.cc:64: base::Lock g_cache_lock;
On 2016/09/21 19:04:56, Lei Zhang wrote:
> Does this add a static initializer? Maybe make it part of struct Cache?

Done.

[commit-bot: I haz the power, 2016-09-22 18:59:38]

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

[commit-bot: I haz the power, 2016-09-22 18:59:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[cfroussios, 2016-09-23 11:49:14]

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

[commit-bot: I haz the power, 2016-09-23 11:49:26]

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-09-23 12:03:44]

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

[commit-bot: I haz the power, 2016-09-23 12:03:44]

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

[Lei Zhang, 2016-09-24 01:30:35]

lgtm

On 2016/09/22 18:04:53, cfroussios wrote:
> I added some tests. TSan would not accept the current pattern (reading the
> initialization condition outside of the guarded code), so I changed to
something
> less efficient unfortunately.

I think it'll be fine.

> I don't exactly follow your point about the clients' call stack. Apart from
the
> 3 initialisation functions, and mocking for tests, encrypt/decrypt are the
only
> entry points to OSCrypt. Calling these methods thread-safe would be accurate
for
> Mac and Linux. I didn't see any locks in windows, so I don't know how the
issue
> is dealt there and if it's accurate to add a comment about every case.

Ah, it's just 2 calls to encrypt/decrypt racing against each other? Wasn't 100%
sure who was participating in the race.

[cfroussios, 2016-09-26 08:57:58]

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

[commit-bot: I haz the power, 2016-09-26 08:58:12]

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-09-26 10:01:10]

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

[commit-bot: I haz the power, 2016-09-26 10:01:11]

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

[cfroussios, 2016-09-26 10:48:04]

The CQ bit was checked by cfroussios@chromium.org

[cfroussios, 2016-09-26 10:48:04]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2359803002/#ps160001
(title: "lint")

[commit-bot: I haz the power, 2016-09-26 10:48:15]

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-09-26 11:59:10]

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

[commit-bot: I haz the power, 2016-09-26 11:59:11]

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

[Lei Zhang, 2016-09-26 18:12:40]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2016-09-26 18:13:12]

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-09-26 21:06:02]

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

[commit-bot: I haz the power, 2016-09-26 21:06:04]

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

[Lei Zhang, 2016-09-27 02:39:38]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2016-09-27 02:40:13]

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-09-27 04:11:37]

Description was changed from

==========
Thread safe initialization of OSCrypt cache

Clients of OSCrypt run in multiple threads. This creates a race
condition for the lazy initialization of OSCrypt, which currently
isn't thread safe.

Additionally, when a synchronous call to libsecret blocks waiting for
user permission, parallel calls to it do not block on the same prompt.
Rather, they fail. This means that, while the first caller will
correctly identify that encryption should be used, subsequent callers
will behave as if full encryption is not available, until the first
caller gets permission and properly initialises OSCrypt.

Fixing thread safety should fix reported cases of data loss.

BUG=631171
==========

to

==========
Thread safe initialization of OSCrypt cache

Clients of OSCrypt run in multiple threads. This creates a race
condition for the lazy initialization of OSCrypt, which currently
isn't thread safe.

Additionally, when a synchronous call to libsecret blocks waiting for
user permission, parallel calls to it do not block on the same prompt.
Rather, they fail. This means that, while the first caller will
correctly identify that encryption should be used, subsequent callers
will behave as if full encryption is not available, until the first
caller gets permission and properly initialises OSCrypt.

Fixing thread safety should fix reported cases of data loss.

BUG=631171
==========

[commit-bot: I haz the power, 2016-09-27 04:11:38]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-09-27 04:14:00]

Description was changed from

==========
Thread safe initialization of OSCrypt cache

Clients of OSCrypt run in multiple threads. This creates a race
condition for the lazy initialization of OSCrypt, which currently
isn't thread safe.

Additionally, when a synchronous call to libsecret blocks waiting for
user permission, parallel calls to it do not block on the same prompt.
Rather, they fail. This means that, while the first caller will
correctly identify that encryption should be used, subsequent callers
will behave as if full encryption is not available, until the first
caller gets permission and properly initialises OSCrypt.

Fixing thread safety should fix reported cases of data loss.

BUG=631171
==========

to

==========
Thread safe initialization of OSCrypt cache

Clients of OSCrypt run in multiple threads. This creates a race
condition for the lazy initialization of OSCrypt, which currently
isn't thread safe.

Additionally, when a synchronous call to libsecret blocks waiting for
user permission, parallel calls to it do not block on the same prompt.
Rather, they fail. This means that, while the first caller will
correctly identify that encryption should be used, subsequent callers
will behave as if full encryption is not available, until the first
caller gets permission and properly initialises OSCrypt.

Fixing thread safety should fix reported cases of data loss.

BUG=631171

Committed: https://crrev.com/fc6f304827896fb27397fc2a27f9f5717eff319c
Cr-Commit-Position: refs/heads/master@{#421097}
==========

[commit-bot: I haz the power, 2016-09-27 04:14:01]

Patchset 9 (id:??) landed as
https://crrev.com/fc6f304827896fb27397fc2a27f9f5717eff319c
Cr-Commit-Position: refs/heads/master@{#421097}