Closed Bug 507422 Opened 11 years ago Closed 11 years ago

crash [@ PORT_FreeArena - lg_mkSecretKeyRep] when PORT_NewArena fails

Categories

(NSS :: Libraries, defect, P2, critical)

defect

Tracking

(Not tracked)

RESOLVED FIXED
3.12.4

People

(Reporter: timeless, Assigned: nelson)

References

()

Details

(Keywords: coverity, crash)

Crash Data

Attachments

(1 file)

786 static NSSLOWKEYPrivateKey *lg_mkSecretKeyRep(const CK_ATTRIBUTE *templ,
799     arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
800     if (arena == NULL) { crv = CKR_HOST_MEMORY; goto loser; }

867 loser:
868     if (crv != CKR_OK) {
869         PORT_FreeArena(arena,PR_FALSE);
The right fix for this bug is to modify PORT_FreeArena so that it 
checks its first argument for NULL and simply returns if so.
All the other PORT_Free* functions do that.   I suspect the code
shown above was written by someone who assumes that PORT_FreeArena
had the same behavior as the rest of the PORT_Free functions, and
IMO, that is a reasonable assumption.  We should make it so,
rather than changing all the callers.

Also, changing PORT_FreeArena avoids any complications with FIPS validation.
changing lg_mkSecretKeyRep would cause those very complications.
Bob, 
Do you agree in principle with my assessment and proposed fix?
Please let me know with your review grade.
Assignee: nobody → nelson
Status: NEW → ASSIGNED
Attachment #391660 - Flags: review?(rrelyea)
Priority: -- → P2
Target Milestone: --- → 3.12.4
Comment on attachment 391660 [details] [diff] [review]
Patch v1 for NSS Trunk

r+ rrelyea

This is safe, and will handle the several cases in NSS where we may not have checked arena is NULL in the error case.

bob
Attachment #391660 - Flags: review?(rrelyea) → review+
Checking in secport.c; new revision: 1.24; previous revision: 1.23
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Crash Signature: [@ PORT_FreeArena - lg_mkSecretKeyRep]
You need to log in before you can comment on or make changes to this bug.