Fix a memory leak in net::CertVerifyProcMac::VerifyInternal

Fix a memory leak in net::CertVerifyProcMac::VerifyInternal

BUG=455533
Committed: https://crrev.com/a52857b59bca6dafed787ea871a0d504816ea565
Cr-Commit-Position: refs/heads/master@{#314931}

[Ryan Sleevi, 2015-02-05 22:02:03]

rsleevi@chromium.org changed reviewers:
+ davidben@chromium.org

[Ryan Sleevi, 2015-02-05 22:02:04]

Assumed I was smart enough to use ScopedCFTypeRef with move semantics. I was
not.

[davidben, 2015-02-05 22:14:35]

lgtm with alternate suggestion, but I'm fine with this version if you think this
is cleaner API-wise.

https://codereview.chromium.org/886933008/diff/1/net/cert/cert_verify_proc_ma...
File net/cert/cert_verify_proc_mac.cc (right):

https://codereview.chromium.org/886933008/diff/1/net/cert/cert_verify_proc_ma...
net/cert/cert_verify_proc_mac.cc:493: ScopedCFTypeRef<CFArrayRef>
original_chain(cert->CreateOSCertChainForCert());
It looks like CreateOSCertChainForCert makes a CFMutableArrayRef anyway (and can
conceivably return NULL but probably takes a malloc error?). Maybe we should
just change the type signature of this function.

https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/x509_cert...

[Ryan Sleevi, 2015-02-05 22:49:40]

https://codereview.chromium.org/886933008/diff/1/net/cert/cert_verify_proc_ma...
File net/cert/cert_verify_proc_mac.cc (right):

https://codereview.chromium.org/886933008/diff/1/net/cert/cert_verify_proc_ma...
net/cert/cert_verify_proc_mac.cc:493: ScopedCFTypeRef<CFArrayRef>
original_chain(cert->CreateOSCertChainForCert());
On 2015/02/05 22:14:34, David Benjamin wrote:
> It looks like CreateOSCertChainForCert makes a CFMutableArrayRef anyway (and
can
> conceivably return NULL but probably takes a malloc error?). Maybe we should
> just change the type signature of this function.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/x509_cert...

I considered that, but we have this same function on windows (which returns a
PCCERT_CONTEXT), and making that ScopedPCCERT_CONTEXT would mean needing to
either include the windows headers or forward declare the free functions.

So for consistency I opted to have both return the native pointer, which feels a
little gross, but less gross than adding more forward declare cruft.

If you feel strongly though, happy to change.

[davidben, 2015-02-05 23:14:24]

On 2015/02/05 22:49:40, Ryan Sleevi wrote:
>
https://codereview.chromium.org/886933008/diff/1/net/cert/cert_verify_proc_ma...
> File net/cert/cert_verify_proc_mac.cc (right):
> 
>
https://codereview.chromium.org/886933008/diff/1/net/cert/cert_verify_proc_ma...
> net/cert/cert_verify_proc_mac.cc:493: ScopedCFTypeRef<CFArrayRef>
> original_chain(cert->CreateOSCertChainForCert());
> On 2015/02/05 22:14:34, David Benjamin wrote:
> > It looks like CreateOSCertChainForCert makes a CFMutableArrayRef anyway (and
> can
> > conceivably return NULL but probably takes a malloc error?). Maybe we should
> > just change the type signature of this function.
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/x509_cert...
> 
> I considered that, but we have this same function on windows (which returns a
> PCCERT_CONTEXT), and making that ScopedPCCERT_CONTEXT would mean needing to
> either include the windows headers or forward declare the free functions.
> 
> So for consistency I opted to have both return the native pointer, which feels
a
> little gross, but less gross than adding more forward declare cruft.
> 
> If you feel strongly though, happy to change.

Sorry, that might not have been clear. I meant change the type signature to
return CFMutableArrayRef (since it already is one) and documenting that it
creates a fresh one (already part of the API contract) that the caller is
allowed to muck with. But I don't feel strongly. Just saves making two of them.

[davidben, 2015-02-05 23:15:22]

On 2015/02/05 23:14:24, David Benjamin wrote:
> Sorry, that might not have been clear. I meant change the type signature to
> return CFMutableArrayRef (since it already is one) and documenting that it
> creates a fresh one (already part of the API contract) that the caller is
> allowed to muck with. But I don't feel strongly. Just saves making two of
them.

Oh, actually, it's not already part of the contract, is it? These things are
reference-counted... anyway, I don't terrible care.

[Ryan Sleevi, 2015-02-05 23:17:04]

On 2015/02/05 23:15:22, David Benjamin wrote:
> On 2015/02/05 23:14:24, David Benjamin wrote:
> > Sorry, that might not have been clear. I meant change the type signature to
> > return CFMutableArrayRef (since it already is one) and documenting that it
> > creates a fresh one (already part of the API contract) that the caller is
> > allowed to muck with.

Oh! Got you! Yeah, that makes total sense.

> > But I don't feel strongly. Just saves making two of them.
> 
> Oh, actually, it's not already part of the contract, is it? 

I'm not sure which "it's" you meant, but it's already allowed to muck with the
result. That's the point of returning it - the OS may muck with things
(certainly on Windows) so we return fresh copies.

I'll make that change and land.

> These things are reference-counted... anyway, I don't terrible care.

[Ryan Sleevi, 2015-02-05 23:39:34]

New patchsets have been uploaded after l-g-t-m from davidben@chromium.org

[Ryan Sleevi, 2015-02-05 23:39:51]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2015-02-05 23:40:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/886933008/20001

[commit-bot: I haz the power, 2015-02-06 01:00:03]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-06 01:01:33]

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