Last Comment Bug 178894 - Quick decoder updates for lib/certdb and lib/certhigh
: Quick decoder updates for lib/certdb and lib/certhigh
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.6
: All All
: P3 enhancement (vote)
: 3.12
Assigned To: Julien Pierre
:
Mentors:
Depends on:
Blocks: 160805
  Show dependency treegraph
 
Reported: 2002-11-07 10:14 PST by Robert Relyea
Modified: 2006-08-07 12:09 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
use QuickDER where possible in lib/certdb (10.85 KB, patch)
2002-12-19 15:42 PST, Julien Pierre
no flags Details | Diff | Splinter Review
use QuickDER where possible in lib/certhigh (1.57 KB, patch)
2002-12-19 15:43 PST, Julien Pierre
no flags Details | Diff | Splinter Review
use QuickDER where possible in lib/certhigh (1.57 KB, patch)
2002-12-19 15:46 PST, Julien Pierre
no flags Details | Diff | Splinter Review
updated patch (9.92 KB, patch)
2003-01-21 21:58 PST, Julien Pierre
no flags Details | Diff | Splinter Review
use QuickDER where possible in lib/certdb (11.20 KB, patch)
2003-03-05 16:48 PST, Julien Pierre
rrelyea: superreview+
Details | Diff | Splinter Review
use QuickDER where possible in lib/certhigh (2.26 KB, patch)
2003-03-05 16:49 PST, Julien Pierre
no flags Details | Diff | Splinter Review
updated optimizations for lib/certdb and lib/certhigh (13.40 KB, patch)
2005-05-06 21:26 PDT, Julien Pierre
nelson: superreview-
Details | Diff | Splinter Review
Update (23.09 KB, patch)
2006-08-04 17:13 PDT, Julien Pierre
nelson: review+
Details | Diff | Splinter Review

Description Robert Relyea 2002-11-07 10:14:01 PST
The use of SEC_ASN1Decode and SECASN1DecodeItem should be examined in lib/certdb
and lib/certhigh to see if we can use the quick decoder instead.
Comment 1 Julien Pierre 2002-12-19 15:42:43 PST
Created attachment 109796 [details] [diff] [review]
use QuickDER where possible in lib/certdb
Comment 2 Julien Pierre 2002-12-19 15:43:04 PST
Created attachment 109797 [details] [diff] [review]
use QuickDER where possible in lib/certhigh
Comment 3 Julien Pierre 2002-12-19 15:45:51 PST
I just created two patches for these libraries. In several places, I have to
make a copy of the input data into the arena in order to ensure that the
resulting decoded structures point to memory that's not going to be deallocated
(ie. stack or temporary heap).

There are only two places left in the CRL code where I couldn't use QuickDER
because the API didn't take an arena. I have tested the changes using the
regular QA suite and everything passed.
Comment 4 Julien Pierre 2002-12-19 15:46:33 PST
Created attachment 109798 [details] [diff] [review]
use QuickDER where possible in lib/certhigh

correct patch
Comment 5 Julien Pierre 2002-12-19 15:51:51 PST
Taking bug. Feel free to review th epatches.
Comment 6 Julien Pierre 2003-01-09 18:37:09 PST
Really taking bug. Patch reviews still needed.
Comment 7 Julien Pierre 2003-01-21 21:57:13 PST
Nelson pointed out potential problems in the usage of QuickDER in certv3 to look
up subject key ID. This is because of an incorrectly designed API. See bug
183612 comment 19 (http://bugzilla.mozilla.org/show_bug.cgi?id=183612#c19) for
more information.

I actually ran into a crash with certutil -A which was caused by it in my tree.
So I'm removing that usage from the patch and generate a new one. 
Comment 8 Julien Pierre 2003-01-21 21:58:59 PST
Created attachment 112253 [details] [diff] [review]
updated patch

Don't use QuickDER to look up subject key ID due to memory allocation issues
Comment 9 Julien Pierre 2003-03-05 16:48:48 PST
Created attachment 116371 [details] [diff] [review]
use QuickDER where possible in lib/certdb

Don't use mark & release
Comment 10 Julien Pierre 2003-03-05 16:49:28 PST
Created attachment 116372 [details] [diff] [review]
use QuickDER where possible in lib/certhigh

Don't use mark & release
Comment 11 Julien Pierre 2003-04-07 16:52:52 PDT
Comment on attachment 116371 [details] [diff] [review]
use QuickDER where possible in lib/certdb

There is something wrong in one of the files in this patch. It causes "certutil
-d . -h all -L" to assert under the MS debugger on NT. See bug 199390 .
Comment 12 Julien Pierre 2003-05-12 16:29:21 PDT
Performance enhancement. Reprioritizing to P3.
Comment 13 Julien Pierre 2003-10-14 11:29:50 PDT
Bob, are you sure you meant to add a review+ for this patch ? It is known to
cause problems in the test suite, which is why it has not been checked in yet
and this bug is still opened. I hoped to get to this in 3.9 but I'm not sure now
given close deadlines.
Comment 14 Robert Relyea 2003-10-14 11:42:48 PDT
I didn't notice anything wrong in the review, but if it doesn't pass the test
suite, we definately shouldn't check it in. I'm just trying to clean up my
review log.

bob
Comment 15 Julien Pierre 2005-05-06 21:26:16 PDT
Created attachment 182838 [details] [diff] [review]
updated optimizations for lib/certdb and lib/certhigh

all.sh passes with these .
Comment 16 Nelson Bolyard (seldom reads bugmail) 2005-05-18 10:30:11 PDT
Comment on attachment 182838 [details] [diff] [review]
updated optimizations for lib/certdb and lib/certhigh

Several problems with this patch.
1. There are several functions that previously accepted NULL arena
   pointers that will now fail with NULL arena pointers, AND will
   leak allocated memory when they do.	
2. Several functions changed here have always leaked memory in 
   their error paths, (when arena was NULL), and now they leak more
   than before.  

I'm not sure whether it's OK for functions to now fail where they
previously succeeded (e.g. with NULL arenas), but I'm sure it's 
NOT ok for functions to leak if they fail.  

I think every function that fails when arena is NULL should detect
NULL arena explicitly at the begining, and return with an error 
code set before allocating anything that might leak.  In public
functions, it's not enough just to PORT_Assert(arena), because
(as we recently learned) some of our customers always test with
our optimized builds (even in their own debug builds).	So 
assertions need to be followed by tests that deal with their failures.

3. There appears to be one new leak that occurs in a success case.
That occurs in CERT_GetNickName:



>@@ -1566,21 +1582,21 @@
> 	    SECOID_FindOIDTag(&current->name.OthName.oid) == 
> 	      SEC_OID_NETSCAPE_NICKNAME) {
> 	    found = 1;
> 	    break;
> 	}
> 	current = CERT_GetNextGeneralName(current);
>     } while (current != names);
>     if (!found)
>     	goto loser;
> 
>-    rv = SEC_ASN1DecodeItem(arena, &nick, SEC_IA5StringTemplate, 
>+    rv = SEC_QuickDERDecodeItem(arena, &nick, SEC_IA5StringTemplate, 
> 			    &current->name.OthName.name);
>     if (rv != SECSuccess) {
> 	goto loser;
>     }
> 
>     /* make a null terminated string out of nick, with room enough at
>     ** the end to add on a number of up to 21 digits in length, (a signed
>     ** 64-bit number in decimal) plus a space and a "#". 
>     */
>     nickname = (char*)PORT_ZAlloc(nick.len + 24);
>@@ -1600,21 +1616,21 @@
> 		goto loser;
> 	}
> 	count++;
> 	sprintf(nickname, "%s #%d", basename, count);
>     }
> 
>     /* success */
>     if (nicknameArena) {
> 	returnName =  PORT_ArenaStrdup(nicknameArena, nickname);
>     } else {
>-	returnName = nickname;
>+	returnName = PORT_Strdup(nickname);
> 	nickname = NULL;
>     }
> loser:
>     if (arena != NULL) 
> 	PORT_FreeArena(arena, PR_FALSE);
>     if (nickname)
> 	PORT_Free(nickname);

The above new PORT_Strdup call causes nickname to be leaked in
that path.  nickname is allocated, (see the PORT_Zalloc call in
the "hunk" just above), and so the Strdup is unnecessary. 
I just wouldn't make this Strdup change, but it would also suffice 
to remove the nickname = NULL line just below the new Strdup call.
Comment 17 Robert Relyea 2005-06-23 16:12:52 PDT
Comment on attachment 182838 [details] [diff] [review]
updated optimizations for lib/certdb and lib/certhigh

Nelson already rejected this patch, removing review request
Comment 18 Julien Pierre 2006-08-04 16:16:53 PDT
Nelson,

Responding to comment #16 :

1. Since my patch applied to a number of functions, it would have been very helpful in your review if you could have specified exactly which functions have these problems, rather than just "several". I have had to review all of them in order to make sense of your comment.

Most of the functions actually require arenas to operate properly - otherwise there would be major problems such as leaks. Thus, I assumed they were already called with non-NULL arenas. I had done this analysis when I wrote the patch, but now I have done it all over again, and the result is below. Most of the changes I will make that result are simply to check for a non-NULL arena upon function entry. I will rename all the "arena" argument to "reqArena" in the prototype in the .c and .h files to make it clear when arenas are required.

- CERT_KeyFromDERCert is a private function. It is only called from 2 places, both with an arena. It will not leak if called with a NULL arena - it will just fail. So, no issue.

- CERT_KeyFromDERCrl is a public function . Internally, it is always called with a non-NULL arena.
Before this patch, it would allocated the returned derCrl either in the main heap if the passed-in arena was NULL, or in the arena if it wasn't - this is the behavior of SEC_ASN1DecodeItem.
With this patch, it does the same, by using SECITEM_CopyItem.
However, there is a bug in my patch where the second call to SEC_QuickDERDecodeItem uses the passed-in arena ("arena") rather than the temporary arena ("myArena") and thus the function will fail if a NULL arena is passed in. I will provide a fix for this. 
The function only allocated the memory upon decoder success, and the temporary arena is always freed. Thus, there is no leak introduced by my patch in this function.

- CERT_GeneralName is a public function. It didn't enforce a non-NULL arena upon entry. There is one previous caller which also didn't check if the arena it passed was non-NULL, CERT_DecodeAuthInfoAccessExtension .
Thus, the patch will make the function fail if arena argument is non-NULL. 

This function will leak in this case because it calls SECITEM_ArenaDupItem to make a temporary copy of the encoded name in the arena. Unfortunately, SECITEM_ArenaDupItem doesn't fail if the arena pointer is NULL ; it uses the heap instead.

To fix the leak, the function could be made to fail if arena is NULL before calling SECITEM_ArenaDupItem, since it will fail in SEC_QuickDERDecodeItem anyway without an arena. I think this is the best solution, because we can't really support this function with a NULL arena. There are pre-existing leaks in SEC_ASN1DecodeItem when called with a NULL arena because CERTGeneralName is a complex structure that should not have its individual fields allocated on the heap (eg. the array of rdns within CERTName) ; otherwise it would be extremely tedious to free. And we don't have any code anywhere to do that .

Thus, I propose that simply checking for a non-NULL arena upon function entry is the fix for this function.

- cert_DecodeNameConstraint has the same issue where it allocates something with SECITEM_ArenaDupItem which may end up on the heap instead of an arena . It is a private function which only has one caller, cert_DecodeNameConstraintSubTree , which is in turned called only by cert_DecodeNameConstraints (plural).
It returns a CERTNameConstraint structure which contains a CERTGeneralName, created by invoking CERT_DecodeGeneralName . We have already determined that CERT_DecodeGeneralName requires a non-NULL arena. Thus cert_DecodeNAmeConstraint also requires an arena.
A check should be added for it upon function entry.

- cert_DecodeNameConstraints (plural) has the same issue where it allocates something with SECITEM_ArenaDupItem which may end up on the heap instead of an arena. It is an internal function, called by a public function which does not check for an arena, CERT_DecodeNameConstraintsExtension , and by a private function, CERT_CompareNameSpace . The later is always called with an arena. 

cert_DecodeNameConstraints calls cert_DecodeNameConstraintSubTree, which calls cert_DecodeNameConstraint, which requires an arena. Thus, cert_DecodeNameConstraints should also require an arena.
A check should be added upon entry of this function .

- CERT_DecodeAltNameExtension is a public function which doesn't check for a NULL arena upon entry. It calls cert_decodeGeneralNames , which calls CERT_DecodeGeneralName, which requires an arena . Thus this function should check for a non-NULL arena upon entry.

- CERT_DecodeAuthInfoAccessExtension calls CERT_DecodeGeneralName, thus it should check for a non-NULL arena upon entry .

- ocsp_FinishDecodingSingleResponses previously called PORT_ArenaZAlloc which requires an arena. This call will fail (or even crash) if called with a NULL arena. It precedes the SECITEM_ArenaDupItem added by my patch, so I don't think there is a leak introduced by my patch, since there would be a crash or at least prior failure if a NULL arena was given. This function should be changed to check for a NULL arena upon entry.

2. Regarding pre-existing leaks on error paths with NULL arenas, please let me know if there is anything I haven't covered in the review above.

3. Regarding my change to CERT_GetNickName, you are right, that PORT_Strdup doesn't belong. I didn't notice the nickname = NULL; statement that prevents the freeing of the string upon exit .

A new patch will be forthcoming.
Comment 19 Julien Pierre 2006-08-04 17:13:57 PDT
Created attachment 232232 [details] [diff] [review]
 Update
Comment 20 Julien Pierre 2006-08-04 18:25:55 PDT
In comment 18, the third listed function should be "CERT_DecodeGeneralName", not CERT_GeneralName . This is important since this function is referenced in many places afterwards in my comments as having a requirement for a non-NULL arena.
Comment 21 Nelson Bolyard (seldom reads bugmail) 2006-08-07 02:15:08 PDT
Comment on attachment 232232 [details] [diff] [review]
 Update

I'm sorry that I didn't name the functions in my previous comment.  If I should do that again in some future review, please don't hesitate to ask me to add that info, and please do it right away!

This patch looks good.  We *may* have some complaints about binary compatibility (failing where it previously succeeded but leaked).  I think we'll just have to cross that bridge when we come to it.
r=nelson
Comment 22 Julien Pierre 2006-08-07 12:09:59 PDT
Thanks for the review, Nelson . I checked in this patch to the tip.

Checking in certdb/cert.h;
/cvsroot/mozilla/security/nss/lib/certdb/cert.h,v  <--  cert.h
new revision: 1.54; previous revision: 1.53
done
Checking in certdb/certdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.78; previous revision: 1.77
done
Checking in certdb/crl.c;
/cvsroot/mozilla/security/nss/lib/certdb/crl.c,v  <--  crl.c
new revision: 1.53; previous revision: 1.52
done
Checking in certdb/genname.c;
/cvsroot/mozilla/security/nss/lib/certdb/genname.c,v  <--  genname.c
new revision: 1.31; previous revision: 1.30
done
Checking in certdb/genname.h;
/cvsroot/mozilla/security/nss/lib/certdb/genname.h,v  <--  genname.h
new revision: 1.10; previous revision: 1.9
done
Checking in certdb/xconst.c;
/cvsroot/mozilla/security/nss/lib/certdb/xconst.c,v  <--  xconst.c
new revision: 1.9; previous revision: 1.8
done
Checking in certhigh/ocsp.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.27; previous revision: 1.26
done

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