Closed Bug 337110 Opened 15 years ago Closed 15 years ago

Coverity OOM Crash and memory leak [@ PK11_CreatePBEParams]

Categories

(NSS :: Libraries, defect, P2)

3.11
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.11.2

People

(Reporter: timeless, Assigned: alvolkov.bgs)

References

()

Details

(Keywords: coverity, crash, memory-leak)

Crash Data

Attachments

(2 files, 2 obsolete files)

 
Assignee: nobody → alexei.volkov.bugs
Keywords: coverity
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → 3.11.2
Also fixed mem leak by replacing SECITEM_AllocItem with PORT_ZAlloc and fixed missuse(and therefore mem leak) of PORT_ZFree.
Attachment #222757 - Flags: review?(nelson)
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.
Attachment #222757 - Flags: review?(nelson) → review+
Since bug 320336 is fixed, please review new patch that uses SECITEM_AllocItem
Attachment #225313 - Flags: review?(nelson)
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
Attachment #225313 - Flags: review?(nelson) → review+
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
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Can not find CID for this bug.
Status: RESOLVED → VERIFIED
Blocks: 341455
This patch caused some bad problems.  
Status: VERIFIED → REOPENED
Keywords: mlk
Resolution: FIXED → ---
Summary: OOM Crash [@ PK11_CreatePBEParams] → OOM Crash and memory leak [@ PK11_CreatePBEParams]
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.
Attached patch patch v3 (obsolete) — Splinter Review
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.
Attachment #225313 - Attachment is obsolete: true
Attachment #225525 - Flags: superreview?(julien.pierre.bugs)
Attachment #225525 - Flags: review?(alexei.volkov.bugs)
All.sh passes on my WinXP PC with this patch applied.  Please review.
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.
Attachment #225525 - Flags: review?(alexei.volkov.bugs) → review+
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.
Attachment #225525 - Flags: review+ → review-
Attachment #225525 - Flags: superreview?(julien.pierre.bugs)
Attached patch patch v4Splinter Review
Attachment #225525 - Attachment is obsolete: true
Attachment #225619 - Flags: superreview?(nelson)
Attachment #225619 - Flags: review?(julien.pierre.bugs)
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 ;)
Attachment #225619 - Flags: review?(julien.pierre.bugs) → review+
Comment on attachment 225619 [details] [diff] [review]
patch v4

r=nelson
Attachment #225619 - Flags: superreview?(nelson) → review+
i didn't file this bug w/ coverity keyword which should mean i found it on my own.
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
tip:
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pbe.c,v  <--  pk11pbe.c
new revision: 1.14; previous revision: 1.13
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.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Summary: OOM Crash and memory leak [@ PK11_CreatePBEParams] → Coverity OOM Crash and memory leak [@ PK11_CreatePBEParams]
Crash Signature: [@ PK11_CreatePBEParams]
You need to log in before you can comment on or make changes to this bug.