447563 - modutil -add prints no error explanation on failure

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

When the modutil -add command fails, modutil tells the user that it failed,
but does not print the text of any NSS or NSPR error code that might explain
the failure.  This is because the code in that one error path in modutil uses
a technique to get the error text that simply does not work and never has.  See
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/cmd/modutil/pk11.c&rev=1.26&mark=295-297#286

The short term solution is to use one of NSS's working functions that prints 
NSS and NSPR error codes here, rather than this non-working technique.
Modutil should print error codes the same way that all other NSS programs do.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Bug present since rev 1.10 in NSS 3.2
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/cmd/modutil/pk11.c&rev=1.10&mark=244-247#236

Comment 2

[Elio Maldonado]

The problem seems to be shared among modutil and p11wrap.  modutil's pk11.c relies on PR_GetErrorText(errtxt) after calling SECMOD_AddNewModuleEx which which in turns uses PR_SetErrorText(0, NULL);  Searching on _SetError nowhere else inside nss/lib I see that SetErrorText is used only by SECMOD_AddNewModuleEx. The rest of nss relies on either nss_SetError or on PORT_SetError. I am not sure I understand when one is supposed to nss_SetError and when PORT_SetError.

Comment 3

[Elio Maldonado]

Attachment: Use NSS's functions for error reporting

Proposed simple fix attached.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 331746 [details] [diff] [review]
Use NSS's functions for error reporting

Elio, In email, you expressed a desire to retain the use of the 
PR_GetErrorText code path, because it may give slightly better errors in 
certain cases.  If you want to do that, you could just change the patch 
so that it merely replaces the code in the path where PR_GetErrorText 
produced no output:

>-	    PR_fprintf(PR_STDERR, errStrings[ADD_MODULE_FAILED_ERR], 
>-		       moduleName);
>+	    PR_fprintf(PR_STDERR, errStrings[ADD_MODULE_FAILED_STATUS_ERR],
>+		       moduleName, SECU_Strerror(PORT_GetError()));

If you want to do that, please submit another patch that does that.

Whether you do that, or leave this patch as it is, the patch will remove 
one use of the error string ADD_MODULE_FAILED_ERR, replacing it with a 
use of the error string ADD_MODULE_FAILED_STATUS_ERR.
If there are no remaining uses of ADD_MODULE_FAILED_ERR, then we should 
get rid of that string, too, IMO.  In that case, you could attach an 
additional patch to do that.

Comment 5

[Elio Maldonado]

Attachment: Use NSS error functions yet preserve detailed error.

Yes, is good to preserve the better error message. Thanks. New patch attached.

Comment 6

[Elio Maldonado]

Attachment: resending renamed as previous doesn't show good diff

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 331825 [details] [diff] [review]
resending renamed as previous doesn't show good diff

r=nelson

Comment 8

[Elio Maldonado]

Nelson, r=nelson, was this an r+?

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

This checkin broke the automated tests on several platforms. 
See http://tinderbox.mozilla.org/showbuilds.cgi?tree=NSS

Comment 10

[Elio Maldonado]

Changed: pk11.c v1.26, error.h v 1.6. Investigating tests that started failing.

Comment 11

[Elio Maldonado]

The new test failures are caused by hard-coded values in the fips dbtests scripts. With this fix pk11 now returns a different error code and one unused error code was removed from err.h. The dbtest application in turn returns a different error code (one less than before) but the dbtests.sh and fips.sh scripts which call this app have the expected error code hard-coded to 46. It's now 45.  I have a suggested patch for dbtests.sh fips.sh that should fix this failures.  I'll open a bug on the other two shell scripts and attach the patches. We could also back off this fix for tonight until the other solution is in.

Comment 12

[Wan-Teh Chang]

Attachment: Preserve the values of error codes

We should avoid changing the exit codes of our command-line
tools.  In this case, we can accomplish this with this simple
patch.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

OK, so the problem is actually the removal of an error code from 
modutil/error.h.  Backing that out will fix the tests and preserve the
good change to pk11.c.  

Wan-Teh pointed out that by not deleting members from that enumeration,
we assure that the values of the enumeration constants will not change.

Another way to fix the problem in error.h is to assign hard-coded numbers
to the members of the enumeration, so that when items are deleted from the
enumeration, the values of the other enumeration constants don't change, e.g.

 typedef enum {
   NO_ERR                         = 0,
   INVALID_USAGE_ERR              = 1,
   UNEXPECTED_ARG_ERR             = 2,
   UNKNOWN_OPTION_ERR             = 3,
   MULTIPLE_COMMAND_ERR           = 4,
   OPTION_NEEDS_ARG_ERR           = 5,

and so on.  Then when we delete an entry such as ADD_MODULE_FAILED_ERR
the values of the subsequent entries will not change, e.g .

   INVALID_CONSTANT_ERR           = 44,
   ADD_MODULE_FAILED_STATUS_ERR   = 46,

Comment 14

[Wan-Teh Chang]

Attachment: Preserve the values of error codes, v2

I did another cleanup.

Since there are no longer two flavors of the
"add module failed" error, we don't need the
_STATUS in the error code name to tell them
apart.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

There should also be a block comment at the beginning of the enumeration explaining that the values of these enumerated constants are immutable,
and must not be changed.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Backed out change to cmd/modutil/error.h
Checking in error.h; new revision: 1.7; previous revision: 1.6

Comment 18

[Wan-Teh Chang]

Attachment: Patch as checked in

This patch was diff'ed against the versions of the files
before Elio's checkin, so that we can see the overall changes
in one place.

I checked it in on the NSS trunk (NSS 3.12.2).

Checking in error.h;
/cvsroot/mozilla/security/nss/cmd/modutil/error.h,v  <--  error.h
new revision: 1.8; previous revision: 1.7
done
Checking in pk11.c;
/cvsroot/mozilla/security/nss/cmd/modutil/pk11.c,v  <--  pk11.c
new revision: 1.29; previous revision: 1.28
done

Comment 19

[Wan-Teh Chang]

I also added the block comment that Nelson suggested in comment 15.