Persona is no longer an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 160805 - Convert NSS code to use SEC_QuickDERDecodeItem
: Convert NSS code to use SEC_QuickDERDecodeItem
Status: NEW
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.6
: All All
: P2 normal (vote)
: ---
Assigned To: Robert Relyea
Depends on: 178893 178894 178895 178897 178898 252398
  Show dependency treegraph
Reported: 2002-08-02 18:59 PDT by Julien Pierre
Modified: 2010-09-27 18:11 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Large patch to replace SEC_ASN1DecodeItem calls with SEC_QuickDERDecodeItem on "lib" directories (9.87 KB, patch)
2002-08-15 22:16 PDT, Julien Pierre
rrelyea: review+
Details | Diff | Splinter Review
context diff (17.50 KB, patch)
2002-08-23 17:52 PDT, Julien Pierre
no flags Details | Diff | Splinter Review
Patch for incorrect SEC_QuickDERDecodeItem usages (17.73 KB, patch)
2002-08-29 17:20 PDT, Julien Pierre
no flags Details | Diff | Splinter Review
Fix a double free in CERT_DecodeAVAValue (528 bytes, patch)
2002-09-07 11:38 PDT, Wan-Teh Chang
no flags Details | Diff | Splinter Review
Fix a leak and a double free in CERT_DecodeAVAValue (2.06 KB, patch)
2002-09-07 20:38 PDT, Wan-Teh Chang
julien.pierre: review+
Details | Diff | Splinter Review
fix a regression in NSS 3.6 & up with incorrect QuickDER usage (1.68 KB, patch)
2003-01-15 16:51 PST, Julien Pierre
no flags Details | Diff | Splinter Review

Description Julien Pierre 2002-08-02 18:59:32 PDT
Wherever possible in NSS, DER decoding code should be converted to use this
function in place of SEC_ASN1DecodeItem. This will save memory and CPU. The
changes needed to do this are very minor :
1) the caller must keep the data in src
2) the caller must pass an arena to the decoder

Many places already comply with those requirements, and will only need a symbol
change. Places that don't need arena or free the input will need more extensive

This bug is a placeholder which is expected to receive a lot of small patches in
various areas of NSS to implement the same change accross the board.
Comment 1 Julien Pierre 2002-08-15 22:16:11 PDT
Created attachment 95523 [details] [diff] [review]
Large patch to replace SEC_ASN1DecodeItem calls with SEC_QuickDERDecodeItem on "lib" directories
Comment 2 Julien Pierre 2002-08-15 22:30:24 PDT
I have attached a fairly large patch that replaces the calls to
SEC_ASN1DecodeItem with calls to SEC_QuickDERDecodeItem.

I primarily went after the instances of SEC_ASN1DecodeItem that already used an
arena, since these calls can be directly replaced with calls to the new decoder.
The hard part was determining in which places BER may be expected instead of
DER, so as not to patch these calls. The one that gave me trouble was the P12
code, as some calls are not just under pkcs12 but also in the softoken and
pk11wrap. This is what the review should focus on - make sure I didn't patch any
places that shouldn't be because they can accept BER.

There is also one patch that converts a non-arena usage to arena in secname.c . 

I have run on a tip build with this patch and everything is green. I have
also used Mozilla and had the same success with the patch as without.

I believe this set of changes is safe to check in. The benefit is a performance
improvement, most probably in the certificate decoding. I did not have time to
run any performance tests, but if the effect of the quick DER decoder is as
significant on certs as it was on CRLs, then this could be a big win for some
apps that manipulate a lot of certs like PSM.

There should also be a phase 2 - look at the DER usages of SEC_ASN1DecodeItem
that don't use arenas. They should be revisited whenever possible to be able to
use the new decoder, which requires an arena. But that's more complicated and
may not make the cut in 3.6 like this patch should.
Comment 3 Julien Pierre 2002-08-22 16:23:52 PDT
Would anyone care to review this patch ?
It would provide a performance improvement. If there is no time to review the
entire patch or if we are only concerned about cert decoding performance for
now, the review can be limited to the cert files for NSS 3.6.
Comment 4 Robert Relyea 2002-08-22 17:11:27 PDT
Comment on attachment 95523 [details] [diff] [review]
Large patch to replace SEC_ASN1DecodeItem calls with SEC_QuickDERDecodeItem on "lib" directories

As far as I can tell it looks good, a context diff would be preferable. All the
objects I see are DER not BER objects, and they all appear to be using the
Comment 5 Julien Pierre 2002-08-23 17:52:30 PDT
Created attachment 96510 [details] [diff] [review]
context diff
Comment 6 Julien Pierre 2002-08-23 18:07:12 PDT
I attached the full context diff of the same patches. Since it's already been
reviewed, it is now checked in to the tip.

I am keeping this bug open however as there is still a second half to it - the
conversion of code that doesn't use arenas.
Comment 7 Julien Pierre 2002-08-28 16:44:51 PDT
There is a problem with this patch.
Unlike SEC_ASN1DecodeItem, SEC_QuickDERDecodeItem does not make a copy of the
DER input. Rather, the result always points back into the DER input.
If the input was not allocated in the arena and is freed, then the result of the
decoder becomes invalid.

The solution for this is to make a copy of the DER input into the arena.
However, this will use more memory, so we don't want to do it by default.
This means the caller should do it.

We need to re-review all 47 cases of calls to SEC_ASN1QuickDERDecodeItem in the
patch that went in last friday to ensure that is the case.

There is at least one instance with cert extensions that Bob found where the DER
didn't come from the arena, so the decoded data became invalid.
Comment 8 Julien Pierre 2002-08-29 17:20:09 PDT
Created attachment 97249 [details] [diff] [review]
Patch for incorrect SEC_QuickDERDecodeItem usages

This patch makes a copy of the SECItem in all functions that call it which
don't free the arena before returning.

This ensures that there is never any decoded data pointing to a buffer outside
the arena, which may get freed separately.
Comment 9 Julien Pierre 2002-08-30 17:48:40 PDT
I checked in the patch and removed the copy in SEC_QuickDERDecodeItem.
Bob, it would still be helpful if a new review was done of the remaining usages
of quickder, just in case I missed any.

Some of the cases were not obvious, like decoding done in multiple steps. In
those cases, I only did the copy at the first step, not in the following
intermediate steps, to conserve memory.
Comment 10 Wan-Teh Chang 2002-09-07 11:38:56 PDT
Created attachment 98281 [details] [diff] [review]
Fix a double free in CERT_DecodeAVAValue

There is a pre-existing double free in CERT_DecodeAVAValue.
As part of a patch for this bug the PORT_Free calls were
replaced by PORT_ArenaFree calls, and the double free
results in an infinite loop in NSPR's FreeArenaList function.

This patch fixes that.	Since this is a pre-existing problem,
I will open a separate bug.
Comment 11 Wan-Teh Chang 2002-09-07 20:38:04 PDT
Created attachment 98302 [details] [diff] [review]
Fix a leak and a double free in CERT_DecodeAVAValue

I found that the double free in CERT_DecodeAVAValue
is not a pre-existing bug but rather was introduced
by an earlier patch for this bug.

I also found that my previous fix is wrong.  Here
is a correct fix.

1. It uses the current name 'PLArenaPool' instead
of the old name 'PRArenaPool'.

2. It fixes the problem that the arena is not freed
if SEC_QuickDERDecodeItem fails.

3. There are two ways to fix the double free.  I
choose the simpler fix.  I allocate 'utf8Val' in the
arena.	This makes it simple to free all the data
allocated in this function: just free the arena.

In the interest of time, I've checked in this fix,
but I would really appreciate a code review.
Comment 12 Julien Pierre 2002-10-17 18:21:50 PDT
I think we should continue this effort in 3.7 and go after the places where DER
is used but an arena was not, and see if it is possible to change the memory
management easily. I did not even attempt it for those cases in 3.6 .

Also, I only went over the nss/lib/* subdirectories, completely ignoring the
cmd/* directories . As Nelson found out, there is code that could benefit from
QuickDER there too, especially as it appears to have fewer bugs.
Comment 13 Wan-Teh Chang 2002-10-29 16:28:32 PST
I suggest that we divide the NSS source tree into
five pieces, open a bug for each, and use this bug
as the tracking bug.  It's too much work for one
person to make the requested change to the entire

Bob, could you take care of this and assign the
Comment 14 Robert Relyea 2002-11-07 10:22:03 PST
Adding dependencies for the 5 separate bugs to update the tree.
Comment 15 Wan-Teh Chang 2002-12-06 11:16:29 PST
Moved to target milestone 3.8 because the original
NSS 3.7 release has been renamed 3.8.
Comment 16 Julien Pierre 2003-01-15 16:50:08 PST
Nelson pointed out that one of the optimizations made to softoken was incorrect
and requires backing out :

"pk11_unwrapPrivateKey now calls SEC_QuickDERDecodeItem to decode 
unwrapped private keys.  This is OK for keys that were wrapped by our
token, because our token always DER-encodes private keys. But the PKCS 11
spec says that private keys are BER encoded, so private keys from another
implementation might fail to decode now that this function has switched to
the DER-only decoder."

I will attach a patch to fix the regression.
Comment 17 Julien Pierre 2003-01-15 16:51:37 PST
Created attachment 111664 [details] [diff] [review]
fix a regression in NSS 3.6 & up with incorrect QuickDER usage
Comment 18 Julien Pierre 2003-01-15 17:03:32 PST
Comment on attachment 111664 [details] [diff] [review]
fix a regression in NSS 3.6 & up with incorrect QuickDER usage

Checked this patch into the tip (NSS 3.8).

Checking in pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v	<--  pkcs11c.c
new revision: 1.43; previous revision: 1.42
Comment 19 Wan-Teh Chang 2003-01-15 17:12:41 PST
Julien, could you open a separate bug for this regression?
Comment 20 Nelson Bolyard (seldom reads bugmail) 2003-05-09 21:19:10 PDT
Remove target milestone of 3.8, since these bugs didn't get into that release.

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