Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2377973002:
Fix race condition on OSCrypt linux (Closed)
|
Created:
4 years, 2 months ago by cfroussios Modified:
4 years, 2 months ago Reviewers:
Lei Zhang CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix race condition on OSCrypt linux
This fixes the test that is broken on tsan.
BUG=650902, 631171
Committed: https://crrev.com/840961a10fc0c0e24299ced23a60908ffa09e9d1
Cr-Commit-Position: refs/heads/master@{#421605}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Call GetEncryptionKey only once #Patch Set 3 : call with version #Patch Set 4 : move comment #
Total comments: 3
Patch Set 5 : typo #Messages
Total messages: 32 (22 generated)
The CQ bit was checked by cfroussios@chromium.org 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: This issue passed the CQ dry run.
Description was changed from ========== Fix race condition on OSCrypt linux This fixes the test that is broken on tsan. BUG=650902,631171 ========== to ========== Fix race condition on OSCrypt linux This fixes the test that is broken on tsan. BUG=650902,631171 ==========
cfroussios@chromium.org changed reviewers: + thestig@chromium.org
Hi! Can you review this fix? Thanks! Also, just to be clear, tsan isn't supposed to give false positives, correct?
I'd like to think TSAN is correct most of the time. https://codereview.chromium.org/2377973002/diff/1/components/os_crypt/os_cryp... File components/os_crypt/os_crypt_linux.cc (right): https://codereview.chromium.org/2377973002/diff/1/components/os_crypt/os_cryp... components/os_crypt/os_crypt_linux.cc:155: // If a |KeyStorage| is available, use a password backed by the |KeyStorage|. The comment is a bit out of date. https://codereview.chromium.org/2377973002/diff/1/components/os_crypt/os_cryp... components/os_crypt/os_crypt_linux.cc:161: GetEncryptionKey(version)); Aren't we calling GetEncryptionKey() twice in a row now?
https://codereview.chromium.org/2377973002/diff/1/components/os_crypt/os_cryp... File components/os_crypt/os_crypt_linux.cc (right): https://codereview.chromium.org/2377973002/diff/1/components/os_crypt/os_cryp... components/os_crypt/os_crypt_linux.cc:161: GetEncryptionKey(version)); On 2016/09/28 15:35:06, Lei Zhang wrote: > Aren't we calling GetEncryptionKey() twice in a row now? So I was thinking the code below does the same but avoids the second GetEncryptionKey() call if the first succeeds. WDYT? Version version = Version::V11; std::unique_ptr<crypto::SymmetricKey> encryption_key = GetEncryptionKey(version); if (!encryption_key) { version = Version::V10; encryption_key = GetEncryptionKey(version); } if (!encryption_key) return false;
The CQ bit was checked by cfroussios@chromium.org 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 cfroussios@chromium.org 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 cfroussios@chromium.org 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...
https://codereview.chromium.org/2377973002/diff/1/components/os_crypt/os_cryp... File components/os_crypt/os_crypt_linux.cc (right): https://codereview.chromium.org/2377973002/diff/1/components/os_crypt/os_cryp... components/os_crypt/os_crypt_linux.cc:155: // If a |KeyStorage| is available, use a password backed by the |KeyStorage|. On 2016/09/28 15:35:06, Lei Zhang wrote: > The comment is a bit out of date. Done. https://codereview.chromium.org/2377973002/diff/1/components/os_crypt/os_cryp... components/os_crypt/os_crypt_linux.cc:161: GetEncryptionKey(version)); On 2016/09/28 15:43:21, Lei Zhang wrote: > On 2016/09/28 15:35:06, Lei Zhang wrote: > > Aren't we calling GetEncryptionKey() twice in a row now? > > So I was thinking the code below does the same but avoids the second > GetEncryptionKey() call if the first succeeds. WDYT? > > Version version = Version::V11; > std::unique_ptr<crypto::SymmetricKey> encryption_key = > GetEncryptionKey(version); > if (!encryption_key) { > version = Version::V10; > encryption_key = GetEncryptionKey(version); > } > if (!encryption_key) > return false; I had a fix ready for this, but I was thinking that I wanted |encryption_key| to continue and explicitly depend on |version|. I'm going to go with your code.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2377973002/diff/60001/components/os_crypt/os_... File components/os_crypt/os_crypt_linux.cc (right): https://codereview.chromium.org/2377973002/diff/60001/components/os_crypt/os_... components/os_crypt/os_crypt_linux.cc:155: // If we are able to create a V11 key (i.e. a KeyStorage was avaible), then available
The CQ bit was checked by cfroussios@chromium.org 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...
https://codereview.chromium.org/2377973002/diff/60001/components/os_crypt/os_... File components/os_crypt/os_crypt_linux.cc (right): https://codereview.chromium.org/2377973002/diff/60001/components/os_crypt/os_... components/os_crypt/os_crypt_linux.cc:155: // If we are able to create a V11 key (i.e. a KeyStorage was avaible), then On 2016/09/28 16:45:14, Lei Zhang wrote: > available Eh. I'll just fix this in a follow up CL.
https://codereview.chromium.org/2377973002/diff/60001/components/os_crypt/os_... File components/os_crypt/os_crypt_linux.cc (right): https://codereview.chromium.org/2377973002/diff/60001/components/os_crypt/os_... components/os_crypt/os_crypt_linux.cc:155: // If we are able to create a V11 key (i.e. a KeyStorage was avaible), then On 2016/09/28 17:32:58, Lei Zhang wrote: > On 2016/09/28 16:45:14, Lei Zhang wrote: > > available > > Eh. I'll just fix this in a follow up CL. Oh wait, you did, I just didn't see an email from you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by thestig@chromium.org
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/2377973002/#ps80001 (title: "typo")
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 race condition on OSCrypt linux This fixes the test that is broken on tsan. BUG=650902,631171 ========== to ========== Fix race condition on OSCrypt linux This fixes the test that is broken on tsan. BUG=650902,631171 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix race condition on OSCrypt linux This fixes the test that is broken on tsan. BUG=650902,631171 ========== to ========== Fix race condition on OSCrypt linux This fixes the test that is broken on tsan. BUG=650902,631171 Committed: https://crrev.com/840961a10fc0c0e24299ced23a60908ffa09e9d1 Cr-Commit-Position: refs/heads/master@{#421605} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/840961a10fc0c0e24299ced23a60908ffa09e9d1 Cr-Commit-Position: refs/heads/master@{#421605} |
