Closed Bug 1017826 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: keeler, Assigned: hpathak)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID1195361)

Attachments

(1 file, 1 obsolete file)

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.
Keywords: coverity
Whiteboard: CID1195361
Attached patch Bug1017826_memory_leak_fix.diff (obsolete) — Splinter Review
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 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".
Attachment #8437046 - Flags: review+
(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.
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.
Assignee: nobody → hpathak
Flags: needinfo?(hpathak)
https://tbpl.mozilla.org/?tree=Try&rev=cbb957e96911
Attachment #8437046 - Attachment is obsolete: true
Attachment #8441116 - Flags: review+
Flags: needinfo?(hpathak)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a624df7becd2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1026805
You need to log in before you can comment on or make changes to this bug.