Closed
Bug 149816
Opened 23 years ago
Closed 23 years ago
Need partial CRL DER decoding for performance
Categories
(NSS :: Libraries, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
3.6
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.
Assignee | ||
Comment 1•23 years ago
|
||
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.
Assignee | ||
Comment 2•23 years ago
|
||
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.
Assignee | ||
Comment 3•23 years ago
|
||
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 ?
Comment 4•23 years ago
|
||
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.
Assignee | ||
Comment 5•23 years ago
|
||
Really taking bug
Assignee: wtc → jpierre
Priority: -- → P1
Target Milestone: --- → 3.6
Assignee | ||
Comment 6•23 years ago
|
||
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.
Assignee | ||
Comment 7•23 years ago
|
||
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.
Comment 8•23 years ago
|
||
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
Assignee | ||
Comment 9•23 years ago
|
||
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.
Comment 10•23 years ago
|
||
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.
Assignee | ||
Comment 11•23 years ago
|
||
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 ...
Assignee | ||
Comment 12•23 years ago
|
||
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 ?
Comment 13•23 years ago
|
||
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.
Assignee | ||
Comment 14•23 years ago
|
||
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.
Assignee | ||
Comment 15•23 years ago
|
||
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.
Assignee | ||
Comment 16•23 years ago
|
||
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.
Assignee | ||
Comment 17•23 years ago
|
||
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 }
};
Assignee | ||
Comment 18•23 years ago
|
||
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
Assignee | ||
Comment 19•23 years ago
|
||
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.
Assignee | ||
Comment 20•23 years ago
|
||
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 ...
Assignee | ||
Comment 21•23 years ago
|
||
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 ?
Assignee | ||
Comment 22•23 years ago
|
||
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.
Comment 23•23 years ago
|
||
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
Assignee | ||
Comment 24•23 years ago
|
||
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.
Assignee | ||
Comment 25•23 years ago
|
||
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.
Assignee | ||
Comment 26•23 years ago
|
||
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.
Assignee | ||
Comment 27•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Attachment #92322 -
Attachment is obsolete: true
Assignee | ||
Updated•23 years ago
|
Attachment #92555 -
Attachment is obsolete: true
Assignee | ||
Updated•23 years ago
|
Attachment #92993 -
Attachment is obsolete: true
Assignee | ||
Comment 28•23 years ago
|
||
Assignee | ||
Comment 29•23 years ago
|
||
Assignee | ||
Comment 30•23 years ago
|
||
Assignee | ||
Comment 31•23 years ago
|
||
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.
Assignee | ||
Comment 32•23 years ago
|
||
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 .
Assignee | ||
Comment 33•23 years ago
|
||
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
Assignee | ||
Updated•23 years ago
|
Attachment #93261 -
Attachment is obsolete: true
Comment 34•23 years ago
|
||
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.
Comment 35•23 years ago
|
||
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.
Assignee | ||
Comment 36•23 years ago
|
||
Attachment #93386 -
Attachment is obsolete: true
Assignee | ||
Comment 37•23 years ago
|
||
Assignee | ||
Comment 38•23 years ago
|
||
Comment 39•23 years ago
|
||
You da man, Julien!
Assignee | ||
Comment 40•23 years ago
|
||
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.
Assignee | ||
Comment 41•23 years ago
|
||
update adding support for debug facility through SEC_ASN1_DEBUG which can be
specified in a template
Attachment #93533 -
Attachment is obsolete: true
Assignee | ||
Comment 42•23 years ago
|
||
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
Assignee | ||
Comment 43•23 years ago
|
||
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>.
Assignee | ||
Comment 44•23 years ago
|
||
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.
Assignee | ||
Comment 45•23 years ago
|
||
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.
Assignee | ||
Comment 46•23 years ago
|
||
Attachment #93260 -
Attachment is obsolete: true
Assignee | ||
Comment 47•23 years ago
|
||
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 48•23 years ago
|
||
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 49•23 years ago
|
||
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+
Assignee | ||
Updated•23 years ago
|
Attachment #93262 -
Attachment is obsolete: true
Assignee | ||
Updated•23 years ago
|
Attachment #93546 -
Attachment is obsolete: true
Assignee | ||
Updated•23 years ago
|
Attachment #93688 -
Attachment is obsolete: true
Assignee | ||
Updated•23 years ago
|
Attachment #93690 -
Attachment is patch: true
Assignee | ||
Comment 50•23 years ago
|
||
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
Assignee | ||
Updated•23 years ago
|
Attachment #93690 -
Attachment is obsolete: true
Assignee | ||
Updated•23 years ago
|
Attachment #93534 -
Attachment is obsolete: true
Assignee | ||
Comment 51•23 years ago
|
||
This supports partial decoding by skipping entries. This is done using the new
quick DER decoder.
Assignee | ||
Comment 52•23 years ago
|
||
Assignee | ||
Comment 53•23 years ago
|
||
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.
Description
•