220217 - modutil -rawlist falsely reports a module spec when none exists

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

The command
   modutil -rawlist -dbdir /DoesNotExist -nocertdb
should report an error, namely that no secmod.db file exists in /DoesNotExist.
But instead, modutil reports one module spec, as if the secmod.db did exist
and contained a spec for the NSS internal module.  

This has caused LOTS of wasted time.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

modutil has code to explicitly check that the dbdir exists and is readable
and that the secmod.db exists and is readable.

The problem appears to be that the calls to the "raw" command code
were inserted into the main function BEFORE (ahead of) any of those checks.

So, whether the files and/or directories exist or not, main calls 
RawListModule(), which calls SECMOD_GetModuleSpecList().  

Except for no memory or null input pointer errors, SECMOD_GetModuleSpecList
always returns a list of Module Specs that contains at least one module spec,
the one for the softoken module, even when no secmod.db exists.  NSS_Init and
NSS_NoDBInit depend on that behavior.  The ability to run NSS programs even
when no secmod.db exists depends on that behavior.  

So, the solution is not to make SECMOD_GetModuleSpecList return NULL when
secmod.db does not exist and cannot be created.  Rather, it is for the 
modutil application to check for valid files and directories before calling
SECMOD_GetModuleSpecList().  

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: diffs v1

Here is a synopsis of this set of diffs:

1. split the function init_crypto into two functions that may be called 
sequentially:  check_crypto and init_crypto.  

2. restore to check_crypto the functionality, previously ifdeffed out, that
checks the secmod db file to see if it is present, readable or writable,
as needed.  Note this is only done for secmod, not for cert and key.
NSS libaries really should offer a function to do this, but for now,
I think it is OK for modutil to house this code.

3. move the code around in main, so that the checks for the secmod file are
done first, then the raw functions are done (if selected), and if not,
then NSS is initialized and the other functions are done.  

4. Fix a null pointer dereference crash.
5. Double space the output of the -rawlist function, so that we can more 
easily see where one spec ends and the next begins.
6. some whitespace cleanup.

Note that these diffs are not a "patch" per se'.  These files intend that
tabs be every 4 spaces, but since the diffs viewed here will have tabs set
to 8 spaces, they will be quite hard to read.  So, I processed these diffs
to appear as they would with tabstops set to 4.  The result is Much easier
to read and review, but will not work as a patch.

all.sh passes with these changes, and modutil -rawlist detects missing 
secmod.db files.  

Tomorrow I will attach another patch to this bug, one which fixes some null
pointer crashes in NSS libs, which I found while developing this patch.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 132184 [details] [diff] [review]
diffs  v1

reviewers, please read synopsis of this patch in the bug report before
reviewing.

Comment 4

[Robert Relyea]

Sigh.

There is a fundamental problem having the application check the databases.

That problem is that application may not really know where the databases are and
what they are called. 
The database checks were commented out because the failed for 2 cases since NSS
3.3: 1) the case where we rev'ed to cert8.db, and 2) the case where you want to
access the multi-access database. 

We can handle make both of these cases work, but we are violating our Binary
API. It might be OK for modutil, but we need to make sure modutil is not part of
the backwards compatibility test, and make strong comments in the code about not
doing this in your own application.

bob

Comment 5

[Robert Relyea]

OK, on reviewing the patch more closely, this only affects secmod.db
specifically, not the other databases. We would still break if we renamed
secmod.db, but maybe that's OK for modutil? We still need to skip the check if
dirname begins with "MULTIACCESS:" or modutil will fail to connect to
multiaccess databases.

bob

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Attachment: diffs v2

This set of diffs incorporates Bob's feedback concerning "multiaccess:"
(which must be lower case).  It also removes all the remaining ifdeffed code
that attempts to use the names of the cert and key dbs.  The only name that
this code now uses is the name of the secmod DB.

The right way to do this is for NSS to provide a function to check the 
necessary directories and files, and perhaps return pathnames when there is
a problem.  I will file a separate RFE about that.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 132224 [details] [diff] [review]
diffs  v2

Reviewers, Please read the bug comments before reviewing this patch. Thanks.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Attachment: related patch to NSS libs v1

I found potential crashes in the code that returns a list of module specs,
and in several places that call it.  All crashes due to not checking for
NULL pointers.	This patch fixes what I found.	NSS_Init (and its siblings)
now return an error rather than success when the list is NULL or empty.
All.sh passes with this patch.

Comment 9

[Robert Relyea]

Comment on attachment 132229 [details] [diff] [review]
related patch to NSS libs  v1

There are some more failed for NULL checks in SECMOD_LoadModule(), but they
preexist this patch.

bob

Comment 10

[Robert Relyea]

Comment on attachment 132224 [details] [diff] [review]
diffs  v2

r=relyea looks good...

Comment 11

[Wan-Teh Chang]

Assigned the bug to Nelson.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Marking P2

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Checked in on trunk.  Resolved/fixed.

/cvsroot/mozilla/security/nss/cmd/modutil/error.h,v  <--  error.h
new revision: 1.4; previous revision: 1.3

/cvsroot/mozilla/security/nss/cmd/modutil/modutil.c,v  <--  modutil.c
new revision: 1.20; previous revision: 1.19

/cvsroot/mozilla/security/nss/cmd/modutil/pk11.c,v  <--  pk11.c
new revision: 1.19; previous revision: 1.18

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Checkin second patch above.  

/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pars.c,v  <--  pk11pars.c
new revision: 1.14; previous revision: 1.13

/cvsroot/mozilla/security/nss/lib/softoken/pk11db.c,v  <--  pk11db.c
new revision: 1.26; previous revision: 1.25

Comment 15

[Robert Relyea]

Comment on attachment 132184 [details] [diff] [review]
diffs  v1

r+ already given to updated patches.