See bug 240661 comment 9 and bug 240661 comment 10.

This conflicts with the patches in bug 240661, but oh, well. I'll merge in either direction as needed.

Comment on attachment 146528 [details] [diff] [review] Same as diff -b, FOR REVIEW ONLY John, would you review? Nelson, if you have time to take a look, that would also be much appreciated... Who'd be a good sr for changes to this code?

Comment on attachment 146528 [details] [diff] [review] Same as diff -b, FOR REVIEW ONLY I don't think the fingerprint can have non-ASCII characters, so full UTF-8 conversion isn't absolutely necessary there.

Comment on attachment 146528 [details] [diff] [review] Same as diff -b, FOR REVIEW ONLY Brendan, would you sr?

Taking...

Several suggestions about this patch: 1. I think you should ignore the return value from Update, and base your decision entirely on the return value from Finish. I do not know that it can never be true that update returns an error bug finish also returns a non-null value, so I think the assertion might conceivably hit. 2. You could replace the sequence of calls to the decoder (start, update, finish) with a single call to SEC_PKCS7DecodeItem(). 3. NSS has an old PKCS7 decoder, and a new CMS decoder. CMS is a major new revision of the old PKCS7 standard. It allows signatures to contain a superset of the things that were allowed in PKCS7. AFAIK, the function being patched here is that last remaining use of the old PKCS7 decoder in mozilla. All other calls have been converted to use the CMS decoder, IINM. So, it might be good to start using the new NSS CMS decoder in this function, rather than continuing to use the old PKCS7 decoder However, the conversion may be non-trivial. The CMS decoder function that is closest to SEC_PKCS7DecodeItem() is NSS_CMSMessage_CreateFromDER() (which should have been named FromBER). So, the suggestion to convert to the CMS decoder should probably be in another RFE.

Comment on attachment 146528 [details] [diff] [review] Same as diff -b, FOR REVIEW ONLY Yeah, item 3 sounds like a little more than I'm willing to do given my general lack of knowledge in this code.... I'll work on doing item 2 (which obviates item 1 if I read correctly).

Hmm.. Using SEC_PKCS7DecodeItem() means shoehorning my data into a SECItem, which means converting my |const char*| to |unsigned char*|. I guess I could just reinterpret_cast it.... :(

Comment on attachment 146632 [details] [diff] [review] Same as diff -b, for review only I went with siEncodedCertBuffer as the type, but that could well be totally wrong. If someone knows what the actual type is (not that SEC_PKCS7DecodeItem cares), please tell me....

r=MisterSSL for the diffs between the two patches. There are very few places where si.type is used. I think you made the right choice.

Comment on attachment 146632 [details] [diff] [review] Same as diff -b, for review only brendan, could you please sr?

Comment on attachment 146632 [details] [diff] [review] Same as diff -b, for review only sr=brendan@mozilla.org /be

Checked in on the trunk for Mozilla 1.8a.