The default bug view has changed. See this FAQ.

PK11_ImportDERPrivateKeyInfoAndReturnKey frees the private key incorrectly

RESOLVED FIXED in 3.4.2

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: Jamie Nicolson, Assigned: Jamie Nicolson)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

15 years ago
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!
(Assignee)

Comment 1

15 years ago
Created attachment 85396 [details] [diff] [review]
proposed patch--destroy the PKI properly
(Assignee)

Comment 2

15 years ago
Target 3.4.2.
Target Milestone: --- → 3.4.2
(Assignee)

Comment 3

15 years ago
Created attachment 85828 [details] [diff] [review]
new and improved

This patch is a little cleaner.
Attachment #85396 - Attachment is obsolete: true

Updated

15 years ago
Priority: -- → P1
(Assignee)

Comment 4

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

Updated

15 years ago
Attachment #85828 - Flags: review+

Comment 5

15 years ago
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.
(Assignee)

Comment 6

15 years ago
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.
(Assignee)

Comment 7

15 years ago
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.
Attachment #85828 - Attachment is obsolete: true
(Assignee)

Comment 8

15 years ago
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
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 9

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

r=wtc.

Please also check it into the NSS_3_5_BRANCH.
Thanks.
Attachment #85851 - Flags: review+
You need to log in before you can comment on or make changes to this bug.