Closed Bug 149816 Opened 23 years ago Closed 23 years ago

Need partial CRL DER decoding for performance

Categories

(NSS :: Libraries, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: julien.pierre, Assigned: julien.pierre)

References

Details

Attachments

(3 files, 15 obsolete files)

(deleted), text/html
Details
(deleted), patch
Details | Diff | Splinter Review
(deleted), text/plain
Details
Many applications don't need to do a full DER decode of a CRL, which can be very expensive if decoding all the entries. An example of this is that a full DER decode of a CRL with 1,200,000 entries takes 3 minutes of complete CPU peaking on my Sun Ultra10. Oftentimes, the application just needs access to a few fields, like the issuer name and the signature, but specifically not the entries themselves, which are the most expensive part of the decoding. So there needs to be a way to do a partial CRL decode to optimize these applications. For example, PSM could do a quick partial decode after downloading the DER CRL, and then actually import the CRL if the user wants to install it, and otherwise discard it without ever having done the full lengthy decode. I propose a new argument to CERT_DecodeDERCrl, SEC_CRL_TYPE_NO_ENTRIES , which will decode a CRL into the same CERTSignedCrl structure as the code does today, except that it will skip the decoding of crl.entries in CERTSignedCRL.
I created a copy of the CRL template and replaced { SEC_ASN1_OPTIONAL | SEC_ASN1_SEQUENCE_OF, offsetof(CERTCrl,entries), cert_CrlEntryTemplate }, with { SEC_ASN1_SKIP | SEC_ASN1_OPTIONAL | SEC_ASN1_SEQUENCE_OF }, Unfortunately, when running the partial decode against the big CRL mentioned above, I got an assertion at line 545 in secasn1d.c : 542 if (encode_kind & (SEC_ASN1_ANY | SEC_ASN1_SKIP)) { 543 PORT_Assert (encode_kind == under_kind); 544 if (encode_kind & SEC_ASN1_SKIP) { 545 PORT_Assert (!optional); 546 PORT_Assert (encode_kind == SEC_ASN1_SKIP); 547 state->dest = NULL; 548 } It's apparently not possible to skip decoding of an optional field. I don't know if it's not possible at all without having the definition of the field in the template, or if it's a limitation of our current ASN.1 decoder. From looking at dumpasn1 output with Nelson, I think it's the later.
I tried removing the SEC_ASN1_OPTIONAL, since I was testing with a CRL that contained entries. It expected that to work for this case, but it still didn't. I got : Assertion failure: encode_kind == SEC_ASN1_SKIP, at secasn1d.c:546 This is in the same code block as pasted above.
I tried another approach, which was to keep the sequence in the higher-level CRL template as follows : { SEC_ASN1_OPTIONAL | SEC_ASN1_SEQUENCE_OF, offsetof(CERTCrl,entries), cert_CrlSkipEntryTemplate }, and a skipping sub-template for each entry : static const SEC_ASN1Template cert_CrlSkipEntryTemplate[] = { { SEC_ASN1_SKIP_REST }, { 0 } }; I still got a failure : Assertion failure: state->top->current == state, at secasn1d.c:1304 /* * Now parse that out of our data. */ if (SEC_ASN1DecoderUpdate (state->top, (char *) item->data, item->len) != SECSuccess) return; PORT_Assert (state->top->current == state); PORT_Assert (state->child == child); /* At this point, I'm really not sure what sort of ASN.1 template I need in order to skip over the CRL entries. Bob, Ian, you are both a lot more familiar with ASN.1 than I am ... Any suggestions ?
Blocks: 149835
The assertion you cited in comment 2 clearly indicates that SEC_ASN1_SKIP is expected to be mutually exclusive with all other non-zero bits in that word of the template. However, I recall being told that some of these assertions were/are there to detect bad or unexpected (untested) templates, so it's possible that if you got past those assertions, the code would work OK. Suggest you try a) just using SEC_ASN1_SKIP in your template for the sequence of entries, and b) try commenting out those two assertions and see how far you get.
Really taking bug
Assignee: wtc → jpierre
Priority: -- → P1
Target Milestone: --- → 3.6
Attached patch Support partial decoding of CRLs (obsolete) (deleted) — Splinter Review
This patch adds support for the new SEC_CRL_TYPE_NO_ENTRIES type for the CERT_DecodeDERCrl function. This will allow an application to do a quick decode of a CRL, skipping the biggest field, which is the entries. Importing a partial CRL is not supported (enforced in certhigh.c in CERT_ImportCRL). The resulting structure is strictly for use by the application and not by any other NSS API. The only thing that can be done with it is to look at the fields, or to free it with SEC_DestroyCrl.
FYI, I tested the performance of the partial decode using a patched crlutil before I enforced that the partial CRLs couldn't be imported. On a CRL contained 1.2 million entries, it took 23s to do the partial decode vs 1m09s to do a full decode. This is for the optimized build. On the debug build, it took about 55s vs 2m50s. This is still far too much. Skipping over the entries in the CRL should reduce the decoding time to almost nothing - there is only about 2 KB of DER to actually decode. But this patch already provides a 3:1 improvement, without touching the DER decoder.
A couple of comments: 1) We might consider always doing partial decodes except for display and Cert Verification (even the display case, I don't think any of your current clients displays the whole CRL). 2) For the decode case in the revocator: Take a look at the code I did in softoken to reduce our use of ASN1_Decode. There is some simple code which hand decodes an ANS.1 structure if we are just after a few simple elements. The code is called from DecodeACert(). That code should decode a CRL is a fixed time independent of how big the CRL is. bob
Bob, As far as doing partial decodes everywhere - the partial decode seemed to break the CRL import to the database. I didn't yet try to figure out why yesterday. However, I think we ought to decode the whole DER block before importing it into the database. Otherwise, we may end up with a corrupt object that doesn't decode when it gets used during cert verification, and I think we don't want that to happen. As far as CRL display, this is an application specific usage, and the decision to do a full or partial CRL decode is already available to the application with my patch - it just needs to pass SEC_CRL_NO_ENTRIES as the type when doing the decode. There isn't a way yet to go from a partial to a full decode yet. This is something I'd like to add, but which would require a new API, and some support from the ASN.1 decoder to retrieve the offset and size of the "entries" field into the DER blob during partial decode, so that we can later do a DER decode of that CRL field alone. This wasn't part of the original design for this feature, though.
Hmm, that would indicate something is wrong with the partial part of the partial decode because the database does not store decoded objects. The only way partial decode would fail to import is if the URL or Issuer fields are not being decoded correctly. I think we should debug this before we check in the code.
When I traced the code yesterday, all the fields were identical between the two decodes, except for the entries pointer which was NULL. I will trace this again as soon as my home directory is revived from its zombie state - I/O operations on it are taking hours ...
Actually I think I know why the import failed - this is because my new value of 2 for SEC_CRL_TYPE_NO_ENTRIES was being propagated to the PK11_PutCrl call. All the code that deals with CRLs and KRLs assumes that if it's not one, it's the other. So this partial CRL was being treated as a KRL - saved with the wrong attribute. Perhaps it is wrong for me to add a new type like this given the problem that it creates, but instead a new API should be created, eg. CERT_PartialDecodeDERCrl ?
I think using the type in decode was fine, but the resulting CRL should be a TYPE_CRL. We should have a flag, however, that we can check to make indicate the CRL is partially decoded. The check revocation function (I forget the name) in CERT_VerifyCertChain() would check this before it searched for the cert on the revocation list.
The numbers I got with crlutil were affected by disk I/O. I was timing from the start to the end of the process, but there was time needed to read the file and then to save the CRL to the database. The only way to get around this was to try to test with a blank cert7.db - not containing the CA issuer cert - so that saving would save. This prevented flushing the disk cache. Then measuring performance of crlutil the second time, after the CRL was entirely in the OS file cache. These are the new results I got after doing this : Solaris - optimized full decode : 00:36 partial decode : 00:16 Solaris - debug full decode : 03:01 partial decode : 01:06 Windows - optimized : full decode : 00:27 partial decode : 00:13 Windows - debug : full decode : 01:37 partial decode : 00:29 OS/2 - optimized full decode : 00:51 partial decode : 00:23 OS/2 - debug full decode : 03:37 partial decode : 00:28 All 3 systems have approximately the same MHz - UltraSparc IIi 440 MHz, Pentium II 450 for the Windows box and Pentiu; II 400 for the OS/2 box.
A CRL with no entries ends up with a NULL "entries" even with a full decode, so there needs to be a boolean to keep track of the fact that it's been decoded. Given that the performance is still not what it should be on the partial decode, I'm going to pursue what we discussed in the NSS meeting today, which is to write a hand-decoder that will skip the entries, as Bob suggested in comment #8. That should result result in a fixed-time partial decode.
Blocks: 158239
Actually, it turns out that the hand-decoder would be quite complicated to write. The CRL has many fields, and there is a CRL structure inside of a signed CRL structure. The multi-level of DER decoding causes even more problems. I tried getting rid of the lower-level template completely, and keeping the higher-level one only, telling it to skip the decoding of the CRL altogether - and only decode the signature - but it still took quite some time to decode (about half the time as my previous partial decode, but of course I don't even know the date of the CRL at that point). The main problem that I have observed by debugging is that the decoder insists on iterating a loop a single byte at a time, which is extremely wasteful on a 25 MB object. With the nested CRL templates, it does this twice. If I could get it to properly skip for the SEC_ASN1_SKIP case, I think it would solve all the performance problems. I'm still trying to figure that out.
To illustrate the problem, I created a fake decode template like this. I just have to know the number of fields in the CRL in order to make it work : static const SEC_ASN1Template BlankTemplate[] = { { SEC_ASN1_SKIP }, { SEC_ASN1_SKIP }, { SEC_ASN1_SKIP }, { SEC_ASN1_SKIP }, { SEC_ASN1_SKIP }, { 0 } }; The above template causes the decoder to about 30s to decode the 25 MB CRL (and in effect, does nothing). If I use the following template on the other hand, the call returns immediately : static const SEC_ASN1Template BlankTemplate[] = { { SEC_ASN1_SKIP_REST }, { 0 } };
Attached patch update - but still uses SEC_ASN1_SKIP (obsolete) (deleted) — Splinter Review
This is an update to the previous patch. It uses the same approach, but works with the new CRL functions in a cleaner way, using bitflags. For now unfortunately, it still relies on SEC_ASN1_SKIP to do partial decoding, which gives unsatisfactory performance.
Attachment #91439 - Attachment is obsolete: true
Ran under purify under windows. The partial decode was taking 29s. Under purify it took 18 minutes. Results show : - about 2.5 million arena allocation (sec_asn1d_zalloc) accounting for 17% of the time - roughly the same number of arena frees (PORT_ArenaRelease), accounting for 29% of the time. - 11% of the time is spent in the 3 calls to SEC_ASN1_DecoderUpdate, in the loop. - sec_asn1d_prepare_for_contents is called 7.2 million times, and accounts for 2.2% of the time spent.
Additional data points : 1) sec_asn1d_prepare_for_contents causes an allocation of at least the size of the DER input, at line 1076 : item->data = (unsigned char*)sec_asn1d_zalloc (poolp, alloc_len); I'm not sure if this is necessary. I would hope that it can be eliminated, since RAM can be a limitng factor with large decodes (though not on my machines) 2) I set breakpoints on all lines that referenced SEC_ASN1_SKIP . None got hit until about half the time needed for the overall decode . Looking at the stack, I saw that only 276 bytes were left to consume at the top level decode function, out of 26 MB. Unfortunately, the current logic of the decoder is to first look at the input stream, one byte at a time, then look at the template after the end of a field to process it ... From this, I conclude that the very logic of the decoder has to be changed in order to be able to get good "skip" performance. It would need to look at the template first, rather than the input data, in order to possibly be efficient. It might be possible to hack it and make it work for this one-off skip case, but I don't think it will truly solve the problem. As soon as there is an inline template, the effect of this is multiplied. Even if the SEC_ASN1_SKIP performance is fixed, if it is in an inline template, the "outer" decoding would still read one byte at a time. It's looking as if a field by field "hand" CRL decoder is the only short-term solution for this problem , until a better decoder is written ...
I started implementing this field by field decoder using Bob's approach from comment #8. I almost got it working, except for the handling of optional fields. The partial decode is taking between 0.0 and 0.1 second of user time on Solaris according to the "time" command , compared with 50 s previously. Bob's code uses the following to determine the presence of an optional field : /* skip past any optional version number */ if ((buf[0] & 0xa0) == 0xa0) { dummy = nsslowcert_dataStart(buf,buf_length,&dummylen,PR_FALSE); if (dummy == NULL) return SECFailure; buf_length -= (dummy-buf) + dummylen; buf = dummy + dummylen; } This check does not work for me. I haven't been able to find a check that worked reliably for all types of optional fields. When I get to the optional CRL version (which I know is missing in my CRL), the first byte is is 0x30. This appears to be the beginning of the next field, which is a sequence (signature algorithm). Is that to say that there is no marker for a missing optional field ? If so, is the application expected to look at the type of the next field to determine which field was missing ? And if so, how would that work for SKIP ? Wouldn't it mean that you would need to know the format and parse all optional fields, in order to be able to properly skip them ? I hope this is not why we have asserts about not having SEC_ASN1_SKIP and SEC_ASN1_OPTIONAL at the same time in the current decoder ... I guess I need to know a little bit more about the implicit rules of ASN.1 . eg. is there something that forbids two consecutive fields from being the same type ?
Attached patch work in progress . contains new ASN.1 decoder (obsolete) (deleted) — Splinter Review
This patch implements several of the suggestions in the defect. This is a work in progress. It implements SKIP functionality in a very fast way. a) The function DecodeSignedCRL takes Bob's field-by-field approach to the decode, for the outer template, which is a sequence. This works fine. b) The function DecodeCRL attempts to mimic our decoder's API and in fact uses an array of templates as input, rather than code for each field, while still performing well. This is only working for the SEC_ASN1_SEQUENCE case for now, but could likely be improved to work with all types of templates if the concept proves sound. I'm almost ready to test the full CRL decoding with this code, and I also expect it to be much improved. The only problem currently for the partial CRL decoding, which happens regardless of method a) or b), is the handling of optional fields. I'm not able to properly detect that an optional field is missing. Currently, they are all assumed to be missing. Therefore, if one is instead present, subsequent decoding is hosed. If knowledge of the type of the next field is required to fix, then this would be a major reason for having to use method a) and writing different testing code for each optional field, instead of method b). It might still be possible to automate this in processing of a template array. However, this wouldn't be fully compatible with the current decoder, which doesn't support the concept of skipping an optional field, and may still require providing the type information in the template. Ie. once you use the new decoder, you may create a decoding template that the old decoder couldn't handle. And conversely, I don't claim that this limited piece of decoder can handle all the functionality of the existing decoder; I'm only focusing on what's used in the CRL templates specifically at the moment, but if this works and ends up significantly improving not only partial decodes but also full decodes, as I believe it will, it could be extended to other places in NSS where DER is decoded.
Per comment 21 : Optional fields are not tagged as optional. You have to know that an optional field is expected to be there, and what it's tag is supposed to be. In the case of the cert code the only optional field I run into is the serial number, which has an int tag. (which is why the check 0xa0). 0x30 is a sequence. You know the optional field is there because you see the optional tag instead of the tag you are expecting for the next field. bob
Bob, Thanks. I was trying to come up with some generic algorithm that would allow me to detect missing optional fields in a sequence. I was trying to find a way that didn't require looking at the next item's type in the template array, as this would make things complicated if there were several consecutive optional fields, or if the next type was an inline template array. I'm going to test another approach that doesn't require any knowledge of the next item - just trying to decode the optional item, and if the decode fails, assume it's missing and move on to the next template entry. Still, I assume there must be some sort of restriction in ASN.1 on optional fields. eg. in a consecutive set of optional fields, can there be two of the same type ? And if so how do you distinguish which one is missing - since you couldn't do it based on type ? I am in need of ASN.1 documentation to be able to implement the faster generic decode algorithm.
Quoting from "A Layman's Guide to a subet of ASN.1, BER and DER", section 5.12 "Sequence" : "The types of any consecutive series of components with the OPTIONAL or DEFAULT QUALIFIER, as well as of any component immediately following that series, must have distinct tags. This requirement is typically ssatisfied with explicit or implicit tagging on some of the components." This appears to clarify my questions from last night and earlier today, and makes it possible to check for any optional type based on the template definition, without writing custom code, since one can predict the tag to expect from it, and if it doesn't match, assume that the field is missing. I have implemented the algorithm following this assumption, and it appears to work very well. The partial CRL decoding is complete, shrinking on my Solaris debug build from about 50 seconds to ~0 . The full CRL decoding using this new decoder is also working. It went down from about 3 minutes to 2 minutes 10s. There is still much room for improvement as currently the new decoder reverts to the old decoder for encountering nested sequences. The decoding of the entries with the old decoder alone is what's taking nearly all of the 2mn10s time in the full decoding. I will make further improvements to the decoder. I believe the new decoder may be usable as a drop-in replacement for the old one. It will be optimized for the areas that I'm running into performance problems with CRLs, but all of the existing functionality should be supported, plus some extra that I already added (previously disallowed asserted cases of flags that are now permitted, like SKIP and OPTIONAL !!!). Another advantage of the new decoder is that there is never any copy of the input. Rather, pointers at various offsets in the original DER input from the top-level are passed down to the low-level decode functions for primitive types. This has reduced the peak memory usage for the full decode to 120MB.
I should add that it's not really possible with the restricted existing template-based decoder to get the partial CRL decoding working. I have been doing it only by luck because my CRL has entries. The existing template syntax doesn't allow skipping of optional fields - eg. mixing SEC_ASN1_OPTIONAL with SEC_ASN1_SKIP. Since the CRL entries are optional, but we want to skip them , when decoding a CRL that is missing them using the "skip entries" template I used, the extensions would be skipped instead. So technically, a new decoder is required for this functionality, and not just for performance - a decoder that is able to look at the type of the optional field so that it can skip it, which I have implemented, and where SEC_ASN1_OPTIONAL and SEC_ASN1_SKIP can both be specified, but with an additional type requirement.
Attachment #92322 - Attachment is obsolete: true
Attachment #92555 - Attachment is obsolete: true
Attachment #92993 - Attachment is obsolete: true
Attached file New ASN.1 decoder (obsolete) (deleted) —
Attached patch patch to crlutil for partial decoding option (obsolete) (deleted) — Splinter Review
I have now managed to get the performance for the full decoding of the 26 MB CRL down to a very low level. On an optimized build on my 450 MHz Sparc box, the new full decoding time is 3.5 seconds, compared to 34 seconds with the old decoder. The total process memory usage for crlutil is also down to 67 MB, from 144 MB. This means the new decoder is now roughly 10x as fast, and uses less than half the memory. On a debug build the full decode takes under 9 seconds with the new decoder, vs about 3 minutes with the old one. RAM usage is about the same as for optimized. I invite you to review the code for the new decoder in der.c , http://bugzilla.mozilla.org/attachment.cgi?id=93261&action=view . I have also debugged and successfully tested this code with NES and it was able to handle that same large CRL for client auth.
I now have a complete implementation of a DER decoder, except for SEC_ASN1_DYNAMIC, and the decoder is up to about 800 lines of code, with plenty of error checking added. I attempted to replace the default decoder in NSS with mine. I created a wrapper function that duplicated the whole DER input, so that decoded output would remain valid for callers that free the DER input memory after decoding - there are some several such cases I ran into that gave me headaches debugging. I then simply mapped this new function to the symbol SEC_ASN1DecodeItem, while keeping the old one as Legacy_ASN1DecodeItem, since I still call it in the one case for SEC_ASN1_DYNAMIC that I haven't implemented yet. The goal of this exercise was to see which applications I could run this way. In particular, I expected our command-line tools and NES to run, since they didn't use BER (with the exception of the NES admin that needs to handle PKCS#7 input). One difference that became immediately apparent in the form of a crash is that for the existing decoder, the arena parameter is really optional. If NULL is passed in, the old decoder would use PORT_ZAlloc for allocation of each item and subitem. This is so bad for performance that my decoder doesn't handle it at all - it requires an arena for any allocation that may be necessary in a very few cases, such as the pointer array for the SET OF / SEQUENCE OF case and SEC_ASN1_POINTER that I just implemented, where the substructure is allocated by the decoder. Unfortunately, there are plenty of places in NSS where temporary DER decodes are done, where no arena is used, and the decoded objects are subsequently freed with PORT_Free by the caller. These will require changes before they can use my new decoder. The quick fix is to create a temporary arena, use it for decoding, and then free the arena when done. I just implemented one such quick fix in secname.c to allow the decoding of certs to work. This was enough to make certutil work with my decoder, as well as NES ! It looks like my decoder can't quite be a plug-in replacement for the old one because of the difference in memory management. Even for callers who already pass an arena, there is still the issue of whether the DER input needs to be duplicated or not. Taking the conservative route of always duplicating it (as I did in my test) negates some of the memory saving benefits of the new decoder, although it will still use less memory than the old one even when doing that once at the top level DER, as the old decoder would make more than one copy - one DER copy at each subitem level ... Coupled with the fact that BER isn't handled, this means the new decoder deserves a new signature before it can go into NSS. I think it should probably take a PRArenaPool** in/out parameter instead of PRArenaPool*, so that it could create and return an arena if the caller didn't create one. Then there is the whole question of whether to make a copy of the DER input or not, which might deserve a PRBool. There are really 4 cases : a) Ideally, the caller would have allocated the DER in an arena, and would then pass the pointer to the buffer and the matching arena. ie. arenaptr = &derinputarena ; copyDER = PR_FALSE; b) DER input wasn't in an arena (or in an arena that has other things too). So the caller wants a new arena for the decoded object. But the caller doesn't want to duplicate the DER because it is large. Caller will be responsible for keeping the DER input around as long as the decoded object is around. Freeing of the object would consist of freeing the arena for the decoded part, and another caller-specific method for the input. Caller passes in arenaptr = &newarena ; copyDER = PR_FALSE; (where newarena = NULL) c) DER input wasn't in an arena. The caller wants to duplicate the DER so it can easily free the decoded object with a simple PORT_ArenaFree call. He needs to pass in arenaptr = &newarena; copyDER = PR_TRUE. (where newarena = NULL) d) the prototype would allows for a fourth case where arenaptr points to an existing arena yet copyDER is PR_FALSE . This is similar to case b) , but I can't think of any relevant use for this case . Most likely the arena passed in would be a brand new one .
Attached file update to the new DER ASN.1 decoder (obsolete) (deleted) —
This implements 99% of the ASN.1 DER specification and all our template functionality. Only SEC_ASN1_DYNAMIC is missing. A good prototype for the new decoder's public function still needs to be agreed upon
Attachment #93261 - Attachment is obsolete: true
Julien, If I understand you correctly, your decoder is functionally equivalent to the old decoder with only three exceptions. 1. The new decoder does not implement SEC_ASN1_DYNAMIC. 2. The new decoder does not copy DER. 3. The new decoder does not use PORT_Alloc if 'arena' is NULL. If SEC_ASN1_DYNAMIC is not hard to support, I suggest that we implement SEC_ASN1_DYNAMIC support in the new decoder and rip out the old decoder. I am worried about the code bloat resulting from having two decoders. It should be easy to modify the new decoder so that it uses PORT_Alloc if 'arena' is NULL. This feature is only needed to emulate the old ASN.1 decode functions. It should be easy to emulate the current ASN.1 decode functions with the new decoder. The current ASN.1 decode functions would first copy the whole DER input and then call the new decoder. NSS would call the new decoder API directly where appropriate. The new decoder API can also be exported. NSS clients that want to take advantage of the better performance and memory usage of the new decoder will need to modify their code to call the new decoder API.
I think we cannot rip out the old decoder because the new decoder only decodes DER. We still need a full BER decoder for PKCS#7 and PKCS#12.
Attachment #93386 - Attachment is obsolete: true
Attached file public header file defining the new DER decoder (obsolete) (deleted) —
You da man, Julien!
I have completed the implementation of the new quick DER decoder. Attachment #93532 [details] [diff] is ready for review. Keep in mind that it has only been tested on certs and CRLs so far. Some of the code paths may not have been exercised yet - in particular DecodeGroup, DecodePointer and DecodeChoice, though I believe the implementations are correct. I would appreciate in particular comments about the correctness of MatchItemType() and the decode part of DecodeItem(), since I can't be 100% sure that I handled all the cases correctly in them, although I looked at many template definitions and Lisa's code whenever in doubt.
Attached file quickder.h - header file defining new DER decoder (obsolete) (deleted) —
update adding support for debug facility through SEC_ASN1_DEBUG which can be specified in a template
Attachment #93533 - Attachment is obsolete: true
Attached file quickder.c - source for new DER decoder (obsolete) (deleted) —
update adding support for debug facility through SEC_ASN1_DEBUG which can be specified in a template also fixes a bug for constructed types
Attachment #93532 - Attachment is obsolete: true
Of course, 10 minutes after I had posted the source for review, I found a bug in the code which caused the CRL extensions never to be decoded, though the decode was still passing as they are optional. This was working a few days ago while I had the hybrid decoder, but not aymore with the full implementation, so I had to add a little extra code for constructed types. While debugging this problem, I also added a new flag for templates, SEC_ASN1_DEBUG, which can be inserted into a decoding template for debugging purposes and greatly simplifies the task of chasing through the code in the unlikely event the decoder wouldn't perfectly decode everything <cough cough>.
Just to be clear, attachments 93546 / 93547 contain the fixes I just did and are the ones that should be reviewed. I just obsoleted all the previous ones since I assumed nobody started reviewing them, but if anyone did already, the difference is very minor, just that one constructed bug and the SEC_ASN1_DEBUG support.
Attached file HTML of CRL decoding performance data (deleted) —
This page shows various times in seconds for CRL decodes, both full and partial, on 3 different platforms, comparing the use of the old decoder with the new one. The very precise times were obtained by adding NSPR timing code around the cert decode functions. All testing was performed with crlutil. These tests were repeated with 3 different size of CRLs : 1) the 26 MB test case from broom that I had been using 2) the 300 KB Thawte Freemail CRL 3) the 30 KB corporate CRL In all the cases, major reductions of decoding time were observed. On optimized builds, the new decoder shows improvements of 3.5x to 6x on the smallest CRL, and 8x to 9x on the largest CRL. On debug builds, the improvements range from 4x to 6x on the smallest CRL, and 18x to 33x on the largest CRL.
SEC_ASN1_SKIP only worked together with a type or SEC_ASN1_OPTIONAL . Provide capability to skip a required field without providing its type. This is needed for compatibility with old template syntax SEC_ASN1_SKIP_REST was implemented but never got tested, and it didn't work. Fixed it
Attachment #93547 - Attachment is obsolete: true
Comment on attachment 93546 [details] quickder.h - header file defining new DER decoder Review comments of quickder.h. 1. Add the copyright/license header. 2. Move the overview comments in quickder.c to this file. 3. Document the new SEC_QuickDERDecodeItem function. 4. The definition of the SEC_ASN1_DEBUG macro probably should be moved to secasn1t.h, which defines all other SEC_ASN1_XXX macros. Probably should name this SEC_ASN1_DEBUG_BREAK. (Usually XXX_DEBUG means "enable the debug code for the XXX subsystem.")
Attachment #93546 - Flags: needs-work+
Comment on attachment 93534 [details] [diff] [review] makefile patch to build quick DER decoder as part of lib/util Review comments of the makefile patch. If you want to export SEC_QuickDERDecodeItem, you also need to add it to lib/nss/nss.def. If you don't want to export the new function in NSS 3.6, add quickder.h to PRIVATE_EXPORTS instead so that the header file won't be shipped to our customers. It may be a good idea to gain more experience with the new function before exporting it.
Attachment #93534 - Flags: needs-work+
Attachment #93262 - Attachment is obsolete: true
Attachment #93546 - Attachment is obsolete: true
Attachment #93688 - Attachment is obsolete: true
Attachment #93690 - Attachment is patch: true
This bug was getting too crowded with attachments and comments, so I have created a separate bug #161215 for the new DER decoder. This bug will depend on it. The rest of this bug should be only to discuss partial CRL decoding and the small patches associated with it.
Depends on: 161215
Attachment #93690 - Attachment is obsolete: true
Attachment #93534 - Attachment is obsolete: true
This supports partial decoding by skipping entries. This is done using the new quick DER decoder.
Patches are checked in to the NSS 3.6 tip.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: