Last Comment Bug 338453 - Leaks in security/nss/lib/jar/jarfile.c
: Leaks in security/nss/lib/jar/jarfile.c
Status: RESOLVED FIXED
: coverity
Product: NSS
Classification: Components
Component: Tools (show other bugs)
: 3.11
: All All
: P3 normal (vote)
: 3.12
Assigned To: :Ehsan Akhgari
:
Mentors:
http://bonsai.mozilla.org/cvsblame.cg...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-05-18 10:43 PDT by Kenneth Herron
Modified: 2009-02-01 00:32 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Prevent memory leak (2.50 KB, patch)
2006-09-05 00:24 PDT, :Ehsan Akhgari
nelson: review+
Details | Diff | Splinter Review
Prevent more memory leaks in jarfile.c (3.52 KB, patch)
2006-09-05 01:25 PDT, :Ehsan Akhgari
nelson: review+
alvolkov.bgs: review-
Details | Diff | Splinter Review
Revision of attachment #236779 (3.57 KB, patch)
2006-09-05 10:58 PDT, :Ehsan Akhgari
nelson: review+
alvolkov.bgs: superreview+
Details | Diff | Splinter Review

Description Kenneth Herron 2006-05-18 10:43:18 PDT
Please refer to the sample URL. Coverity CID 565 is that the |outbuf| allocation at line 366 is leaked at lines 376 and 391. It also appears to me that line 414 is another leak site, and the |inbuf| allocation at line 363 is also leaked in these cases.

Coverity 563 is that the |outbuf| allocation at line 468 is leaked at line 480. It appears to me that lines 494 and 502 also leak.
Comment 1 :Ehsan Akhgari 2006-09-05 00:24:14 PDT
Created attachment 236777 [details] [diff] [review]
Prevent memory leak

I fixed the leaks in jar_physical_inflate() and jar_inflate_memory().

There are other leaks in this file.  Should I open new bugs and submit patches under those, or fix them all and post a new patch here?
Comment 2 Nelson Bolyard (seldom reads bugmail) 2006-09-05 00:49:45 PDT
Comment on attachment 236777 [details] [diff] [review]
Prevent memory leak

r=nelsonb
Comment 3 Nelson Bolyard (seldom reads bugmail) 2006-09-05 00:54:39 PDT
Ehasn,  Thanks for your patch!

There are other bugs already filed for leaks in lib/jar.
See bug 334914, bug 337354 and bug 337361 

If you find additional leaks in those files, not covered by those bug reports,
please add comments to those bugs documenting the additional leaks.  
Not much point in having multiple bugs filed for leaks in the same source
file.  Feel free to write patches and attach them to any of those bugs. 
You may also "take" those bug (reassign them to yourself).

Checkins of the approved patches must be done by NSS team members, but 
they'll happily give you credit in the checkin messages.  
Comment 4 Nelson Bolyard (seldom reads bugmail) 2006-09-05 00:55:48 PDT
Ehsan,  sorry for mistyping your name.  
Comment 5 :Ehsan Akhgari 2006-09-05 01:25:08 PDT
Created attachment 236779 [details] [diff] [review]
Prevent more memory leaks in jarfile.c

(In reply to comment #3)
> Ehasn,  Thanks for your patch!
> 
> There are other bugs already filed for leaks in lib/jar.
> See bug 334914, bug 337354 and bug 337361 

I'll take a look at those bugs as well.  Thanks!

> If you find additional leaks in those files, not covered by those bug reports,
> please add comments to those bugs documenting the additional leaks.  
> Not much point in having multiple bugs filed for leaks in the same source
> file.  Feel free to write patches and attach them to any of those bugs. 
> You may also "take" those bug (reassign them to yourself).

There is one more leak in jarfile.c, so I'm posting a new attachment which deprecates attachment #236777 [details] [diff] [review].  This new patch includes the previous, as well as this other leak's fix.

(In reply to comment #4)
> Ehsan,  sorry for mistyping your name.  

No worries.  I'm used to the "Eshan" spelling variant as well.  ;-)
Comment 6 Nelson Bolyard (seldom reads bugmail) 2006-09-05 02:25:02 PDT
Comment on attachment 236779 [details] [diff] [review]
Prevent more memory leaks in jarfile.c

r=nelsonb, requesting second review for checkin.
Comment 7 Alexei Volkov 2006-09-05 10:45:01 PDT
Comment on attachment 236779 [details] [diff] [review]
Prevent more memory leaks in jarfile.c

in jar_physical_inflate, JAR_FOPEN opens a file descriptor(jarfile.c:378), and leaks it upon a return from the function with an error.
Comment 8 :Ehsan Akhgari 2006-09-05 10:58:50 PDT
Created attachment 236820 [details] [diff] [review]
Revision of attachment #236779 [details] [diff] [review]

(In reply to comment #7)
> (From update of attachment 236779 [details] [diff] [review] [edit])
> in jar_physical_inflate, JAR_FOPEN opens a file descriptor(jarfile.c:378), and
> leaks it upon a return from the function with an error.

That's right.  Sorry I didn't catch it at first.  This patch fixes this issue.
Comment 9 Nelson Bolyard (seldom reads bugmail) 2006-09-05 12:52:32 PDT
Comment on attachment 236820 [details] [diff] [review]
Revision of attachment #236779 [details] [diff] [review]

Alexei, thanks for catching that FD leak.

r=nelsonb
My review+ signifies that this patch correctly fixes the leaks it is intended to fix, not that if fixes all possible leaks in this code.
Comment 10 Nelson Bolyard (seldom reads bugmail) 2006-09-25 12:22:54 PDT
Fix leaks in jarfile.c (bug 338453), jarjart.c (bug 351408), and
jarver.c (bug 337361). Patch contributed by ehsan.akhgari@gmail.com

Checking in jarfile.c; new revision: 1.6; previous revision: 1.5
Checking in jarjart.c; new revision: 1.7; previous revision: 1.6
Checking in jarver.c;  new revision: 1.12; previous revision: 1.11

Note You need to log in before you can comment on or make changes to this bug.