147794 - PK11_ImportDERPrivateKeyInfoAndReturnKey frees the private key incorrectly

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Jamie Nicolson]

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]

Attachment: proposed patch--destroy the PKI properly

Comment 2

[Jamie Nicolson]

Target 3.4.2.

Comment 3

[Jamie Nicolson]

Attachment: new and improved

This patch is a little cleaner.

Comment 4

[Jamie Nicolson]

Wan-Teh: is it ok to check this in?

Comment 5

[Wan-Teh Chang]

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]

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]

Attachment: 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]

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]

Comment on attachment 85851 [details] [diff] [review]
ArenaZAlloc -> ArenaZNew

r=wtc.

Please also check it into the NSS_3_5_BRANCH.
Thanks.