338356 - Coverity Leak in PK11_TokenKeyGenWithFlags (security/nss/lib/pk11wrap/pk11skey.c)

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Kenneth Herron]

This is coverity CID 318. Please see the sample URL.

|pk11_CreateSymKey| normally returns a pointer to allocated memory (via PORT_New). Here, the calls at lines 943 and 947 are leaked if the test at 956 succeeds.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

correction to comment 0:
the SYMKEYS allocated at lines 943 and 947 are leaked if the test at 956
succeeds.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch - alternative 1 of 2

This is one of two alternative patches for this bug.
Bob, please pick one.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch - Alternative 2

This is the second of the proposed alternatives.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

The second alternative is intended to avoid unnecessarily allocating and 
freeing a symkey object before we're sure we've got the right conditions
to use it once created.  

Bob, please vote ith your review. :)

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 224829 [details] [diff] [review]
patch - alternative 1 of 2

I'm withdrawing the review request for this patch,
because I only want to check in the other one.

Comment 6

[Alexei Volkov]

Comment on attachment 224830 [details] [diff] [review]
Patch - Alternative 2

I think it will work.
r=alexei.volkov

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Fix leak and OOM crash.  Bug 338352 and 338356. Coverity.  
r=rrelyea, alexei.volkov

Checking in pk11skey.c; new revision: 1.109;     previous revision: 1.108
Checking in pk11skey.c; new revision: 1.108.2.1; previous revision: 1.108

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

CID 318

Comment 9

[Robert Relyea]

Comment on attachment 224830 [details] [diff] [review]
Patch - Alternative 2

r=rrelyea

Moving the test is safe and the right thing to do.

bob