Force crypto::AppleKeychain access to be guarded by a Big Global Lock

Force crypto::AppleKeychain access to be guarded by a Big Global Lock

Apple removed the Big Global Lock guarding the Security.framework API,
but there are a number of thread-unsafe places in the API. Additionally,
it seems that OS X 10.8.2 has introduced some deadlock potential, so
force calls to be serialized behind a Chrome-supplied Big Global Lock
until it's safe to do otherwise.

BUG=151707
TEST=See bug


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=159839

[Ryan Sleevi, 2012-09-29 08:11:28]

rsesek, scottbyer: Could one of you test?
kerz: FYI

Will TBR it to wtc@ if this actually (partially) mitigates the issue.

[Robert Sesek, 2012-09-29 20:54:37]

I didn't see this issue locally and cannot repro. I can ask a coworker on Monday
to try it via his profile. But this LGTM.

[Scott Byer, 2012-09-30 01:21:37]

I won't get a chance to try this out until first thing Monday morning,
unfortunately. I'll make it a priority - generally it seems the more profiles
with more logged in pages, the easier, but it still can take a few tries,
depending.

On 2012/09/29 20:54:37, rsesek wrote:
> I didn't see this issue locally and cannot repro. I can ask a coworker on
Monday
> to try it via his profile. But this LGTM.

[Scott Byer, 2012-10-01 16:26:12]

Testing now...

On 2012/09/30 01:21:37, Scott Byer wrote:
> I won't get a chance to try this out until first thing Monday morning,
> unfortunately. I'll make it a priority - generally it seems the more profiles
> with more logged in pages, the easier, but it still can take a few tries,
> depending.
> 
> On 2012/09/29 20:54:37, rsesek wrote:
> > I didn't see this issue locally and cannot repro. I can ask a coworker on
> Monday
> > to try it via his profile. But this LGTM.

[Ryan Sleevi, 2012-10-01 18:30:11]

On 2012/10/01 16:26:12, Scott Byer wrote:
> Testing now...

Initial results weren't promising - it looked like the deadlock moved from being
between two password threads to 1 password + 1 cert verify thread.

Turns out cert verification was using a verifier-local lock, rather than a truly
Big Global Lock. Updated CertVerifyProcMac to also use the BGL.

[wtc, 2012-10-02 01:46:42]

Review comments on patch set 7:

This solution is brittle. It'll be easy to add a new SecTrust
call without knowing it needs to be guarded by a global lock.
Does Apple know about this bug? In any case, we'll do what we
need to do.

http://codereview.chromium.org/11016004/diff/14001/net/base/cert_verify_proc_...
File net/base/cert_verify_proc_mac.cc (right):

http://codereview.chromium.org/11016004/diff/14001/net/base/cert_verify_proc_...
net/base/cert_verify_proc_mac.cc:370: // So as a speculative fix allow only one
thread to use SecTrust on this cert.

"on this cert" needs to be removed because the lock is no
longer a member of this cert. The lock is a global lock now,
applying to all certs.

[Ryan Sleevi, 2012-10-02 02:10:04]

On Oct 1, 2012 9:46 PM, <wtc@chromium.org> wrote:
>
> Review comments on patch set 7:
>
> This solution is brittle. It'll be easy to add a new SecTrust
> call without knowing it needs to be guarded by a global lock.
> Does Apple know about this bug? In any case, we'll do what we
> need to do.

Agreed on the brittleness. I really really don't like this as a solution -
but I want to keep something mergeable.

There are still calls not guarded (eg: the CDSA calls within NSS for client
auth) - this just catches the outliers for most users and Googlers.

As far as Apple goes, yes, a radar has been created and it has been
escalated with contacts. But I suspect we will need hacks like this for
some time :-/

>
>
>
http://codereview.chromium.org/11016004/diff/14001/net/base/cert_verify_proc_...
> File net/base/cert_verify_proc_mac.cc (right):
>
>
http://codereview.chromium.org/11016004/diff/14001/net/base/cert_verify_proc_...
> net/base/cert_verify_proc_mac.cc:370: // So as a speculative fix allow
> only one thread to use SecTrust on this cert.
>
> "on this cert" needs to be removed because the lock is no
> longer a member of this cert. The lock is a global lock now,
> applying to all certs.
>
> http://codereview.chromium.org/11016004/

[Scott Byer, 2012-10-02 17:33:05]

LGTM. Went through 50 launches using patch set 7, no issues.

On 2012/10/02 02:10:04, Ryan Sleevi wrote:
> On Oct 1, 2012 9:46 PM, <mailto:wtc@chromium.org> wrote:
> >
> > Review comments on patch set 7:
> >
> > This solution is brittle. It'll be easy to add a new SecTrust
> > call without knowing it needs to be guarded by a global lock.
> > Does Apple know about this bug? In any case, we'll do what we
> > need to do.
> 
> Agreed on the brittleness. I really really don't like this as a solution -
> but I want to keep something mergeable.
> 
> There are still calls not guarded (eg: the CDSA calls within NSS for client
> auth) - this just catches the outliers for most users and Googlers.
> 
> As far as Apple goes, yes, a radar has been created and it has been
> escalated with contacts. But I suspect we will need hacks like this for
> some time :-/
> 
> >
> >
> >
>
http://codereview.chromium.org/11016004/diff/14001/net/base/cert_verify_proc_...
> > File net/base/cert_verify_proc_mac.cc (right):
> >
> >
>
http://codereview.chromium.org/11016004/diff/14001/net/base/cert_verify_proc_...
> > net/base/cert_verify_proc_mac.cc:370: // So as a speculative fix allow
> > only one thread to use SecTrust on this cert.
> >
> > "on this cert" needs to be removed because the lock is no
> > longer a member of this cert. The lock is a global lock now,
> > applying to all certs.
> >
> > http://codereview.chromium.org/11016004/

[commit-bot: I haz the power, 2012-10-02 23:24:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/11016004/16001

[commit-bot: I haz the power, 2012-10-03 05:46:47]

Change committed as 159839