337354 - Leaks in JAR_cert_attribute (security/nss/lib/jar/jarver.c)

Status: RESOLVED FIXED

(NSS :: Tools, defect, P3)

Description

[Kenneth Herron]

Please refer to the sample URL. The functions |CERT_GetCommonName|, |CERT_GetCertEmailAddress|, |CERT_GetOrgName|, and |CERT_GetCountryName| all return pointers to allocated memory. There is a group of calls to these functions at lines 1135-1141 which is leaked by the return at line 1166, and another group at 1184-1190 which is leaked at 1215.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

This large duplicated block of code should be put into a function and
called from two places.

There is a crash in this block of code, if the PORT_ZAlloc call returns NULL.
That should be fixed too.

Comment 2

[Kenneth Herron]

This is arguably a separate bug, but it's another leak a few lines later in the same function. See <http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/jar/jarver.c&rev=1.11&mark=1230-1233,1253,1256#1230>. This is coverity CID 553.

At line 1232, |jar_choose_nickname| returns either NULL or a pointer to allocated memory containing a string. At line 1253 the string returned by |jar_choose_nickname| is copied and the copy is returned to the caller. The original string returned by |jar_choose_nickname| is never freed.

Comment 3

[Kenneth Herron]

Coverity CID 551 is a variant of CID 553. See <http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/jar/jarver.c&rev=1.11&mark=1222,1232,1244,1253,1256#1220>
The calls to |CERT_Hexify| at lines 1222 and 1244 also return allocated memory, which is copied and leaked when the function returns.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

This bug now represents all these different coverity CIDs:
549 551 553 558 907 908 909

We should be sure we've addressed all those CIDs before marking it fixed.
Taking.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

*** Bug 334913 has been marked as a duplicate of this bug. ***

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

By FAR, the one file of code in NSS with which coverity is MOST unhappy 
is in lib/jar, and specifically lib/jar/jarver.c.  
This is 8+ year old code that people have been afraid to touch for years
for fear it would stick to them.  Well, I'm gonna touch it.  

I'm gonna consolidate all the jar memory leak bugs into this one.
The patch is gonna be HUGE, and probably will take several passes before
coverity blesses it as being truly free of leaks.  

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Attachment: cleanup memory mgmt in libjar, v1

This patch does these things:
a) changes macro ADDITEM to take another argument, which is the statement it 
should execute if it fails.  Previously that statement was hardcoded as 
return err;  Perhaps I should just get rid of this macro alltogether?

b) gets rid of lots of dead code, both #if 0 and #ifdef WIN16
eliminates some old macros that were only used with WIN16 (xp_HUGExxx)

c) tries to get cert ref counting right.  Previous code seemed to treat 
certs as uncounted objects.

d) factors out a large block of duplicated code from two cases in 
JAR_cert_attribute, and puts one copy of that code into a new function 
named jar_compose_name_string.  Plugs leaks in that one copy.

e) factors out a large body of code from inside a loop that walks down a 
list, processing (destroying) the members.  that inner code knows all about
the private details of the members, including what hidden inner components
need to be freed.  Now it's in a new function named jar_destroy_list_item_data, which can be called from various places that need to free one of those things.

f) plug LOTS of leaks.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Next I need to figure out what, if any, libjar test tools we have ...

Comment 9

[Alexei Volkov]

Comment on attachment 223264 [details] [diff] [review]
cleanup memory mgmt in libjar, v1

Here are the things I've found within the files you've touched in this patch. Functions marked with !are needed to be fixed:


function JAR_stash_cert:
  In case when JAR_open_database() fails, we return without freeing cert.
  In case "newcert && newcert->isperm" (10 lines lower) nickename should be freed as well if it is not null.

function jar_inflate_memory leaks outbuf.

function jar_physical_inflate leaks inbuf, outbuf upon  errors.

function  *jar_choose_nickname:
cert_o and cert_cn_o are leaked

! function JAR_cert_html:
   leaks cert

! function JAR_fetch_cert leaks cert.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Alexei, your review findings are all correct.  
You found problems in two functions I had modified, and 
problems in 4 more functions I had not yet modified.

But, I am reducing the priority on this bug to P3, maybe it should be even 
lower.  Although lib/jar is part of nss/lib, it really belongs in nss/cmd.
It is not used in NSS anywhere except in modutil and signtool.  
It does not appear to be used by PSM at all (or so lxr suggests).
I suspect much of it is dead code.

So, I don't want to spend any more time on it until we've got the coverity 
bugs out of the shared library code we actually SHIP.

Comment 11

[Robert Relyea]

If mozilla doesn't call it. I'm all for moving it to command.

bob

Comment 12

[Kai Engert (:KaiE:)]

psm does not call JAR_cert_attribute

Comment 13

[Robert Relyea]

does psm call and libjar functions kai?

Comment 14

[Kai Engert (:KaiE:)]

(In reply to comment #13)
> does psm call and libjar functions kai?

I do not see any occurrences of JAR_ in Psm code, does that answer your question?

Comment 15

[Julien Pierre]

Retargetting all P2s to 3.11.3 .

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2

This patch addresses the previous review comments except:
> ! function JAR_fetch_cert leaks cert.
I found that it doesn't leak the cert, but returns it.

This patch is quite different from the previous one because of the 
many other checkins to cleanup libjar recently contributed and 
committed.  I had to manually re-apply parts of the old patch to the 
tree.  It builds and runs (or, it did for me), but our QA testing 
is not very thorough, so it needs to be reviewed carefully.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

remove target milestone, since the target was missed.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 240057 [details] [diff] [review]
patch v2

This review request is over a year old.  I'll ask someone else to review it.
Julien?

Comment 19

[Julien Pierre]

Comment on attachment 240057 [details] [diff] [review]
patch v2

This patch is bit-rotten and no longer applies to the trunk for jarver.c, which is the biggest chunk of it.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Definitely not blocking any mozilla client, since no mozilla client uses it.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Coverity now reports 5 issues in lib/jar in Firefox run 278.

CID 1355 - leaks in JAR_cert_attribute

Allocated value stored in variable "ret" at
1199 	    case jarCertSerial:
1201 	      ret = CERT_Hexify (&cert->serialNumber, 1);
Is not freed.

Likewise, in 
1214 	    case jarCertFinger:
the value allocated in this call
1223 	        ret = CERT_Hexify (&hexme, 1);
is not freed.

====================================================================
CID 1356 - another leak

Likewise in 
1209 	    case jarCertNickname:
the value allocate in this call
1211 	      ret = jar_choose_nickname (cert);
is not freed. 

======================================================================
CID 1381 - NULL check after pointer dereference

In function JAR_stash_cert, pointer "nickname" is dereferenced without 
checking it for NULL first, and then is checked for NULL later.

1406 	  nickname = jar_choose_nickname (cert);
1408 	  newcert = CERT_FindCertByNickname (certdb, nickname);

1424 	  if (nickname != NULL)

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

CID 1357 - memory leak

In function JAR_JAR_list_certs, Coverity claims that the block allocated
for "ugly_list"

120  	  ugly_list = (char*)PORT_ZAlloc (16);

is leaked in some cases.

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

In the year 2009, well after this Coverity test was run, NSS's libJAR was
seriously overhauled to fix MANY bugs.  Consequently, it is possible that
some of these Coverity CIDs are no longer valid, and that other new ones 
may now exist.  For example, function JAR_JAR_list_certs, mentioned in 
comment 22, no longer exists (according to MXR), and function JAR_stash_certs
is dead code (has no callers, according to MXR).

Timeless, could you get us a current list of Coverity's complaints for libJAR?

Comment 24

[timeless]

yeah. i'm running through coverity _now_.

Comment 25

[timeless]

This is the list of 'uninspected' items, anything else in my database _should_ have been filed by me.

 	CID 	Checker	Classification	Owner	Severity	Action	Function	File	Final Event	Final Event Description	Line
	28591	nsJAREnumerator::nsJAREnumerator(nsZipFind *)	20100302-1.9.3-xr26_0m6/modules/libjar/nsJAR.h
uninit_member	Non-static class member mNameLen not initialized in this constructor or any functions called by it	202
	10903	nsZipWriter::nsZipWriter()	20100302-1.9.3-xr26_0m6/modules/libjar/zipwriter/src/nsZipWriter.cpp
uninit_member	Non-static class member mCDSOffset not initialized in this constructor or any functions called by it	84
	10902	nsJARInputStream::nsJARInputStream()	20100302-1.9.3-xr26_0m6/modules/libjar/nsJARInputStream.h
uninit_member	Non-static class member mNameLen not initialized in this constructor or any functions called by it	60
	10768	nsZipArchive::nsZipArchive()	20100302-1.9.3-xr26_0m6/modules/libjar/nsZipArchive.cpp
uninit_member	Non-static class member field mArena.mask not initialized in this constructor or any functions called by it	796
	10572	nsDeflateConverter::nsDeflateConverter()	20100302-1.9.3-xr26_0m6/modules/libjar/zipwriter/src/nsDeflateConverter.h
uninit_member	Non-static class member mWriteBuffer not initialized in this constructor or any functions called by it	66
	10571	nsDeflateConverter::nsDeflateConverter(int)	20100302-1.9.3-xr26_0m6/modules/libjar/zipwriter/src/nsDeflateConverter.h
uninit_member	Non-static class member mWriteBuffer not initialized in this constructor or any functions called by it	71

Comment 26

[timeless]

Sorry, wrong "lib jar"
 	CID 	Checker		Function	File	Final Event	Final Event Description	Line
	9950	REVERSE_INULL	JAR_stash_cert	20100302-1.9.3-xr26_0m6/security/nss/lib/jar/jarver.c	deref_ptr_in_call
Dereferences pointer "nickname"	1242
	9740	RESOURCE_LEAK	JAR_cert_attribute	20100302-1.9.3-xr26_0m6/security/nss/lib/jar/jarver.c	noescape
Variable "ret" not freed or pointed-to in function "strlen"	1116
	9740	RESOURCE_LEAK	JAR_cert_attribute	20100302-1.9.3-xr26_0m6/security/nss/lib/jar/jarver.c	noescape
Variable "ret" not freed or pointed-to in function "strlen"	1116
	9740	RESOURCE_LEAK	JAR_cert_attribute	20100302-1.9.3-xr26_0m6/security/nss/lib/jar/jarver.c	noescape
Variable "ret" not freed or pointed-to in function "strlen"	1116
	9740	RESOURCE_LEAK	JAR_cert_attribute	20100302-1.9.3-xr26_0m6/security/nss/lib/jar/jarver.c	noescape
Variable "ret" not freed or pointed-to in function "strlen"	1116
	9739	RESOURCE_LEAK	JAR_cert_attribute	20100302-1.9.3-xr26_0m6/security/nss/lib/jar/jarver.c	noescape
Variable "cer_o" not freed or pointed-to in function "strlen"	1047
	9739	RESOURCE_LEAK	JAR_cert_attribute	20100302-1.9.3-xr26_0m6/security/nss/lib/jar/jarver.c	noescape
Variable "cer_o" not freed or pointed-to in function "strlen"	976
	9738	RESOURCE_LEAK	JAR_cert_attribute	20100302-1.9.3-xr26_0m6/security/nss/lib/jar/jarver.c	noescape
Variable "cer_l" not freed or pointed-to in function "strlen"	1049
	9738	RESOURCE_LEAK	JAR_cert_attribute	20100302-1.9.3-xr26_0m6/security/nss/lib/jar/jarver.c	noescape
Variable "cer_l" not freed or pointed-to in function "strlen"	978
	9737	RESOURCE_LEAK	JAR_cert_attribute	20100302-1.9.3-xr26_0m6/security/nss/lib/jar/jarver.c	noescape
Variable "cer_cn" not freed or pointed-to in function "strlen"	1037
	9737	RESOURCE_LEAK	JAR_cert_attribute	20100302-1.9.3-xr26_0m6/security/nss/lib/jar/jarver.c	noescape
Variable "cer_cn" not freed or pointed-to in function "strlen"	966
	9736	RESOURCE_LEAK	JAR_cert_attribute	20100302-1.9.3-xr26_0m6/security/nss/lib/jar/jarver.c	noescape
Variable "cer_e" not freed or pointed-to in function "strlen"	1039
	9736	RESOURCE_LEAK	JAR_cert_attribute	20100302-1.9.3-xr26_0m6/security/nss/lib/jar/jarver.c	noescape
Variable "cer_e" not freed or pointed-to in function "strlen"	968
	9200	NO_EFFECT	jar_listzip	20100302-1.9.3-xr26_0m6/security/nss/lib/jar/jarfile.c	unsigned_compare
Comparing unsigned greater than or equal zero is always true. "compression >= 0U"	750
	8910	FORWARD_NULL	JAR_find_next	20100302-1.9.3-xr26_0m6/security/nss/lib/jar/jar.c	var_deref_op
Variable "signer" tracked as NULL was dereferenced.	393
	8909	FORWARD_NULL	JAR_find_next	20100302-1.9.3-xr26_0m6/security/nss/lib/jar/jar.c	var_deref_op
Variable "ctx->next" tracked as NULL was dereferenced.	447

Note that the datestamp here may or may not be meaningful, i can't make any promises.

The CIDs are of course only meaningful to me (and barely at that).

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch v3 - for review

I believe this patch addresses all the latest issues reported in libjar
by Coverity.  I also factored out a large block of duplicated code into 
a new function so I only had to fix it once, not twice.  

Timeless, please review.

Comment 28

[timeless]

Comment on attachment 441330 [details] [diff] [review]
Patch v3 - for review

I'm pretty sure you don't want to use SEPLEN the way you do (but you're just moving code). I'd suggest:

 #define SEP " <br> "
+#define CALC_SEPLEN() int seplen = PORT_Strlen(SEP);
+#define SEPLEN seplen

 /* This is pretty ugly looking but only used
  *   here for this one purpose. */
 static char *
 jar_cert_name_string(CERTName * name)
 {
+    CALC_SEPLEN();

+    if (retlen <= 1)
+        goto FAIL;
     ret = (char *) PORT_ZAlloc(1 + retlen);
...
+fail:
     PORT_Free(cer_cn);

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v4 - for review

Thanks for the suggestion.  I implemented it. 
I also found a nasty bug in my previous patch.  This patch fixes it.
When I fixed it, I found another bug in the original code.  This patch 
fixes that too, I believe.  It's all just string process stuff, no heavy
duty crypto stuff.  I'm consistently amazed at how we get the hard stuff
right and the easy stuff wrong. :-/ 

Please review again, sorry.

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 441341 [details] [diff] [review]
patch v4 - for review

Wait a minute.
In comment 23, I observed that function JAR_stash_cert is dead code, 
having NO callers anywhere in NSS or all of Mozilla for that matter.  
According to MXR, that is still true.

So why is Coverity reporting deref_ptr_in_call ?  
Shouldn't it be reporting "Dead code" ?

Likewise, it appears that function JAR_cert_attribute, which I just 
spent HOURS cleaning up, is dead code.  What a waste of time!

I'm canceling this review request.

In a while, I will submit a completely different patch for review, 
one that expunges lots of dead code.

Comment 31

[timeless]

Given that this is Linux, that I doubt you're doing symbol hiding, and that I suspect Coverity isn't very aware of Linux symbol hiding anyway... I suspect Coverity doesn't have a way to know that your function can't be used by a third party. With FOO_naming, I'd assume they're exports anyway :).

Comment 32

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch v5 - for review

This patch fixes a few real Coverity issues, and removes LOTS of dead code.

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

Bug 337354: Fix various errors in lib/jar reported by Coverity.
Also remove much dead code.  r=Timeless.

Checking in jar.c;     new revision: 1.7; previous revision: 1.6
Checking in jarfile.c; new revision: 1.13; previous revision: 1.12
Checking in jarver.c;  new revision: 1.19; previous revision: 1.18

Thanks, Timeless

Comment 34

[Robert Relyea]

thanks nelson, the more dead code we can get our of libjar the better...

bob