66472 - NSS should have only one function that generates error messages

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

NSS seems to have several functions that return error text strings 
for NSS's error codes.  

SECU_Strerror() in cmd/lib/secerror.c returns a pointer to an const
error string.  It returns strings for all known SEC_ SSL_ and NSPR
error codes.  It returns NULL for unknown error codes.  In my opinion
this function should be the ONLY source of error strings in NSS.

cmd/lib/secutil.c also contains several other functions for returning
error strings, including SECU_ErrorString(), SECU_ErrorStringRaw(),
and SECU_GetString(), and others.  These all use a comon static 
buffer to hold error messages, and so none of them is thread safe.
SECU_ErrorStringRaw provides strings for a small subset of the SEC_
and SSL_ error codes, and none for the NSPR error codes.  It's 
error messages are very terse and potentially ambiguous.
These functions are presently used in the following source files
in nss/cmd/...

lib/secutil.c
modutil/install.c
modutil/instsec.c
pk12util/pk12util.c
signtool/sign.c
signtool/util.c
signtool/verify.c
signver/signver.c
sslstrength/sslstrength.c

I thing that SECU_ErrorStringRaw should be deleted and functions
that use it should be converted to use SEC_Strerror instead.
Also, it would be good to make the functions in secutil be 
thread safe.

Comment 1

[Wan-Teh Chang]

Assigned the bug to Kirk.

Kirk, didn't you already add such a function
(SECU_PrintPRandOSError) in bug #66244?  If
so, you just need to make sure that all the command
line utilities on Nelson's list use this new
function.

Comment 2

[Wan-Teh Chang]

Changed the QA contact to Bishakha.

Comment 3

[Wan-Teh Chang]

Set target milestone to NSS 3.5.

Comment 4

[Kirk Erickson]

Set target milestone to NSS 3.6.

Comment 5

[Wan-Teh Chang]

Moved to target milestone 3.8 because the original
NSS 3.7 release has been renamed 3.8.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Remove target milestone of 3.8, since these bugs didn't get into that release.

Comment 7

[Kirk Erickson]

Target Milestone: 3.9

Comment 8

[Kirk Erickson]

Retargeted Future.  Plan to address Sun issues first.

Comment 9

[Chris Newman]

The right way to fix this is to have NSS_Init install error code translations
into NSPR via PR_ErrorInstallTable, and then use the PR_ErrorToString() API for
its intended purpose.  An NSPR socket with NSS installed under it will cause
NSPR APIs to return SSL errors, so PR_ErrorToString should have the strings for
those errors installed.

This is also important because when the error to string tables are not in the
library that generates the error, then the numbers can get out of sync.  For
example, Mozilla libsslldap library has a copy of the errors and I just had to
put another copy in the async I/O layer we use for Messaging Server at Sun.  As
NSS adds new errors, these tables won't be updated automatically, so it would be
better if the tables were in libnss.so and not duplicated in everything that
calls it.

Comment 10

[Chris Newman]

Attachment: Example of what NSPR error table for SEC errors looks like

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Chris, thanks for your contribution!  

I don't think NSS_Init should install these values, but I agree the 
values should be available for NSS-based programs to use.  

Perhaps you would care to contribute a patch that shows the conversion
of a small NSS utility command, such as pk12util/pk12util.c to 
a) install error code translations into NSPR via PR_ErrorInstallTable, 
   (say, just before the call to NSS_Init) and then 
b) use the PR_ErrorToString() API for its intended purpose. 

Comment 12

[Chris Newman]

I'm very busy at work and NSS is not my direct responsibility.  In order for me
to make time to build a full patch, I would have to get something out of it. 
The benefit I want only happens if the error tables are folded into the NSS
shared objects so I no longer have to maintain a separate error table in my
layered code  If there is agreement to do that, then I'm willing to do the
integration work as a patch (including all tools and the core libraries).  If we
don't have NSS_Init install the table, then we need to export the table itself
as a symbol so the caller can install it -- I'm fine with either approach.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Attachment: alternative patch - maintenance free

Chris,

This patch creates two tables, one for SEC errors and one for SSL errors.
It also includes a little function that installs those tables into NSPR's
error mechanism.  
It does NOT duplicate the contents of SECerrs.h or SSLerrs.h, but rather
includes them, so as those tables grow, there is no need to perform 
maintenance on this source file.  
Is this what you want?

If so, please contribute a small patch showing how any of the NSS 
utility commands might make use of this.  Thanks.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 141848 [details] [diff] [review]
alternative patch - maintenance free

I have added a large patch to bug 172051 that creates a 
function that registers the NSS error strings with NSPR.
This new function becomes part of libNSS.  

I decided to include the SSL error strings in libNSS, and
have one function that registers both NSS and SSL error codes,
because NSS uses SSL error codes in places that are not 
necessarily SSL errors.  

The patch changes the common SECU_ error string functions
to (a) call the new NSS function to register its error strings,
and (b) use NSPR's set of registered error strings exclusively.

The patch does not eliminate the above named functions that
use a static error message buffer, but it does change them to
use NSPR's strings rather than their own.

After that patch is checked in and bug 172051 is closed, I can
go to work on eliminating the calls to the error functions 
that use a static error buffer, replacing them with calls to 
the safe function(s).  

I believe that the patch for bug 172051 meets Chris Newman's needs.

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, and this bug has priority 'P2'.
:beurdouche, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 16

[Robert Relyea]

I believe this bug can be closed FIXED.

• As comment 14 says, we now have all the error message in an NSPR error table (bug 172051).
• SECU_ErrorString(), SECU_ErrorStringRaw(), and SECU_GetString() no longer exists
• SEC_Strerror is #defined to PR_ErrorToString()
• SECU_PrintError() calls PR_ErrorToName() and PR_ErrorToString()

Comment 17

[Marco Castelluccio [:marco]]

We have modified the bot to only consider P1 as high priority, so I'm cancelling the needinfo here.

Closing as FIXED given comment 16.