Last Comment Bug 39476 - SECKEY_OpenKeyDB inconsistent in freeing memory
: SECKEY_OpenKeyDB inconsistent in freeing memory
Status: VERIFIED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: unspecified
: x86 Linux
: P3 critical (vote)
: 3.1
Assigned To: Terry Hayes
: Bob Lord
Mentors:
: 39481 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2000-05-16 14:18 PDT by Fred Roeber
Modified: 2000-10-01 10:17 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed fix 1 (790 bytes, patch)
2000-08-18 11:44 PDT, John G. Myers
no flags Details | Diff | Splinter Review

Description Fred Roeber 2000-05-16 14:18:22 PDT
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 John G. Myers 2000-08-11 13:47:14 PDT
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 2 John G. Myers 2000-08-11 13:49:13 PDT
*** Bug 39481 has been marked as a duplicate of this bug. ***
Comment 3 John G. Myers 2000-08-18 11:44:51 PDT
Created attachment 13135 [details] [diff] [review]
proposed fix 1
Comment 4 John G. Myers 2000-08-18 11:51:16 PDT
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.
Comment 5 John G. Myers 2000-08-18 11:54:39 PDT
The CERT_OpenCertDB() issue is bug 49477.
Comment 6 Nelson Bolyard (seldom reads bugmail) 2000-08-18 14:30:39 PDT
This bug shold be assigned to either relyea or thayes.
I nominate Terry.
Comment 7 Terry Hayes 2000-09-28 14:36:51 PDT
Suggested patch applied to keydb.c
Comment 8 Wan-Teh Chang 2000-10-01 10:17:23 PDT
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

Note You need to log in before you can comment on or make changes to this bug.