129080 - Unable to import large CRLs into cert7.db

Status: RESOLVED FIXED

(NSS :: Build, defect, P1)

Description

[Jim McCarthy]

I am trying to import a CRL into the cert7.db using CERT_ImportCRL().  The 
function returns a valid CERTSignedCrl structure but I cannot read the CRL out 
of the cert7.db.  I am using the SEC_LookupCrls function to get a 
CERTCrlHeadNode structure.  The function returns a valid structure but the 
first node in the structure is null.

I have 2 CRLs I'm testing with; one is 108KB and the other one is 36KB.  The 
smaller CRL works fine; the larger one is the one I'm having problems with.

I created the cert7.db using the certutil that came with 3.3.1.  I also tried 
to do this with the crlutil that comes with 3.3.1, but I'm getting the same 
error.  Here is what I'm doing with crlutil:

# crlutil -I -i chamb.crl -t 1 -d . (108KB CRL - no errors reported)
# clrutil -L -d .
CRL names                                CRL Type

# crlutil -I -i denver.crl -t 1 -d . (36KB CRL - no errors reported)
# clrutil -L -d .
CRL names                                CRL Type
CN=DOD CLASS 3 CA-4, OU=PKI, OU=DoD, O=U.S. Government, C=US CRL

Is there still a problem with the CRL size?

Comment 1

[Wan-Teh Chang]

Assigned the bug to Bob.

Comment 2

[Robert Relyea]

Yes there is a bug in NSS 3.3 and earlier when CRL's > 64K would not be
visible in the database after loading.

This has been fixed in NSS 3.3.1 and later. I just verified that NSS 3.4 works
correctly. pre-NSS 3.4, crlutil is statically bound, so it may be possible that
an old crlutil is being used?

bob

Comment 3

[Wan-Teh Chang]

This problem with loading CRL's > 64K was fixed as PSM bug 74343.
The date it was marked VERIFIED was 2001-06-22, so it is definitely
fixed in NSS 3.3.1 and may even be fixed in NSS 3.3.

Comment 4

[Wan-Teh Chang]

I verified that bug 74343 was fixed in NSS 3.3.

Comment 5

[Robert Relyea]

OK, the problem is DBM cannot return an object bigger than it's cache. There are
two options for fixing this problem:

1) increase the size of the cache when we open the file in NSS. (change to NSS)
2) change DBM to increase it's buffersize while reading in a file by locking the
buffer pages. (change to dbm).

Both changes would require a respin of nss3.dll and certain tools (3.3.x) and
softokn3.dll and tools (3.4).

Change 1 has the advantages of :
   Not requiring a rebuild of dbm.
   Does not change the semantics of dbm for other dbm users.
   Risk of unattended side effects are small.

Change 2 has the advantages of:
   Will work with any database objects of arbitarially large size (limitted by
application memory).
   Users which do not have large CRL's do not pay the dbm cache penalty.

I'm attaching 3 separate patches:
Patch a: change 1 for 3.3.x
Patch b: change 1 for 3.4
Patch c: change 2 for mozilla dbm.

bob

Comment 6

[Robert Relyea]

Attachment: Option 1, nss3.3 version

Note pcertdb.c is in mozilla/security/nss/lib/certdb in nss 3.3

Comment 7

[Robert Relyea]

Attachment: Option 1: nss 3.4 version

NOTE: pcertdb is in mozilla/security/nss/lib/softoken in nss 3.4

Comment 8

[Robert Relyea]

Attachment: Option 2; Mozilla dbm patch

Comment 9

[Robert Relyea]

Adding Nelson to review (patches for option 1).

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Patch review comments:

Patch id 74774 increases the sizes of the DB buffer pools to 1/2 megabyte 
each for both the temporary and permanent cert DBs.  It appears to be 
correct, given what it is attempting.  However, I believe that most NSS 
clients would find the increase in memory allocation objectionable.
It's probably ok for servers.

Patch id 74775 contains some changes that are not obviously related to
the CRL size issue.  Also, it doesn't declare the new HASHINFO to be const.
It really should declare that to be const.  Otherwise, it seems correct.
This patch changes the size of the perm cert DB's memory buffer pool to 
1/2 megabyte, and therefore is probably objectionable for the browser.  
Also, is 1/2 megabyte enough for the servers for the forseeable future?

Patch id 74777 requires more review than its size suggests.

Comment 11

[Robert Relyea]

I've checked in the NSS 3.3 version of the patch.

A better solution for both clients and servers would be to fix the dbm collect()
code so that it does not use the entire buffer pool to read in large objects, or
can dynamically increase the buffer pool size when reading large objects. We'll
work this idea for NSS 3.4.

Comment 12

[Robert Relyea]

Attachment: Update DBM so it can handle files bigger than it's existing cache

This patch does not prevent the cache from growing when it encounters large
objects. Flushing out the buffers has some unintended side effects, including
some  very long write times. The patch does pin all the buffers that it needs
to pin, that is because it is possible for a random buffer the cause another
buffer on the list (not necessarily on the LRU) to get freed because it
believes that buffer is part of it's overlay tree. There is now code in the
overlay stuff that prevents buffers that are pinned from being freed.

Comment 13

[Robert Relyea]

Attachment: rest of changes for patch 75309

Oops, the buffer stuff I described in comment 12 is in hash_buf.c, included in
this patch.

Comment 14

[Robert Relyea]

Attachment: Arrn NFS messed me up...

This *should* be the correct patch.

Comment 15

[Jim McCarthy]

Will these new patches be making their way into version 3.4?  I see that the 
RC1 version was just released.

Comment 16

[Robert Relyea]

Yes, they are targetting for the final release of NSS 3.4

Comment 17

[Robert Relyea]

Attachment: Fix comments and indentation from previous patch

Comment 18

[Wan-Teh Chang]

Attachment: Fix more comments and indentation. Declare 'save_flags' as 'char'.

I fixed a few typographical errors in the comments,
and changed spaces to tab characters in two places.
(DBM sources indent with tab characters.)  I also declare
'save_flags' as 'char' because that's the type of the
'flags' field in struct _bufhead (BUFHEAD).

I compiled DBM with gcc on Linux and MSVC 6.0 on Windows
and found that Bob's new code only introduced one warning:
  'bp' might be used uninitialized in 'collect_data'.
I reviewed the code and verified that this warning is
benign, that is, 'bp' is never used uninitialized.

Comment 19

[Wan-Teh Chang]

I ran the 'lots' test program (mozilla/dbm/tests/lots.c)
under BoundsChecker with both "-quick" and "-quick -large"
command line options.  No memory leaks or memory access
errors were reported in DBM.

Comment 20

[Wan-Teh Chang]

Attachment: Combined patch for easier review

I combined the previous patches into this patch.

Nelson, Bob, could you review this patch and see
if any additional changes need to be made?  Thanks.

Comment 21

[Wan-Teh Chang]

All patches have been checked in.  Marked the bug fixed.

Comment 22

[Wan-Teh Chang]

Attachment: Change 'save_flags' back to 'int'

It was pointed out to me that Bob's use of 'int' for
local variables containing a char is actually the C
way.  So I changed the type of 'save_flags' back to
'int'.

Comment 23

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) approval for checkin into 1.0, pending second review.

Comment 24

[Wan-Teh Chang]

Comment on attachment 75643 [details] [diff] [review]
Combined patch for easier review

r=wtc.	(Nelson has already reviewed this patch.)

Comment 25

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 75643 [details] [diff] [review]
Combined patch for easier review

a=shaver for the 1.0 branch.

Comment 26

[Asa Dotzler [:asa]]

a=asa (on behalf of drivers) for checkin to the 1.0 branch (re-approval)

Comment 27

[Wan-Teh Chang]

I checked in the patch (with more comments) on the MOZILLA_1_0_0_BRANCH
and the tip of mozilla/dbm.

Comment 28

[Asa Dotzler [:asa]]

adding fixed1.0.0 keyword (branch resolution). This bug has comments saying it
was fixed on the 1.0 branch and a bonsai checkin comment that agrees. To verify
the bug has been fixed on the 1.0 branch please replace the fixed1.0.0 keyword
with verified1.0.0.