1017826 - newEntry in OCSPCache::Put could leak if its Init fails (i.e. if hashing fails)

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler]]

266  Entry* newEntry = new Entry();
...
274  SECStatus rv = newEntry->Init(aCert, aIssuerCert, aErrorCode, aThisUpdate,
275                                aValidThrough);
276  if (rv != SECSuccess) {
277    return rv;
278  }

Init basically involves doing some hashing. In theory this could fail. If it does, newEntry won't get deleted.

Comment 1

[hpathak]

Attachment: Bug1017826_memory_leak_fix.diff

This is throwing an assert when nightly starts. 

[27769] ###!!! ASSERTION: Existing entry in StartupCache.: 'entry == nullptr', file /Users/mozilla/mozilla-central/startupcache/StartupCache.cpp, line 366

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8437046 [details] [diff] [review]
Bug1017826_memory_leak_fix.diff

Review of attachment 8437046 [details] [diff] [review]:
-----------------------------------------------------------------

Don't forget to ask for review on patches that are ready :)
This looks good.
Just a comment about your commit message, however: First, the beginning should look more like "bug 1017826:". Also, it's best to comment on the goal of the patch rather than how it achieves it. So, maybe something like "prevent a potential memory leak in OCSPCache::Put".

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(In reply to hpathak from comment #1)
> Created attachment 8437046 [details] [diff] [review]
> Bug1017826_memory_leak_fix.diff
> 
> This is throwing an assert when nightly starts. 
> 
> [27769] ###!!! ASSERTION: Existing entry in StartupCache.: 'entry ==
> nullptr', file /Users/mozilla/mozilla-central/startupcache/StartupCache.cpp,
> line 366

I'm fairly sure that's unrelated to this change.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Harsh, a try run with "try: -b do -p <some platform> -u xpcshell -t none" should be sufficient to test this out and get it ready to check in.

Comment 5

[hpathak]

Attachment: Bug1017826_memory_leak_fix.diff

https://tbpl.mozilla.org/?tree=Try&rev=cbb957e96911

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a624df7becd2

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a624df7becd2

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler]]

follow-up to fix indentation nit that I missed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2178beb5988

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e2178beb5988