The default bug view has changed. See this FAQ.

modutil -add prints no error explanation on failure

RESOLVED FIXED in 3.12.2

Status

NSS
Tools
P2
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Elio Maldonado)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 5 obsolete attachments)

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.
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
Priority: -- → P2
Version: 3.9 → 3.2
(Assignee)

Comment 2

9 years ago
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.
(Assignee)

Comment 3

9 years ago
Created attachment 331746 [details] [diff] [review]
Use NSS's functions for error reporting

Proposed simple fix attached.
Assignee: nobody → emaldona
Status: NEW → ASSIGNED
(Assignee)

Updated

9 years ago
Attachment #331746 - Flags: review?(nelson)
(Assignee)

Updated

9 years ago
Attachment #331746 - Attachment description: NSS's working functions that prints use NSS's functions for error reporting → Use NSS's functions for error reporting
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.
Attachment #331746 - Flags: review?(nelson) → review+
(Assignee)

Comment 5

9 years ago
Created attachment 331822 [details] [diff] [review]
Use NSS error functions yet preserve detailed error.

Yes, is good to preserve the better error message. Thanks. New patch attached.
Attachment #331746 - Attachment is obsolete: true
Attachment #331822 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #331822 - Flags: review? → review?(nelson)
(Assignee)

Updated

9 years ago
Attachment #331822 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Updated

9 years ago
Attachment #331822 - Attachment is patch: true
(Assignee)

Comment 6

9 years ago
Created attachment 331825 [details] [diff] [review]
resending renamed as previous doesn't show good diff
Attachment #331825 - Flags: review?(nelson)
(Assignee)

Updated

9 years ago
Attachment #331822 - Attachment is obsolete: true
Attachment #331822 - Flags: review?(nelson)
Comment on attachment 331825 [details] [diff] [review]
resending renamed as previous doesn't show good diff

r=nelson
Attachment #331825 - Flags: review?(nelson) → review+
(Assignee)

Comment 8

9 years ago
Nelson, r=nelson, was this an r+?
(Assignee)

Updated

9 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.12.2
This checkin broke the automated tests on several platforms. 
See http://tinderbox.mozilla.org/showbuilds.cgi?tree=NSS
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 10

9 years ago
Changed: pk11.c v1.26, error.h v 1.6. Investigating tests that started failing.
(Assignee)

Comment 11

9 years ago
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.
(Assignee)

Updated

9 years ago
Depends on: 454716

Comment 12

9 years ago
Created attachment 338036 [details] [diff] [review]
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.
Attachment #338036 - Flags: review?(emaldona)
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

9 years ago
Created attachment 338041 [details] [diff] [review]
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.
Attachment #338036 - Attachment is obsolete: true
Attachment #338041 - Flags: review?(emaldona)
Attachment #338036 - Flags: review?(emaldona)
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.
Backed out change to cmd/modutil/error.h
Checking in error.h; new revision: 1.7; previous revision: 1.6
(Assignee)

Updated

9 years ago
Attachment #338041 - Flags: review?(emaldona) → review+
(Assignee)

Updated

9 years ago
Duplicate of this bug: 454716

Comment 18

9 years ago
Created attachment 338136 [details] [diff] [review]
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
Attachment #331825 - Attachment is obsolete: true
Attachment #338041 - Attachment is obsolete: true

Comment 19

9 years ago
I also added the block comment that Nelson suggested in comment 15.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.