Closed
Bug 39476
Opened 24 years ago
Closed 24 years ago
SECKEY_OpenKeyDB inconsistent in freeing memory
Categories
(NSS :: Libraries, defect, P3)
Tracking
(Not tracked)
VERIFIED
FIXED
3.1
People
(Reporter: bugzilla, Assigned: thayes0993)
References
Details
Attachments
(1 file)
790 bytes,
patch
|
Details | Diff | Splinter Review |
This is a replacement for the internal Netscape bug http://scopus/bugsplat/show_bug.cgi?id=299899 . If I call SECKEY_OpenKeyDBFilename, a pointer to the filename is passed. OpenKeyDB frees this filename. It is not expected behaviour for an input parameter to be freed like this. If you are creating a new database, OpenKeyDB then calls the dbname callback again, which this time returns an empty string (PORT_Free zeroes the memory?). The rest of the function is operating on the wrong file, which causes havoc. Specifically, the global salt value isn't getting set properly. ------- Additional Comments From relyea 07/08/98 10:42 ------- I'm pretty sure this is how the key stuff was supposed to work. I'll assign it to someone who cares and let them work on it as part of the SDK cleanup ------- Additional Comments From nicolson 07/08/98 11:35 ------- The bug might not be in OpenKeyDB, it could be in the interface between OpenKeyDB and OpenKeyDBFilename. But the bottom line is that OpenKeyDBFilename doesn't work if the key database doesn't exist yet and has to be created. ------- Additional Comments From nicolson 08/26/98 23:34 ------- Sorry, but this really is a bug. Anyone who uses SECKEY_OpenKeyDBFilename is going to have an error that is very difficult to track down. Later in their code they will SEGV because their global salt has been corrupted. If they stop in SECKEY_OpenKeyDB and examine the salt, it's fine. This needs to be fixed before more people shoot themselves in the foot. I'm labeling it CRITICAL because it causes a SEGV. This will happen when you use OpenKeyDBFilename if the file doesn't already exist. ------- Additional Comments From nicolson 08/26/98 23:41 ------- Actually, the fix for this is trivial: change keyDBFilenameCallback in keydb.c to return PORT_strdup((char*)arg) instead of just (char*)arg. ------- Additional Comments From nicolson 09/16/98 16:45 ------- The exact same bug (and solution) applies to CERT_OpenCertDBFilename. I just blew several hours tracking down mysterious memory corruption to discover this. Please fix it soon.
Comment 1•24 years ago
|
||
OpenKeyDB needs to either always free the string returned by the callback or never free it. It should not be freeing the string only in the error case--this causes a memory leak in the success case.
Comment 3•24 years ago
|
||
Comment 4•24 years ago
|
||
Assign to nelsonb for review. CERT_OpenCertDB(), on the other hand, always leaks the filename. It should probably be changed to match SECKEY_OpenKeyDB(), but that's a different bug.
Assignee: lord → nelsonb
Summary: SECKEY_OpenKeyDB overzealous in freeing memory → SECKEY_OpenKeyDB inconsistent in freeing memory
Comment 6•24 years ago
|
||
This bug shold be assigned to either relyea or thayes. I nominate Terry.
Assignee: nelsonb → thayes
Assignee | ||
Updated•24 years ago
|
Status: NEW → ASSIGNED
Updated•24 years ago
|
Target Milestone: --- → 3.1
Assignee | ||
Comment 7•24 years ago
|
||
Suggested patch applied to keydb.c
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Comment 8•24 years ago
|
||
I found that the second half of jgmyers' patch was not applied. I just applied it and verified that all of the patch has been checked in. /cvsroot/mozilla/security/nss/lib/softoken/keydb.c, revisions 1.2,1.3
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•