Last Comment Bug 66472 - NSS should have only one function that generates error messages
: NSS should have only one function that generates error messages
Status: ASSIGNED
:
Product: NSS
Classification: Components
Component: Tools (show other bugs)
: 3.2
: All All
: P2 normal with 1 vote (vote)
: ---
Assigned To: Nelson Bolyard (seldom reads bugmail)
:
:
Mentors:
Depends on: 172051
Blocks: 333403
  Show dependency treegraph
 
Reported: 2001-01-24 15:14 PST by Nelson Bolyard (seldom reads bugmail)
Modified: 2014-06-29 18:47 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Example of what NSPR error table for SEC errors looks like (12.74 KB, text/plain)
2004-01-30 18:17 PST, Chris Newman
no flags Details
alternative patch - maintenance free (703 bytes, patch)
2004-02-20 13:54 PST, Nelson Bolyard (seldom reads bugmail)
no flags Details | Diff | Splinter Review

Description Nelson Bolyard (seldom reads bugmail) 2001-01-24 15:14:44 PST
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 2001-02-21 14:37:04 PST
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 2002-04-25 16:35:40 PDT
Changed the QA contact to Bishakha.
Comment 3 Wan-Teh Chang 2002-05-08 17:07:59 PDT
Set target milestone to NSS 3.5.
Comment 4 Kirk Erickson 2002-05-23 13:51:19 PDT
Set target milestone to NSS 3.6.
Comment 5 Wan-Teh Chang 2002-12-06 11:17:00 PST
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) 2003-05-09 21:19:45 PDT
Remove target milestone of 3.8, since these bugs didn't get into that release.
Comment 7 Kirk Erickson 2003-05-18 23:58:57 PDT
Target Milestone: 3.9
Comment 8 Kirk Erickson 2003-05-24 05:06:14 PDT
Retargeted Future.  Plan to address Sun issues first.
Comment 9 Chris Newman 2004-01-30 18:10:56 PST
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 2004-01-30 18:17:07 PST
Created attachment 140280 [details]
Example of what NSPR error table for SEC errors looks like
Comment 11 Nelson Bolyard (seldom reads bugmail) 2004-01-31 16:17:57 PST
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 2004-02-20 13:03:18 PST
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) 2004-02-20 13:54:57 PST
Created attachment 141848 [details] [diff] [review]
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) 2004-02-27 19:18:05 PST
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.

Note You need to log in before you can comment on or make changes to this bug.