Last Comment Bug 147794 - PK11_ImportDERPrivateKeyInfoAndReturnKey frees the private key incorrectly
: PK11_ImportDERPrivateKeyInfoAndReturnKey frees the private key incorrectly
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.4.1
: All All
: P1 normal (vote)
: 3.4.2
Assigned To: Jamie Nicolson
: Bishakha Banerjee
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-05-28 23:55 PDT by Jamie Nicolson
Modified: 2002-05-31 17:55 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed patch--destroy the PKI properly (750 bytes, patch)
2002-05-28 23:56 PDT, Jamie Nicolson
no flags Details | Diff | Splinter Review
new and improved (971 bytes, patch)
2002-05-31 15:01 PDT, Jamie Nicolson
wtc: review+
Details | Diff | Splinter Review
ArenaZAlloc -> ArenaZNew (1007 bytes, patch)
2002-05-31 17:48 PDT, Jamie Nicolson
wtc: review+
Details | Diff | Splinter Review

Description Jamie Nicolson 2002-05-28 23:55:54 PDT
Two things:

1. It calls SECKEY_DestroyPrivateKeyInfo with freeit==PR_TRUE. This will free
the arena...but then we turn around and free the arena a second time.

2. It doesn't set the arena field of the PrivateKeyInfo, so
DestroyPrivateKeyInfo will still try to call free() on the individual secitems
in the PKI. This doesn't fail immediately, but causes the memory allocator to
become unstable and fail later--Yuck!
Comment 1 Jamie Nicolson 2002-05-28 23:56:50 PDT
Created attachment 85396 [details] [diff] [review]
proposed patch--destroy the PKI properly
Comment 2 Jamie Nicolson 2002-05-29 13:49:57 PDT
Target 3.4.2.
Comment 3 Jamie Nicolson 2002-05-31 15:01:37 PDT
Created attachment 85828 [details] [diff] [review]
new and improved

This patch is a little cleaner.
Comment 4 Jamie Nicolson 2002-05-31 17:08:33 PDT
Wan-Teh: is it ok to check this in?
Comment 5 Wan-Teh Chang 2002-05-31 17:11:50 PDT
Comment on attachment 85828 [details] [diff] [review]
new and improved

r=wtc.

>     temparena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
>-    pki = PORT_ZNew(SECKEYPrivateKeyInfo);
>+    pki = PORT_ArenaZAlloc(SECKEYPrivateKeyInfo);
>+    pki->arena = temparena;
>+    temparena = NULL;

The statement "temparena = NULL;" is not necessary.
Comment 6 Jamie Nicolson 2002-05-31 17:39:31 PDT
Agreed. I put it there as more of a reminder that temparena should not be freed.
But I suppose it clutters up the code so I'll take it out.
Comment 7 Jamie Nicolson 2002-05-31 17:48:02 PDT
Created attachment 85851 [details] [diff] [review]
ArenaZAlloc -> ArenaZNew

The last patch was broken. It called ArenaZAlloc instead of ArenaZNew, and
didn't pass in the arena. The compiler found this problem.
Comment 8 Jamie Nicolson 2002-05-31 17:51:56 PDT
Checked in to NSS_3_4_BRANCH (for 3.4.2) and trunk (for 3.6 and beyond).

/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pk12.c,v  <--  pk11pk12.c
new revision: 1.5.8.1; previous revision: 1.5

/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pk12.c,v  <--  pk11pk12.c
new revision: 1.6; previous revision: 1.5
Comment 9 Wan-Teh Chang 2002-05-31 17:55:04 PDT
Comment on attachment 85851 [details] [diff] [review]
ArenaZAlloc -> ArenaZNew

r=wtc.

Please also check it into the NSS_3_5_BRANCH.
Thanks.

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