Minor clean-up tasks that were TODO(snej)

Minor clean-up tasks that were TODO(snej)

Rename Principal->CertPrincipal, Policy->CertPolicy, both of which are merely syntatic fluff.

Rename Fingerprint->SHA1Fingerprint, which is more important since those using the fingerprint, such as the unit tests, were truly hardcoded against SHA-1 fingerprints, and if the fingerprint ever changed, wouldn't cause errors until run time.

R=wtc
BUG=None
TEST=Compilers stay green

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

[Ryan Sleevi, 2010-07-04 09:41:30]

Wan-Teh,

   This is a follow-up to Jens' original CL at
http://codereview.chromium.org/1128008 , which is why the minor stuff (renaming)
was coupled with the major stuff (using NSS to do the name comparisons).

  One thing this does different was that in the original CL, you suggested using
NSS_CmpCertChainWCANames, which is what the straight NSS implementation uses.
The problem is that NSS_CmpCertChainWCANames doesn't normalize names, just
binary comparison. I'm not sure whether or not this is a bug in NSS (should it
be using CERT_CompareName as well? or is that just a legacy function we happen
to use?).

  Once/if the full switch to SSLClientSocketNSS happens, a lot of the code
related to this in x509_certificate_mac and x509_cert_types_mac will be able to
be removed. It's left in currently because of the --use-system-ssl switch,
though if NSS is always available (no build flags), I'd be happy to move this in
a common place so both SSLClientSocketMac and SSLSlicnetSocketNSS can use this.

[Ryan Sleevi, 2010-07-04 09:44:35]

http://codereview.chromium.org/2815048/diff/1/19
File net/socket/ssl_client_socket_nss_factory.cc (right):

http://codereview.chromium.org/2815048/diff/1/19#newcode32
net/socket/ssl_client_socket_nss_factory.cc:32: return new
SSLClientSocketMac(transport_socket, hostname, ssl_config);
I'm not sure if this should be pulled out into it's own little CL. It looks like
you did a similar change in ClientSocketFactory when you added NSS support to
Mac, though I'm not sure why. The path in Windows comes here to determine
whether to fallback, so I wanted to keep them consistent.

[wtc, 2010-07-14 23:31:19]

LGTM.  Thanks for doing the cleanup.

Please break this CL into two CLs.  The ssl_client_socket_nss.cc
changes are unrelated to and independent of the type
renaming.

I will review your ssl_client_socket_nss.cc changes again
and may send you more comments.

I marked some comments with "IMPORANT" below.  Others
are mostly nits.

http://codereview.chromium.org/2815048/diff/10001/11009
File net/base/ev_root_ca_metadata.h (right):

http://codereview.chromium.org/2815048/diff/10001/11009#newcode51
net/base/ev_root_ca_metadata.h:51: SHA1FingerprintLessThan> PolicyOidMap;
Nit: does this fit on the previous line now?

http://codereview.chromium.org/2815048/diff/10001/11010
File net/base/x509_cert_types.h (right):

http://codereview.chromium.org/2815048/diff/10001/11010#newcode11
net/base/x509_cert_types.h:11: #include <functional>
Nit: list <functional> before <iostream>.  (This file seems
to list the C++ system headers in alphabetical order.)

http://codereview.chromium.org/2815048/diff/10001/11010#newcode49
net/base/x509_cert_types.h:49: return memcmp(lhs.data, rhs.data,
arraysize(lhs.data)) < 0;
Using 'arraysize' is brittle if we ever change the definition
of the 'data' array.

Does sizeof(lhs.data) work?  See the definition of HA1Fingerprint::Equals
at line 39 above.

http://codereview.chromium.org/2815048/diff/10001/11011
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/2815048/diff/10001/11011#newcode38
net/base/x509_certificate.cc:38: // The cache does not hold a reference to the
certificate objects.  The objects
Should we add a TODO comment to fix this?  Generally it's
bad to hold a plain, non-reference pointer to a refcounted
object.

http://codereview.chromium.org/2815048/diff/10001/11011#newcode53
net/base/x509_certificate.cc:53: Cache() { }
Nit: remove the space in {}.  This is recommended by the
Style Guide with an example:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Horizontal_Whi...

http://codereview.chromium.org/2815048/diff/10001/11011#newcode113
net/base/x509_certificate.cc:113: bool
X509Certificate::IsSameOSCert(X509Certificate::OSCertHandle a,
We should not move these two functions.  Moving them make
your CL larger, and make it hard to verify the only change
you made is the FingerprintLessThan => SHA1FingerprintLessThan
renaming.

http://codereview.chromium.org/2815048/diff/10001/11013
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/2815048/diff/10001/11013#newcode833
net/base/x509_certificate_mac.cc:833: const SHA1Fingerprint fingerprint =
cert->fingerprint();
Make this a const reference, or remove 'const'.  I think
you meant to use a const reference here.

http://codereview.chromium.org/2815048/diff/10001/11014
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/2815048/diff/10001/11014#newcode710
net/base/x509_certificate_nss.cc:710:
X509Certificate::CalculateFingerprint(root_ca);
Nit: can this fit on the previous line now?

http://codereview.chromium.org/2815048/diff/10001/11017
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/2815048/diff/10001/11017#newcode8
net/socket/ssl_client_socket_nss.cc:8: // and
`anonymous-namespace'::ChainMatchesIssuerNames derived from
Nit: change the period on the previous line to a comma.

Remove `anonymous-namespace'::

Note: we don't need to acknowledge the source of we copied
just the algorithm, not the code.

Did you copy the code of NSS_CmpCertChainWCANames?  Can
you just copy the algorithm?  If you copied the code, we
now need to do it properly and put it under
src/net/third_party.

http://codereview.chromium.org/2815048/diff/10001/11017#newcode288
net/socket/ssl_client_socket_nss.cc:288: CFArrayRef results = NULL;
Nit: move the declaration of 'results' to line 341 below.
Move the declaration of tp_ssl_options to line 312 below.
(They should be declared near where they're first used.)

http://codereview.chromium.org/2815048/diff/10001/11017#newcode323
net/socket/ssl_client_socket_nss.cc:323: NULL, const_cast<const void**>(
IMPORTANT: remove const_cast.  It's not needed.

http://codereview.chromium.org/2815048/diff/10001/11017#newcode350
net/socket/ssl_client_socket_nss.cc:350: struct CERTDestroyer {
CERTDestroyer and ArenaDestroyer should be added to
src/base/crypto/scoped_nss_types.h, unless we want to
introduce a dependency on certs to "base".

http://codereview.chromium.org/2815048/diff/10001/11017#newcode371
net/socket/ssl_client_socket_nss.cc:371: *arena = PORT_NewArena(2048);
This function should always create the PLAreaPool or require
the caller to supply the PLArenaPool.  Don't support both.

http://codereview.chromium.org/2815048/diff/10001/11017#newcode389
net/socket/ssl_client_socket_nss.cc:389: // one of the certificate issues of
|ca_names|. This routine uses NSS to do
Typo: certificate issues of |ca_names| =>
      certificate issuers in |ca_names|

Nit: add a period at the end of line 391 and line 400.

http://codereview.chromium.org/2815048/diff/10001/11017#newcode413
net/socket/ssl_client_socket_nss.cc:413: // Import into NSS. This is because a
SecCertificateRef always stores it's
Typo: it's => its

I regret that we have to import the certs into NSS.  I
think we can just do binary comparison on the Mac if Mac's
certificate library doesn't support proper name comparison.

http://codereview.chromium.org/2815048/diff/10001/11017#newcode1491
net/socket/ssl_client_socket_nss.cc:1491: // function.
Nit: function => this function
otherwise it's not clear which function you're referring to
(this function or GetCertNamesForCAs).

I believe ca_names->arena is guaranteed to be non-NULL,
and we should depend on that.  But in general it is not
cool to "borrow" the internal arena of an object.

http://codereview.chromium.org/2815048/diff/10001/11017#newcode1511
net/socket/ssl_client_socket_nss.cc:1511: while (!err) {
This should be for (;;) or while (true).  'err' is guaranteed
to be 0 here because of the checks on lines 1506, 1514, and
1520.

http://codereview.chromium.org/2815048/diff/10001/11018
File net/socket/ssl_client_socket_nss_factory.cc (right):

http://codereview.chromium.org/2815048/diff/10001/11018#newcode30
net/socket/ssl_client_socket_nss_factory.cc:30: #elif defined(OS_MACOSX)
IMPORTANT: Undo this change and remove this file from the CL,
because this file is not used for Mac.

Only Windows needs this file because of the issue I described
in lines 13-16 above.  That issue doesn't affect the Mac.

[wtc, 2010-07-15 00:41:34]

More comments on ssl_client_socket_nss.cc.  I found two
off-by-one bugs (marked with "BUG" below).

http://codereview.chromium.org/2815048/diff/10001/11017
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/2815048/diff/10001/11017#newcode297
net/socket/ssl_client_socket_nss.cc:297: SecPolicyRef policy;
Nit: move the declaration of 'policy' to line 304.

Move the declaration of 'status_chain' from line 335 to
line 341.

http://codereview.chromium.org/2815048/diff/10001/11017#newcode371
net/socket/ssl_client_socket_nss.cc:371: *arena = PORT_NewArena(2048);
Use the macro DER_DEFAULT_CHUNKSIZE instead of 2048.

http://codereview.chromium.org/2815048/diff/10001/11017#newcode388
net/socket/ssl_client_socket_nss.cc:388: // Determines whether one of the
certificates in |chain| is, or is issued by,
IMPORTANT: I believe this should be simply "is issued by".
Your for loop doesn't use the derSubject or subject member of any
certificates in |chain|.

http://codereview.chromium.org/2815048/diff/10001/11017#newcode394
net/socket/ssl_client_socket_nss.cc:394: const std::vector<CERTName> &ca_names)
{
Nit: put & next to the type.

Nit: the CERTDistNames and std::vector<CERTName> parameter
names of this function and GetCertNamesForCAs above should
ideally be consistent.

http://codereview.chromium.org/2815048/diff/10001/11017#newcode404
net/socket/ssl_client_socket_nss.cc:404: for (CFIndex i = 0; i <
CFArrayGetCount(chain); ++i) {
Nit: should we count down, starting from the root CA?
This might hit the issuer faster.

http://codereview.chromium.org/2815048/diff/10001/11017#newcode438
net/socket/ssl_client_socket_nss.cc:438: // encoding). DER first, since it's the
best case scenario
Nit: add a period to this line and line 445 below.

http://codereview.chromium.org/2815048/diff/10001/11017#newcode1494
net/socket/ssl_client_socket_nss.cc:1494: if (!GetCertNamesForCAs(ca_names,
&arena, &normalized_ca_names)) {
Nit: we omit curly braces for simple, one-liner "if" bodies.

http://codereview.chromium.org/2815048/diff/10001/11017#newcode1529
net/socket/ssl_client_socket_nss.cc:1529: X509Certificate::OSCertHandles
x509_chain;
This variable is supposed to hold the intermediate CA certs,
so you can also name it intermediate_certs.

http://codereview.chromium.org/2815048/diff/10001/11017#newcode1532
net/socket/ssl_client_socket_nss.cc:1532:
x509_chain.resize(CFArrayGetCount(os_chain) - 1);
This can be simply
    x509_chain.resize(chain_size - 1);

Similarly CFArrayGetCount(os_chain) on the next line can be
replaced by chain_size.

BUG: the second argument to CFRangeMake should be
chain_size - 1 because it is the length of the range:
http://developer.apple.com/mac/library/documentation/CoreFoundation/Reference...

Remove the const_cast on line 1534.

BUG: the array index in line 1535 should be 1, not 0.

[wtc, 2010-07-15 00:49:00]

Ryan: before you make any of my suggested changes, please first
split ssl_client_socket_nss.cc into a separate CL, and upload
the current contents of the file as the baseline.  This will
allow me to see the new changes you make.  (I don't want to
review your changes from scratch again.)  Thanks a lot!

[wtc, 2010-07-15 00:56:17]

On 2010/07/04 09:41:30, rsleevi wrote:
>
> The problem is that NSS_CmpCertChainWCANames doesn't normalize names, just
> binary comparison. I'm not sure whether or not this is a bug in NSS (should it
> be using CERT_CompareName as well? or is that just a legacy function we happen
> to use?).

I didn't look into this.  Could you file an NSS bug report on this issue?

Note: NSS is always available (no build flags) on the Mac, so you
can add a file for common code shared between SSLClientSocketMac
and SSLClientSocketNSS.  However, I think we should avoid using NSS
certificate functions on the Mac, if possible.

[Ryan Sleevi, 2010-07-15 13:13:15]

I pulled out the ssl_client_socket_nss changes, and will be posting them as a
separate CL. I've also added a TODO to the Cache, based on your comments, but I
believe it represents a BUG. The bug isn't new to this CL, just not noticed
until now.

http://codereview.chromium.org/2815048/diff/10001/11009
File net/base/ev_root_ca_metadata.h (right):

http://codereview.chromium.org/2815048/diff/10001/11009#newcode51
net/base/ev_root_ca_metadata.h:51: SHA1FingerprintLessThan> PolicyOidMap;
On 2010/07/14 23:31:20, wtc wrote:
> Nit: does this fit on the previous line now?

Unfortunately, no. 86 characters.

http://codereview.chromium.org/2815048/diff/10001/11011
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/2815048/diff/10001/11011#newcode38
net/base/x509_certificate.cc:38: // The cache does not hold a reference to the
certificate objects.  The objects
On 2010/07/14 23:31:20, wtc wrote:
> Should we add a TODO comment to fix this?  Generally it's
> bad to hold a plain, non-reference pointer to a refcounted
> object.

Indeed, upon looking at it again, it would seem there is a data race here that
I've annotated. I'm not sure the best way to fix it however - do you know of any
places in the code where a similar pattern is followed?

http://codereview.chromium.org/2815048/diff/10001/11011#newcode113
net/base/x509_certificate.cc:113: bool
X509Certificate::IsSameOSCert(X509Certificate::OSCertHandle a,
On 2010/07/14 23:31:20, wtc wrote:
> We should not move these two functions.  Moving them make
> your CL larger, and make it hard to verify the only change
> you made is the FingerprintLessThan => SHA1FingerprintLessThan
> renaming.

Sorry. Was trying to appease those with OCD (
http://sites.google.com/a/chromium.org/dev/developers/coding-style ) :) I'll
save the move for later

http://codereview.chromium.org/2815048/diff/10001/11014
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/2815048/diff/10001/11014#newcode710
net/base/x509_certificate_nss.cc:710:
X509Certificate::CalculateFingerprint(root_ca);
On 2010/07/14 23:31:20, wtc wrote:
> Nit: can this fit on the previous line now?

Just at 80. I had left it on the separate line because I know you preferred 78,
though I've corrected it now.

[Ryan Sleevi, 2010-07-15 13:30:12]

On 2010/07/15 13:13:15, rsleevi wrote:
> I pulled out the ssl_client_socket_nss changes, and will be posting them as a
> separate CL. 

Slight correction: These changes are relocated from ssl_client_socket_nss as
part of http://codereview.chromium.org/2934014/show into "x509_util_nss.h" and
.cc, respectively. I'll address your comments there.

[Ryan Sleevi, 2010-07-15 14:23:21]

http://codereview.chromium.org/2815048/diff/10001/11017
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/2815048/diff/10001/11017#newcode413
net/socket/ssl_client_socket_nss.cc:413: // Import into NSS. This is because a
SecCertificateRef always stores it's
On 2010/07/14 23:31:20, wtc wrote:
> Typo: it's => its
> 
> I regret that we have to import the certs into NSS.  I
> think we can just do binary comparison on the Mac if Mac's
> certificate library doesn't support proper name comparison.

I was trying to keep http://codereview.chromium.org/2934014/show smaller, but I
did notice that we can go underneath the SecCertificate API, to CSSM, and
extract the issuer name via CSSMOID_X509V1SubjectNameStd . The problem is that
the current means of doing so is via GetCertFields(), which asks CSSM to:
1) Parse the certificate using NSS
2) Convert every structure of that certificate to Apple's CSSM preferred form

I'd rather just grab a single field from the certificate, but doing so
efficiently would end up introducing a new class or two (to manage "safe" CSSM
wrappers) and modify x509_certificate_mac.cc. I should have remarked as a TODO
the intent to switch to that, as it would eliminate any dependency on
CERTCertificate* for OS X, and have done so in 2934014.

http://codereview.chromium.org/2815048/diff/10001/11017#newcode1532
net/socket/ssl_client_socket_nss.cc:1532:
x509_chain.resize(CFArrayGetCount(os_chain) - 1);
On 2010/07/15 00:41:34, wtc wrote:
> This can be simply
>     x509_chain.resize(chain_size - 1);
> 
> Similarly CFArrayGetCount(os_chain) on the next line can be
> replaced by chain_size.

Fixed in 2934014
> 
> BUG: the second argument to CFRangeMake should be
> chain_size - 1 because it is the length of the range:
>
http://developer.apple.com/mac/library/documentation/CoreFoundation/Reference...
> 
Fixed in 2934014 

> Remove the const_cast on line 1534.
Fixed in 2934014

> 
> BUG: the array index in line 1535 should be 1, not 0.

Not a bug. The intent is that x509_chain (renamed "intermediates") contains just
the intermediate certificates. It should not contain the EE certificate. The
intent is to shift the array
EE -> CA1 -> CA2 -> CA3
into the vector
CA1 -> CA2
if CA3 was the requested issuer certificate

[wtc, 2010-07-15 14:59:28]

LGTM!  Remember to update the CL's description before you
check in.  (The second half of the description describes
changes to ssl_client_socket_nss.cc, which has been moved.)

http://codereview.chromium.org/2815048/diff/10001/11011
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/2815048/diff/10001/11011#newcode113
net/base/x509_certificate.cc:113: bool
X509Certificate::IsSameOSCert(X509Certificate::OSCertHandle a,
On 2010/07/15 13:13:15, rsleevi wrote:
> 
> Sorry. Was trying to appease those with OCD (
> http://sites.google.com/a/chromium.org/dev/developers/coding-style ) :) I'll
> save the move for later

I know that's why you moved the code.

I add new code in the right order, but usually don't fix
old code in the wrong order (to reduce diffs).  In the
future I'll leave this to your discretion.

http://codereview.chromium.org/2815048/diff/10001/11014
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/2815048/diff/10001/11014#newcode710
net/base/x509_certificate_nss.cc:710:
X509Certificate::CalculateFingerprint(root_ca);
Yes, I prefer 78 (so that with the "+-" added by diff,
it's still under 80).  I'm surprised you remember my
preference.  Thanks! :-)

http://codereview.chromium.org/2815048/diff/24001/25011
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/2815048/diff/24001/25011#newcode77
net/base/x509_certificate.cc:77: // TODO(rsleevi): Fix the cache to hold
references, as there is a race where:
I read your description of the race quickly, and I believe
you're correct.  I don't know if we use the same pattern
elsewhere.  Is the bug simply caused by the cache not holding
a reference, or is there some deeper problem with this pattern?

http://codereview.chromium.org/2815048/diff/24001/25011#newcode112
net/base/x509_certificate.cc:112: // A thread-safe cache for X509Certificate
objects.
Remove this block (lines 112-116), which is a duplicate of
the block at lines 72-76 above.

[wtc, 2010-07-15 15:02:19]

http://codereview.chromium.org/2815048/diff/10001/11017
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/2815048/diff/10001/11017#newcode1532
net/socket/ssl_client_socket_nss.cc:1532:
x509_chain.resize(CFArrayGetCount(os_chain) - 1);
Ah, you're right.  The second "BUG" is not a bug.
I mistakenly thought &x509_chain[0] is the input
(the "from" array).  It's actually the output (the
"to" array).

[wtc, 2010-07-15 15:08:23]

http://codereview.chromium.org/2815048/diff/24001/25010
File net/base/x509_cert_types.h (left):

http://codereview.chromium.org/2815048/diff/24001/25010#oldcode40
net/base/x509_cert_types.h:40: 
Nit: I think it's good to keep this blank line to separate
the methods from the data members.

[Ryan Sleevi, 2010-07-15 18:58:32]

Just a quick note over my lunch break on how the change might work. I wanted to
give you a chance to think about it early, since I won't be able to submit a CL
implementing the fix described until late this evening/tomorrow, and I think
it'd be good to hit before M6.

http://codereview.chromium.org/2815048/diff/24001/25011
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/2815048/diff/24001/25011#newcode77
net/base/x509_certificate.cc:77: // TODO(rsleevi): Fix the cache to hold
references, as there is a race where:
On 2010/07/15 14:59:28, wtc wrote:
> I read your description of the race quickly, and I believe
> you're correct.  I don't know if we use the same pattern
> elsewhere.  Is the bug simply caused by the cache not holding
> a reference, or is there some deeper problem with this pattern?

Well, if it wasn't for the |lock_|, we'd be in much bigger trouble. I don't
think using a scoped_refptr<> here would work (at least not cleanly), because we
want to remove from the cache once we have no more references (eg: in dtor)

The thought for how this pattern might resolve itself would be:
1) During Find()
 a) call ->AddRef() on the pointer
 b) Check if HasOneRef() returns true.

If HasOneRef() returns true, it means we're the only one keeping the object
alive. If that's the case, it means the 'other' holder of the X509Certificate
has already called Release(), and is either about to enter the X509Certificate
dtor() (and block on |lock_|), or is already in the dtor (having decremented
ref_count to 0) and is currently blocked on |lock_|. In such a case, Find()
should return NULL, because the object in the cache is about to disappear.

If HasOneRef() is false, it means we've safely incremented the ref-count from
(minimally) 1 to 2, which means that if the other thread were to release their
object right now, it would not dtor, because Release() will see that ref_count_
== 1 once decremented. 

2) Modify CreateFromHandle() to call ->Release() after Find() returns a valid
object, to balance the ->AddRef() of Find().

3) Modify Remove() to compare the fingerprint (or, in my subsequent CL, the
fingerprint+ intermediates) AND the pointer of the X509Certificate, and only
remove if both are the same. If they're different, than the cert being removed
was already ejected from the cache. This is an optimization to handle a scenario
I'll explain shortly.

This should create the following call scenario:

Thread A: Calls Release(), which decrements-and-returns ref_count_ at 0
Thread B: Calls Find(), which acquires |lock_|
Thread A: X509Certificate dtor invokes, and calls Remove(), which blocks at
|lock_| (but the pointer, and the base RefCountedThreadSafe object, have not
completed dtor - rather, they're in dtor)
Thread B: Finds the certificate, and call AddRef(). HasOneRef returns true,
which means the object should be considered 'dead'. As such, calls ->Release()
and returns NULL, releasing |lock_|
Thread A: Remove() acquires lock, finding the certificate and ejects it from the
cache. 
Thread B: Find() returned NULL, so creates a new certificate, and calls
Insert(). Insert() acquires |lock_| and inserts the new certificate - whose
ref_count_ is 0.

The scenario I mentioned in #3 is this


Thread A: Calls Release(), which decrements-and-returns ref_count_ at 0
Thread B: Calls Find(), which acquires |lock_|
Thread A: X509Certificate dtor invokes, and calls Remove(), which blocks at
|lock_| (but the pointer, and the base RefCountedThreadSafe object, have not
completed dtor - rather, they're in dtor)
Thread B: Finds the certificate, and call AddRef(). HasOneRef returns true,
which means the object should be considered 'dead'. As such, calls ->Release()
and returns NULL, releasing |lock_|.
Thread B: Continues execution (the OS does not perform a thread context-switch
right away), creating a new certificate (because Find() returned NULL), and
calls ->Insert()
Thread B: Insert acquires |lock_|, and replaces the dead pointer with a pointer
to the newly created certificate.
Thread A: Remove() acquires lock. While it finds a certificate with the same
fingerprint, the pointers are different, because Thread B ejected it from the
cache. As such, it does not erase the item from the cache, simply returning.


The worst-case of this is when two callers both try to create the same
X509Certificate.
Thread A: Calls Find(), which acquires |lock_|
Thread A: Does not find the certificate in the cache, returns NULL (releasing
|lock_|
Thread A: Creates a new certificate, calls Insert(), which acquires |lock_|
Thread A: Insert updates the map, returns, releasing |lock_|
Thread A: Paused by OS (context switch)
Thread B: Calls Find(), which acquires |lock_|
Thread B: Finds the certificate, calls ->AddRef(). HasOneRef() returns true
(because Thread A has not yet stored in a scoped_refptr<>, which is what
performs the first ->AddRef())
Thread B: Calls ->Release() (balancing the ref_count_ to 0 for the original
cert), returns NULL
Thread B: Creates a new certificate, calls Insert(), which acquires |lock_|
Thread B: Ejects Thread A's certificate from the cache, stores Thread Bs,
returns, releasing |lock_|
Thread A: Returns from CreateFromHandle(). Caller stores in scoped_refptr<>,
which calls ->AddRef(), putting ref_count_ at 1
Thread B: Returns from CreateFromHandle(). Caller stores in a scoped_refptr<>,
which calls ->AddRef(), putting ref_count_ at 1 (because Thread B's certificate
is different from Thread A's)

In short, this change would force a cache miss if a certificate has not yet been
stored in a scoped_refptr. Personally, I think this would be an edge case,
seeing how it's used in the code, so I would't be too worried about the
performance impact, compared to the trade-off gained by truly making it
thread-safe.

Given this thread-safety complexity, I'm certainly curious if this was related
to the Verify() crashes on Mac. My investigations into the SecCertificate() and
CSSM/CDSA implementation of Apple, for both 10.5.7 and 10.6.4, show appropriate
thread-safety throughout. In addition, the Security API is all guarded by a
giant API-level lock, so there would never be two parallel executions of
SecTrustEvaluate(). The two crash signatures posted related to it (
http://crbug.com/30001 and http://crbug.com/40606 ) both suggest, to me, that
the SecCertificateRef being added/verified had already been released, 
consistent with the dtor() of X509Certificate having already executed before
reaching this point.  While I understandably don't have access to the Chrome
crashdump server (nor is that statement meant to imply I'd want it), I was
hoping you might look at some of the historical crashes, if you still have them,
and see if my findings are consistent with that theory.

Either way, I'll be filing a separate bug and CL to address this particular
issue, since I'd like to get the fix in soon (is it too late for M6?), but I
wanted to give you time to review the proposed changes first.

[davidben, 2010-07-15 23:39:39]

On 2010/07/15 18:58:32, rsleevi wrote:
> My investigations into the SecCertificate() and
> CSSM/CDSA implementation of Apple, for both 10.5.7 and 10.6.4, show
appropriate
> thread-safety throughout. In addition, the Security API is all guarded by a
> giant API-level lock, so there would never be two parallel executions of
> SecTrustEvaluate().

Hrm. So, I looked into this briefly earlier, when multi-threaded key generation
gave intermittent crashes on OS X. Protecting SecKeyCreatePair appeared to solve
the problems. I glanced over parts of the CSSM wrappers Apple used, but
admittedly my investigations weren't very thorough. I don't think I noticed a
global lock or really any particular care being taken for thread-safety in their
DbImpl and friends. (But certainly if code assumed a global lock which I missed,
that would take care of it.) Where does this global lock live?

Apple's documentation does say that

"""
On Mac OS X v10.6, some functions can block while waiting for input from the
user (for example, when the user is asked to unlock a keychain or give
permission to change trust settings). In general, it is safe to use the
functions in this API from threads other than your main thread, but you should
avoid calling the function from multiple operations, work queues, or threads
concurrently. Instead, function calls should be serialized (or confined to a
single thread) to prevent any potential problems. Exceptions are noted in the
discussions of the relevant functions.
"""

which is somewhat vague, but it does suggest that there is some problem with
threads to be wary of.

[Ryan Sleevi, 2010-07-16 00:13:46]

On 2010/07/15 23:39:39, David Benjamin wrote:
> On 2010/07/15 18:58:32, rsleevi wrote:
> > My investigations into the SecCertificate() and
> > CSSM/CDSA implementation of Apple, for both 10.5.7 and 10.6.4, show
> appropriate
> > thread-safety throughout. In addition, the Security API is all guarded by a
> > giant API-level lock, so there would never be two parallel executions of
> > SecTrustEvaluate().
> 
> Hrm. So, I looked into this briefly earlier, when multi-threaded key
generation
> gave intermittent crashes on OS X. Protecting SecKeyCreatePair appeared to
solve
> the problems. I glanced over parts of the CSSM wrappers Apple used, but
> admittedly my investigations weren't very thorough. I don't think I noticed a
> global lock or really any particular care being taken for thread-safety in
their
> DbImpl and friends. (But certainly if code assumed a global lock which I
missed,
> that would take care of it.) Where does this global lock live?

Look for #define BEGIN_SECAPI
for libsecurity_keychain_36620 (10.5.8), it's in SecBridge.h, and it starts on
line 47 with:
#define BEGIN_SECAPI \
    OSStatus __secapiresult; \
	try { \
		StLock<Mutex> stAPILock(globals().apiLock);

Looking back at the SecCertificateRefs internal implementation however, in
10.5.7 there are no locks - it was only 10.6.x (mislabeled my folders). A prime
example is Certificate::data() - 10.5.7 is unlocked, while in 10.6.4, it's
guarded by StLock<Mutex> _(mMutex). data() is called by SecTrustEvaluate (see
Trust.cpp@36620:183), so there could be a race if two SecTrustEvaluate() calls
were happening - but again, SecTrustEvaluate is also guarded by the global
SECAPI lock, so that shouldn't be a problem. You see a lot of calls in the
libsecurity_keychain working around the API lock, so I'm reasonably certain it's
not re-entrant. Look for calls such as BEGIN_SECAPI_INTERNAL_CALL, which match a
locked-API call to the unlocked-implementation, to see such situations.

Further, if you look at the underlying implementation of an call such as
Certificate::handle() (SecCertificateGetClHandle) which is CSSM_CL_CertCache in
libsecurity_apple_x509_cl, you'll see that more locks. Look at
AppleX509CLSession (the CSSM_CL_HANDLE is a pointer to such an object, returned
by the CSSM attach routines), and you'll see that the cacheMap and queryMap are
bothed LockedMaps, which also use StLock<Mutex> as the pattern to protect their
accesses. As such, CertCache will be properly serialized.

[Ryan Sleevi, 2010-07-16 00:28:18]

FWIW, though I mention 10.5.7/10.5.8, the BEGIN_SECAPI goes back to 10.1 (see
http://www.opensource.apple.com/source/Security/Security-28/Keychain/SecKeych...
)

[davidben, 2010-07-16 00:35:00]

On 2010/07/16 00:13:46, rsleevi wrote:
> Look for #define BEGIN_SECAPI
> for libsecurity_keychain_36620 (10.5.8), it's in SecBridge.h, and it starts on
> line 47 with:
> #define BEGIN_SECAPI \
>     OSStatus __secapiresult; \
> 	try { \
> 		StLock<Mutex> stAPILock(globals().apiLock);
> 

Huh. That's interesting... that line is missing from libsecurity_keychain_38504
(10.6.4). There, the macro is just

#define BEGIN_SECAPI \
    OSStatus __secapiresult; \
	try { globals().storageManager.cleanup();

StorageManager::cleanup does internally grab a lock, but it releases it before
returning.

This could explain why I never saw the threading test fail on 10.5, but it did
on a 10.6 bot.

Perhaps they tried to remove the global lock in favor of more fine-grained locks
in 10.6, hence the 10.6-only locks you mention.

[davidben, 2010-07-16 00:38:22]

On 2010/07/16 00:35:00, David Benjamin wrote:
> Perhaps they tried to remove the global lock in favor of more fine-grained
locks
> in 10.6, hence the 10.6-only locks you mention.

(I was primarily looking at 10.6 code, since that's where we saw the failures in
KeygenHandlerTest.ConcurrencyTest.)

Also curious is that their warning in the documentation very explicitly mentions
10.6, rather than OS X in general. (The passage above refers to the iPhone,
where the APIs are supposedly all safe.)

[Ryan Sleevi, 2010-07-16 00:51:11]

On 2010/07/16 00:38:22, David Benjamin wrote:
> On 2010/07/16 00:35:00, David Benjamin wrote:
> > Perhaps they tried to remove the global lock in favor of more fine-grained
> locks
> > in 10.6, hence the 10.6-only locks you mention.
That may be. They definitely have finer grained locks in 10.6 - eg:
http://www.opensource.apple.com/source/libsecurity_keychain/libsecurity_keych...

They moved data()'s work into ItemImpl, and now certHandle() is locked via
Certificate's mMutex. The mMutex comes from

http://www.opensource.apple.com/source/libsecurity_keychain/libsecurity_keych...

Which is the basis for keychain items (SecCertificateRef, SecKeyRef), although
it looks like the lock is unused for SecKeyRefs. I admit, my look at locks only
focused on the Certificate/Trust portion of the Security API, since I was
focused on the verifier lock. Both stack traces (from 30001 and from 40606)
pointed to variable corruption. 40606, which was from 10.5.8, even shows the
call to SecCertificateGetData blocked on the API lock (Thread 6), because it was
held by the SecTrustEvaluate (Thread 20), that's why I suggested something with
the Cache possibly being related.

[wtc, 2010-07-16 19:23:32]

http://codereview.chromium.org/2815048/diff/24001/25011
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/2815048/diff/24001/25011#newcode77
net/base/x509_certificate.cc:77: // TODO(rsleevi): Fix the cache to hold
references, as there is a race where:
rsleevi: even with two shots of coffee, I still couldn't
achieve the concentration to finish reading your long
comment.  So I decided to give up and just review your fix
in http://codereview.chromium.org/2836055/show.

I believe I still gained a full understanding of the bug and
the difficulty of fixing it.