Last Comment Bug 49477 - CERT_OpenCertDB() leaks filename
: CERT_OpenCertDB() leaks filename
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: unspecified
: All All
: P3 normal (vote)
: 3.1
Assigned To: Ian McGreer
: Sonja Mirtitsch
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2000-08-18 11:53 PDT by John G. Myers
Modified: 2000-10-02 18:42 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed patch. (880 bytes, patch)
2000-09-30 23:39 PDT, Wan-Teh Chang
no flags Details | Diff | Splinter Review
Revised patch. certDBFilenameCallback needs to strdup the filename string. (1.00 KB, patch)
2000-10-01 10:24 PDT, Wan-Teh Chang
no flags Details | Diff | Splinter Review

Description John G. Myers 2000-08-18 11:53:54 PDT
CERT_OpenCertDB() leaks the filename returned from the filename callback.
Comment 1 Wan-Teh Chang 2000-09-18 15:36:54 PDT
Ian, is there a duplicate for this bug?
Comment 2 Wan-Teh Chang 2000-09-30 23:39:24 PDT
Created attachment 15921 [details] [diff] [review]
Proposed patch.
Comment 3 Wan-Teh Chang 2000-10-01 10:24:03 PDT
Created attachment 15933 [details] [diff] [review]
Revised patch. certDBFilenameCallback needs to strdup the filename string.
Comment 4 Wan-Teh Chang 2000-10-01 10:29:27 PDT
This bug is similar to, but not a duplicate of, bug #39476.

Comments on the revised patch (id=15933):
1. SEC_OpenPermCertDB (called by CERT_OpenCertDB) should
   free certdbname before it returns.
2. certDBFilenameCallback needs to duplicate the filename
   string so that its return value can be freed by
   SEC_OpenPermCertDB.

Please review the patch.  Thanks.
Comment 5 Ian McGreer 2000-10-02 10:14:52 PDT
this looks fine to me.  Thanks Wan-Teh.
Comment 6 Wan-Teh Chang 2000-10-02 16:24:31 PDT
I checked in the fix on the tip.
/cvsroot/mozilla/security/nss/lib/certdb/pcertdb.c, revision 1.4
Comment 7 John G. Myers 2000-10-02 16:26:51 PDT
test_cdb_name_cb() in mailnews/mime/src/mimefilt.cpp needs to be changed to 
return the cert filename in malloced memory.

Same thing for CertDBFilenameCallback() in security/nss/lib/certdb/pcertdb.c
Comment 8 Wan-Teh Chang 2000-10-02 16:41:32 PDT
mailnews/mime/src/mimefilt.cpp is not part of NSS.

My checkin already included the change you suggested
for certDBFilenameCallback() in
security/nss/lib/certdb/pcertdb.c.
Comment 9 Wan-Teh Chang 2000-10-02 16:46:44 PDT
test_kdb_name_cb() in mailnews/mime/src/mimefilt.cpp
also needs the same change.
Comment 10 Nelson Bolyard (seldom reads bugmail) 2000-10-02 16:55:39 PDT
Do any server products that use NSS also need similar changes?
Comment 11 John G. Myers 2000-10-02 17:08:48 PDT
Possibly.  This needs to be mentioned in the release notes.
Comment 12 Wan-Teh Chang 2000-10-02 17:25:30 PDT
Are CERT_OpenCertDB() and SECKEY_OpenKeyDB() typically
only called once in an application session?   If so, I'd
like to propose that we back out the PORT_Free() calls
from these two functions so that we do not break existing
filename callback functions that do not return their results
in malloc'ed memory.

On the other hand, since NSS 3.1 is likely to be the first
3.x release that most products will pick up, it might be
sufficient to mention this change in the release notes.
Comment 13 John G. Myers 2000-10-02 18:09:09 PDT
SECKEY_OpenKeyDB() already had a PORT_Free() call for the failure case, so any 
existing filename callback function for that function which did not return its 
results in malloc'ed memory is already broken.

All of the CERT_OpenCertDB() calls from adminsdk use functions which return 
malloc'ed memory.
Comment 14 Wan-Teh Chang 2000-10-02 18:35:24 PDT
> SECKEY_OpenKeyDB() already had a PORT_Free() call for
> the failure case, so any existing filename callback function
> for that function which did not return its results in
> malloc'ed memory is already broken.

Is it possible that the failure case code has never been exercised?
Comment 15 John G. Myers 2000-10-02 18:42:06 PDT
No, because the server I work on ran into it extremely early.

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