337361 - Leaks in jar_parse_any (security/nss/lib/jar/jarver.c)

Status: RESOLVED FIXED

(NSS :: Tools, defect, P3)

Description

[Kenneth Herron]

This is coverity CIDs 558 and 559. Please refer to the sample URL. An object is allocated at line 440 (inside a loop) and a pointer to it is stored in |met|. At lines 446-450, if the test fails then the next iteration of the loop begins which will overwrite |met| with another allocation.

Alternately, if the test at line 468 *fails*, then execution reaches the end of the loop (line 485) and |met| goes out of scope without being freed. In this case, it also looks like the allocation at line 465 is leaked, and the allocation at line 466 may be leaked if neither of the tests at 479 or 482 succeed.

Comment 1

[Julien Pierre]

Retargetting all P2s to 3.11.3 .

Comment 2

[(no longer active)]

Attachment: Path for memory leaks in jarver.c

This patch fixes the leaks mentioned in comment #0, plus a number of others in the same file (jarver.c).

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 236787 [details] [diff] [review]
Path for memory leaks in jarver.c

r=nelsonb, requesting second review for checkin.

Note that function JAR_open_database merely returns the value of an 
existing pointer, and JAR_close_database is a no-op.  So, inserting
calls to JAR_close_database won't really fix any leaks.  But I agree 
that it is nicer to match opens with closes.

Ehsan,
If you want to continue to cleanup and fix the JAR code, here are a 
few suggestions for subsequent patches.

1. get rid of all blocks of code in libjar sources that are conditionally 
compiled with 
#if 0 
and 
#ifdef notdef 


2. remove the lines that #define USE_MOZ_THREAD and change all the 
code that reads:

> #ifdef USE_MOZ_THREAD
>     do one thing
> #else
>     do another thing
> #endif

to unconditionally compile the path that is not the USE_MOZ_THREAD path,
and get rid of all the code in the USE_MOZ_THREAD path.
That USE_MOZ_THREAD code is a remnant from the days long ago when NSS
was part of Netscape Communicator.  It should no longer be used at all.

Of course, the code will need to be tested thoroughly when that is done.

Comment 4

[(no longer active)]

(In reply to comment #3)
> Note that function JAR_open_database merely returns the value of an 
> existing pointer, and JAR_close_database is a no-op.  So, inserting
> calls to JAR_close_database won't really fix any leaks.  But I agree 
> that it is nicer to match opens with closes.

Yes, it is nicer, and in addition, if some day someone changes these functions in a way that they actually allocate/free some resources, matching calls to these functions pays off.

> Ehsan,
> If you want to continue to cleanup and fix the JAR code, here are a 
> few suggestions for subsequent patches.
> 
> 1. get rid of all blocks of code in libjar sources that are conditionally 
> compiled with 
> #if 0 
> and 
> #ifdef notdef 

Sure.

> 2. remove the lines that #define USE_MOZ_THREAD and change all the 
> code that reads:
> 
> > #ifdef USE_MOZ_THREAD
> >     do one thing
> > #else
> >     do another thing
> > #endif
> 
> to unconditionally compile the path that is not the USE_MOZ_THREAD path,
> and get rid of all the code in the USE_MOZ_THREAD path.
> That USE_MOZ_THREAD code is a remnant from the days long ago when NSS
> was part of Netscape Communicator.  It should no longer be used at all.
> 
> Of course, the code will need to be tested thoroughly when that is done.

OK.  I assume these issues are not already filed as bug reports?  If so, then I can open the bugs myself and submit my patches to the relevant bugs.

Comment 5

[(no longer active)]

(In reply to comment #3)
> 2. remove the lines that #define USE_MOZ_THREAD and change all the 
> code that reads:
> 
> > #ifdef USE_MOZ_THREAD
> >     do one thing
> > #else
> >     do another thing
> > #endif
> 
> to unconditionally compile the path that is not the USE_MOZ_THREAD path,
> and get rid of all the code in the USE_MOZ_THREAD path.
> That USE_MOZ_THREAD code is a remnant from the days long ago when NSS
> was part of Netscape Communicator.  It should no longer be used at all.

Nelson,

Are you sure you're not wrong here? Grepping for USE_MOZ_THREAD reveals this macro used in jarsign.c and jarver.c, and both these files include a "#define USE_MOZ_THREAD" at their beginning.  So, I guess that the current code always used the sections *in* USE_MOZ_THREAD path.

Am I missing something?

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Yes, the code presently DOES use that path.  
Long ago, in the Netscape Communicator browser, there was a special thread,
known as the "main" thread or the "mozilla" thread, and NSS code that used
the cert DB was required to always run on that thread.  But there is no such
special cert DB thread in mozilla any more, and certainly there is no such
thread in the NSS test programs that use libJAR, such as signtool.  

Originally, the USE_MOZ_THREAD code did the same thing as the other path, 
but it first did an RPC call to the moz_thread, and the actual DB call was
done on the moz thread, and the results returned.  But the only difference
between the USE_MOZ_THEAD path and the other path was supposed to be the
thread on which the code ran.  

IFF you find that the two code paths have diverged, and differ in more than
merely the thread on which they run, we should fix that.  In that case,
the one new path should do "the right thing", whatever that is.

Comment 7

[Alexei Volkov]

Comment on attachment 236787 [details] [diff] [review]
Path for memory leaks in jarver.c

still quite a few leaks left in the areas around the fixes, but patch provides resolution for a number of leaks.
r=alexei.volkov

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Fix leaks in jarfile.c (bug 338453), jarjart.c (bug 351408), and
jarver.c (bug 337361). Patch contributed by [e-mail]

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