337110 - Coverity OOM Crash and memory leak [@ PK11_CreatePBEParams]

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[timeless]

Comment 1

[Alexei Volkov]

Attachment: check value of allocated ptrs

Also fixed mem leak by replacing SECITEM_AllocItem with PORT_ZAlloc and fixed missuse(and therefore mem leak) of PORT_ZFree.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 222757 [details] [diff] [review]
check value of allocated ptrs

r=nelson
Until bug 320336 is fixed, I think this is the only reasonable solution to this problem.

Comment 3

[Alexei Volkov]

Attachment: patch that uses SECITEM_AllocItem

Since bug 320336 is fixed, please review new patch that uses SECITEM_AllocItem

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 225313 [details] [diff] [review]
patch that uses SECITEM_AllocItem

The patch contains:

>+  loser:
>-loser:

Please don't indent the label loser:
Other than that, r=nelsonb

Comment 5

[Alexei Volkov]

tip:
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pbe.c,v  <--  pk11pbe.c
new revision: 1.13; previous revision: 1.12

3.11 branch:
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pbe.c,v  <--  pk11pbe.c
new revision: 1.11.24.2; previous revision: 1.11.24.1

Comment 6

[Alexei Volkov]

Can not find CID for this bug.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

This patch caused some bad problems.  

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

I backed out the above patch, and also Julien's patch for bug 341455, 
on the NSS_3_11_BRANCH only.  
My apologies for not catching the problems in my previous reviews. :(
I will work on a replacement patch for the one above.

At Julien's request, I'm leaving the trunk broken so that we can test
Tinderbox's ability to detect failures like this.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v3

I believe this patch fixes the crashes and/or leaks. 
This patch also plugs the leak reported in bug 341117.

I have not yet tested this patch, but will do so next.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

All.sh passes on my WinXP PC with this patch applied.  Please review.

Comment 11

[Alexei Volkov]

Comment on attachment 225525 [details] [diff] [review]
patch v3

Deallocation of memory blocks that are used for pPassword and pSalt. Pretty bad one.
I'm sorry about this. 

Nelson's patch is good and strait forward.

Comment 12

[Alexei Volkov]

Comment on attachment 225525 [details] [diff] [review]
patch v3

Yet, one more thing was found with the patch.
paramRV->data is allocated with PORT_Alloc. data was not init with zeros.
If pbe_params->pPassword is failed to be allocated, we call pk11_destroy_ck_pbe_params
and since pbe_params->pSalt can have non-zero values, PORT_ZFree will be call to free unallocated memory block.

Comment 13

[Alexei Volkov]

Attachment: patch v4

Comment 14

[Julien Pierre]

Comment on attachment 225619 [details] [diff] [review]
patch v4

This patch looks good. I think is is actually patch v5 if you count the one I put yesterday in bug 341455 ;)

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 225619 [details] [diff] [review]
patch v4

r=nelson

Comment 16

[timeless]

i didn't file this bug w/ coverity keyword which should mean i found it on my own.

Comment 17

[Alexei Volkov]

3.11 branch
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pbe.c,v  <--  pk11pbe.c
new revision: 1.11.24.5; previous revision: 1.11.24.4

Comment 18

[Alexei Volkov]

tip:
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pbe.c,v  <--  pk11pbe.c
new revision: 1.14; previous revision: 1.13

Comment 19

[Julien Pierre]

Thanks for the checkin, Alexei. The tinderbox on Intel just went green ! The others should go green very soon hopefully. So, I'm closing this bug.
Attachment 225313 [details] [diff], which got checked in on tuesday, had the curious side effect of causing an infinite loop on the Linux platforms. It's not immediately obvious to me why. This latest fix should make it go away. But it might still be worth figuring out why once we have more cycles.