172051 - Add localizable error messages for NSS error codes

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Wan-Teh Chang]

We should add localizable error messages for NSS error codes
using the error-code-translation interface declared in prerror.h.

See http://lxr.mozilla.org/nspr/source/nsprpub/pr/include/prerror.h.

Comment 1

[Wan-Teh Chang]

Assigned the bug to Kirk.

This bug is related to bug 66472.

Comment 2

[Wan-Teh Chang]

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

Comment 3

[Kirk Erickson]

SECU_PrintPRandOSError() spits out "NSS_Initialize failed",
so its not as general purpose as we'd like.

Bug 66472 calls for removal of SECU_ErrorStringRaw(), or at least
calls to it other than the one in SECU_Strerror().

We should remove SECU_ErrorStringRaw() and make SECU_Strerror use
the NSPR error-code-translation interface.  Wan-Teh said we can
add the NSS error codes to NSPR's table.   So we end up with a
single localizable thread safe code-to-string routine for all
(NSS, NSPR, and OS) error codes.

As a result of the above change, SECU_PrintError will eliminate
the need to SECU_PrintPRandOSError().

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 5

[Kirk Erickson]

Target Milestone: 3.9

Comment 6

[Kirk Erickson]

Wan-Teh said this should be P2 rather than P1.
I will be focusing on Sun issues first.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

There is a patch to bug 66472 that implements the fix for this bug.  
I will attach it here, also, since it really is a fix for this bug, and
not a fix for bug 66472

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Attachment: register NSS error strings with NSPR. Use those strings.

Comment 9

[John G. Myers]

I recommend using "secerr" and "sslerr" instead of "sec_err" and "ssl_err", as
those form the basis of filenames and some filesystems don't allow underscores.

You need to change SSLerrs.h to fill in all of the holes.  Currently error codes
5 and 10, etc aren't defined, so the translations for most of the SSL error
codes will be off.

SSLerrs.h and SECerrs.h should move from nss/cmd/lib to somewhere under nss/lib

The error tables should be installed by NSS_Init().  Having a separate init
function is needless complexity.  The cost of installing error tables is
negligible to programs that don't use them.

It would be helpful to consumers of NSS if NSS could ship the associated
.properties files, just like NSPR does.  That gives consumers of NSS something
to feed into their localization processes.  The .properties files can be
generated at build time and I would be willing to contribute a followup patch
which does so.

Comment 10

[Wan-Teh Chang]

Comment on attachment 142493 [details] [diff] [review]
register NSS error strings with NSPR. Use those strings.

I also think that the SEC and SSL error tables should be
installed by NSS_Init() automatically.	Since there is no
NSPR function to uninstall an error table, we must take
care not to reinstall the SEC and SSL error tables.
(NSS_Init() may be called again after NSS_Shutdown() has
been called.)

In the new file errstrs.c, the copyright year is 20004,
which has an extra 0.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

prerror.h provides no guidance for the content of "name" except this:
    const char *name; /* Name of error table source */
So, I followed Chris Newman's example.  See first attachment to bug 66472. 
As used in that example and in NSPR's pr/src/misc/prerr.c, the string does
not appear to be a complete file name, e.g. "prerr", not "prerr.h".  
How is this string actually used by NSPR? 

Neither struct PRErrorTable nor function PR_ErrorInstallTable() 
has a member or parameter that specifies which language is used in 
the table of strings being registered.  The documentation says that a table
of strings can only be registered once.  So, it would appear that if an
english string is registered by (say) NSS_Init, then it would not be 
subsequently possible to register strings for another language.  
This is why I am reluctant to register english strings in NSS_Init.

John, care to supply the missing details?

Comment 12

[John G. Myers]

An application which wants to localize the error message would call
PR_ErrorInstallCallback() to provide a list of supported languages and two
localization callbacks.

One callback, of type PRErrorCallbackNewTableFn, gets called by NSPR for each
error table that is installed.  It is provided with the error table and can
return any private data it wants to associate with a table.  An implementation
using bundles would possibly, for each supported language, take the
aforementioned name, append or prepend the language code per its layout, append
".properties", and open the bundle of the resulting filename.

The other callback, of type PRErrorCallbackLookupFn, is called by NSPR for each
call to PR_ErrorToString().  It uses the table and the associated private data
and returns any appropriate translation.

So libraries are responsible for registering tables for their error codes, with
macro names (used for looking up translations) and translations for the
"i-default" (English) language.  Applications are responsible for providing any
localization framework and for translating the error descriptions of the
libraries they use.  As input to the application's localization procedure,
libraries should provide an English .properties file for each table it registers.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 142493 [details] [diff] [review]
register NSS error strings with NSPR. Use those strings.

cancelled this review request.	Will write another patch.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2

In addition to the patches shown in this attachment, I will 
1. move nss/cmd/lib/SECerrs.h  to nss/lib/util/SECerrs.h
2. move nss/cmd/lib/SSLerrs.h  to nss/lib/util/SSLerrs.h
3. cvs remove nss/cmd/lib/NSPRerrs.h

Comment 15

[Wan-Teh Chang]

Comment on attachment 145783 [details] [diff] [review]
patch v2 

Nelson, if nss_Init calls NSS_InitializePRErrorTable,
why do we need to export NSS_InitializePRErrorTable?

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Re; Comment 15
Not all programs that use this error table call NSS_Init or its kin.

Wan-Teh, please mark this bug r+ or r-, thanks.

Comment 17

[Wan-Teh Chang]

Comment on attachment 145783 [details] [diff] [review]
patch v2 

r=wtc.	I have one important notice and some
minor suggested changes.

The important issue is that PR_ErrorToString
is not thread-safe if the error code is not
found in any of the error tables.  In that
case, PR_ErrorToString returns a pointer to
a static buffer that begins with "Unknown code ".
I think NSS needs to work around this NSPR
bug.

Here are my suggested changes.

1. Consider having NSS_InitializePRErrorTable
return SECStatus or PRStatus.  PR_ErrorInstallTable
fails if PR_Malloc fails.

2. The new files need to use the MPL/GPL/LGPL
tri-license header now.

3. This patch creates a dependency of lib/util
on lib/ssl.  For example, lib/util/errstrs.c
includes lib/ssl/sslerr.h.

4. Question: in errstrs.c, can we include
secerr.h, sslerr.h, and prinit.h at the beginning
of the file?

5. certutil.c: we should fix the other instances
of printing the return value of PR_GetError()
directly.

6. secerror.c: the use of initDone can be deleted.
It is handled by the PR_CallOnce call in
NSS_InitializePRErrorTable.

7. secutil.c: SECU_ErrorStringRaw can use PL_strncpyz
instead of PR_snprintf.

    PL_strncpyz(SECUErrorBuf, strg, sizeof SECUErrorBuf);

8. Since PR_ErrorToString never returns NULL, we
may need to update come comments to reflect this
and we can also simplify some code to take advantage
of this.

For example, the comment before SECU_Strerror in
secerror.c says:
    Returns NULL of [sic] errNum is unknown.
This will no longer be true when SECU_Strerror returns
the return value of PR_ErrorToString.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

I hope there will be time to do this for 3.11

Comment 19

[Kai Engert (:KaiE:)]

Nelson, why did you say in comment 14 that you are going to cvs remove NSPRerrs.h? Is there another place that defines strings for those error codes? I couldn't find anything.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Kai asked 

> why did you say in comment 14 that you are going to cvs remove NSPRerrs.h? 

Because NSPR has its own table that maps its error numbers to strings,
and NSPR installs its table during NSPR initialization.  So, when NSS 
provides its error strings through a string table, as NSPR does now,
there will be no need for NSS to continue to duplicate NSPR's error strings.

> Is there another place that defines strings for those error codes?

http://lxr.mozilla.org/nspr/source/nsprpub/pr/src/misc/prerr.c#45

With regard to comment 14, I think it best to move ALL the NSS error strings
out of header files and into .c files, to become part of a string table 
that gets installed by a call to PR_ErrorInstallTable(), just as NSPR now
does with its file prerr.c (URL above).

Comment 21

[Peter Kasting]

The whiteboard here says "awaiting checkin".  Should "patch v2" be checked in on trunk?  It's not clear to me what the status of this old bug is.

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Peter, in comment 17, Wan-Teh gave "patch v2" r+ but then listed 8 changes 
to be made to it first.  To me, this is equivalent of r-.  

I will consider reimplementing this for NSS 3.12

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

Unsetting target milestone in unresolved bugs whose targets have passed.

Comment 24

[Elio Maldonado]

Attachment: patch v3 - Updated for current sources addresses wtc's review suggestions.

Update to Nelson's patch to current sources. 
Implements Wan-Teh's suggestions brought by Wan-Teh as follows:
0. Split the error tables into "regular" nss and ssl error tables and the code that installs them. The first goes to lib/util and the other to lib/nss.

1. NSS_InitializePRErrorTable and SSLInitializePRErroTable
return PRStatus.

2. The new files now use the MPL/GPL/LGPL tri-license header.

3. This patch does not create a dependency of lib/util on lib/ssl. errstrs.c in lib/util instals the table for the non-ssl errors while sslerrstrs.c in lib/nss intstalls the ssl error table. Tried placing this in lib/nss but it wouldn't compile because lib/nss is built earlier.

4. In errstrs.c move the include of secerr.h, and prinit.h to the beginning
of the file, likewise in strerrs.c.

5. certutil.c: fixed all the other instances that prined the return value of PR_GetError() directly.

6. secerror.c: the use of initDone was deleted. Had to add some error handling because NSS_InitializePRErrorTable and SSL_InitializePRErrorTable. of the error tables could fail for lack of memory.

7. secutil.c: SECU_ErrorStringRaw now uses PL_strncpyz.

8. Updated the comment before SECU_Strerror. Though PR_ErrorToString never returns NULL we may still fail to get an error due to failure to install the tables. I'm returning NULL but returning a spacial string stating that that the was a problem mapping the error may be better. In either case we won't be able to return to the caller the accurate description of the error.

Comment 25

[Elio Maldonado]

SECU_Strerror calls both NSS_InitializePRErrorTable and SSL_InitializePRErrorTable, maybe we should have another version that only calls the first one. That way it could be used for error logging when we are testing softoken and the full NSS isn't installed.

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

Elio,
Why are the SSL strings separated from the rest of the NSS error strings?
Wouldn't it be simpler to keep them all together?
If you're going to separate them, why are the SSL strings placed into 
libNSS instead of into libSSL?  Surely they should be in the library that 
uses them (invokes those error codes), no?  
Is it a matter of figuring out how to invoke the function to register the 
strings?  That's solvable without creating circular dependencies.

It appears that you're moving SSLerrs.h from one directory to another.
Are there any differences? or are you merely moving the file from one 
directory to another?
In the latter, case, you should not use CVS add and CVS remove, but rather
use mozilla.org's facility for (effectively) moving an RCS file.

Comment 27

[Elio Maldonado]

(In reply to comment #26)
> Elio,
> Why are the SSL strings separated from the rest of the NSS error strings?
> Wouldn't it be simpler to keep them all together?

This is to avoid creating a dependency of lib/util on lib/ssl.  For example, lib/util/errstrs.c must include lib/ssl/sslerr.h before it includes SSLErrs.h. Admittedly, it's only a dependency on a header with a bunch of constants and not a dependency on ssl code. We could move sslerr.h to lib/util as we did with other headers. Maybe keeping things together is not as bad after all.

> If you're going to separate them, why are the SSL strings placed into 
> libNSS instead of into libSSL?  Surely they should be in the library that 
> uses them (invokes those error codes), no?  
As I mentioned on Item 3 of comment 24, that was my original choice but ran into compile problems because nss is compiled before ssl. I didn't think wise to alter the order in which the directories are built.

> Is it a matter of figuring out how to invoke the function to register the 
> strings?  That's solvable without creating circular dependencies.
Yes, wouldn't we need dynamic invocation here.  Having the ssl library to register a function pointer to its table registration function with nss somehow crossed my mind. Any advise is very welcome.

> 
> It appears that you're moving SSLerrs.h from one directory to another.
> Are there any differences? or are you merely moving the file from one 
> directory to another?
> In the latter, case, you should not use CVS add and CVS remove, but rather
> use mozilla.org's facility for (effectively) moving an RCS file.
This a mere move, just like you had described in comment 14, so I should request mozilla.org to do it.

Comment 28

[Elio Maldonado]

Attachment: patch v4

Install the NSS and SSL error tables with one install call as Nelson's patch v2 had it. To avoid making lib/util dependent on lib/ssl we need to move lib/ssl/sslerr.h to util/sslerr.h where secerr.h already resides.

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

In today's conference call, I learned that Red Hat views it as a problem 
for a lower level RPM (such as the one that contains NSS's libNSSutil) to 
depend on header files from a higher level RPM (such as the one that 
contains NSS's libNSS or NSS's libSSL).  OK, I'll accept that.

But then, this begs the next question:  

Why should these error string tables be in libNSSutil rather than in libNSS?

I think the rule for deciding what belongs in libNSSutil is as follows:

Things belong in libNSSutil if they meet any of the following criteria:
a) they are already in libNSSutil, or 
b) they will be used by libNSSutil, or
c) they will be used by shared libs in multiple NSS RPMs (e.g. NSS RPM *AND* 
in the softoken RPM) and need not live inside the FIPS boundary.
That is, they are used by one or more of 
   - libsoftoken
   - libfreebl
   - libnssdbm
   - libnssckbi
AND by one or more of 
   - libNSS
   - libSSL
   - libSMIME

Now, AFAIK, none of the other NSS shared libs depends on libSSL.  Therefore,
they should not depend on any of the error codes set by the functions in 
libSSL.  Any places in other NSS shared libs that DO reference SSL error 
codes should probably be regarded as bugs and should be fixed, so that they
no longer rely on SSL error codes.

It's completely unclear to me why libSSL's error codes (headers or strings) 
need to live in in the RPM with libNSSutil, and seems very undesirable.

I do see a slight advantage to having a single function to register ALL of 
NSS's localizable error strings, but having to move a significant part of 
libSSL into libNSSutil to accomplish that seems like too high a price to pay.

Comment 30

[Wan-Teh Chang]

I agree.  Perhaps it's time to add an initialization function for
libSSL that can take care of registering the SSL error string table.

I recently built NSS one directory at a time and found some
"wrong" interdependencies between the directories.  The worst
ones are:
- libnss3.so depends on a header in libSSL because certdb.c
  needs SSL_ERROR_BAD_CERT_DOMAIN.
- libnss3.so depends on two headers in libSMIME because pk11wrap
  needs the SEC_PKCS7RecipientInfo and NSSCMSRecipient types.

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 417404 [details] [diff] [review]
patch v3 - Updated for current sources addresses wtc's review suggestions.

I believe this patch was obsoleted by patch v4.

Comment 32

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 418186 [details] [diff] [review]
patch v4

I'm giving this patch r- for two reasons, one very specific and one
more vague.

1) I'm definitely giving r- to the portion of this patch chat modifies
the file mozilla/security/nss/cmd/certutil/certutil.c

The problem is that it changes ALL the places that print error 
messages to call SECU_PrintError, even though (in most cases) 
the messages being printed do not have any relevant NSS/NSPR 
error code associated with them.  For example, there are many 
places that detect problems with illegitimate combinations of 
command line options.  There is no error code set for those errors,
no call to PORT_SetError.  They simply print out a message explaining
what was wrong with the command line options, and then quit.  Changing
those to call SECU_PrintError will make them also print out a string
for whatever value happens to be set in the NSPR error code (that is,
the value returned by PORT_GetError) which is likely to be completely
irrelevant in all cases.  So, I oppose blindly changing all those 
print statements to call SECU_PrintError instead. 

2) There is a design issue that remains to be resolved about where
(in which shared library or libraries) the error strings belong 
and where (in which RPMs) the header files belong that define those 
error strings.  

IMO, this should be primarily determined by run-time dependencies.  
Since there are unlikely to ever be any run-time calls to any 
functions that return error strings for error code numbers from 
any of libnssutil, libsoftoken or libfreebl, my preference would
be to put those tables of error strings, and the functions that 
handle them, into the highest level libraries possible.  

I can see that libNSSutil will occasionally SET error codes that will 
be part of NSS's "SEC" errors, and this will necessitate that the 
header file(s) that define the error codes SET in libNSSutil be part 
of libNSSutil.  But, IMO, libNSSutil should never use (SET) any of 
libSSL's error codes.  

Maybe it's time to refactor  NSS's error code files, separating the
error definitions into files according to which RPM needs to define
them, rather than trying to lump all NSS's error strings together.  

But this is just my opinion.  If Bob, Wan-Teh and/or Alexei want to 
lump them all together, so be it.

Comment 33

[Elio Maldonado]

(In reply to comment #32)
1) Oh yes, that -r on certutil is well deserved. My bad.

2) There is a design issue that remains to be resolved about where
> (in which shared library or libraries) the error strings belong 
> and where (in which RPMs) the header files belong that define those 
> error strings.  
> 
Yes, those tables of error strings, and the functions that handle them, should be into the highest level libraries possible.  
For SSL errors: The tables for ssl errors should be handled by ssl. That brings up the problem that nss shouldn't depend on ssl and nssinit can call ssl to do it.  We either have the caller call an API to install the ssl error table or invokes an ssl initialization call as Wan-Teh suggested. An alternative would be to use shared library initializer/finalizer. 

For the more generic NSS errors: On first thought libnss3 would be the right place to do it but we also need access to error codes when building the softoken/freebl RPM where we don't the rest of nss higher level libraries like nss3.so. I for one would like to run the bltest cipher suite when building the softoken RPM and it needs error code. Neither the softokn nor freebl libraries are suitable to host the NSS error table installation. That leaves only nssutil.

Agreed that nssutil should not have dependencies on the ssl errors. On the other hand we already have secerr.h in lib/util. Having nssutil install the NSS error table doesn't look as bad

> Maybe it's time to refactor  NSS's error code files, separating the
> error definitions into files according to which RPM needs to define
> them, rather than trying to lump all NSS's error strings together. 

IMO a split is needed, let's not do it in a too-fine-grained fashion.
The RPM needs needs should be the guidance rather than who sets the error the error codes. It can get messy with too many places installing tables. Splitting into NSS and SSL is preferable and manageable. 

Is having nssutil handling the installation of the generic NSS error code table objectionable?

Is the use of shared library initializer code so that the loader calls it for us worthwhile or objectionable? Granted, it only helps when using shared libraries. If anyone is linking statically they would need to explicitly request it via the APIs.

Comment 34

[Elio Maldonado]

Changed the target milestone to 3.12.7 as 3.12.6 has been released.

Comment 35

[Elio Maldonado]

Attachment: patch v5

This patch splits the installation of the tables in two.  NSS error tables installed by libutil and SSL ones by libssl. Moved SSLerrs.h to ssl and SECerrs.h to util where secerr.h alreday resides resides. Removed NSPRerrs.h. Added an NSS_Strerror API. When user calls NSS_Init the NSS error table is installed. NSS_Strerror is such that even callers that go via PKCS #11  and don't call NSS_init the get the tables installed. Callers must call SSL_init in order for the ssl error codes to be available. I think this meets all comments and desires as per original bug request and is compatible with our split of nss into nss, softokn, and util srpms. It does not allow me to have tools such as bltest be compilable with only softoken and util present as I would have wanted because libsecutil still depends on higher level APIs for to certs and pkcs7 functionality. That's beyond the original purpose of the bug anyway and is better handled in a separate bug. The patch is rather large as I have modified some of the tools, basically the ssl ones, to work with this refactoring.

Comment 36

[Rich Megginson]

Comment on attachment 492588 [details] [diff] [review]
patch v5

+1 - every program we maintain that uses NSS has been waiting a long time for this - please review this and get it into NSS

Comment 37

[Rich Megginson]

ping - any update on this?

Comment 38

[Elio Maldonado]

In Fedora and RHEL we maintain various packages that would benefit from this patch. I would also like also to run a subset of NSS's own test tools and scripts (e.g. blapitest) as part of the softoken-only builds we do for Fedora/RHEL-6+ and this would facilitate doing it in a clean fashion.

Comment 39

[Konstantin Andreev]

Sorry for being late to the party.

Is this proposal only for GUI applications ? If not, how console applications (part of NSS, e.g. certutil, or 3-rd party) are intended to by localized ?

Error strings are assumed to be printed to console, which is described by locale strings, e.g. lt_LT.ISO8859-13 lt_LT.UTF-8 ru_RU.ISO8859-5 ru_RU.ANSI1251 ...

Who should map "locale language" (lt_LT ru_RU sh_BA) to PRLanguageCode ?

Who should care about converting UTF-8, outputted by PR_ErrorToString, to "locale encoding" (ISO8859-13 ISO8859-5 ANSI1251) ?

Comment 40

[Rich Megginson]

(In reply to comment #39)
> Sorry for being late to the party.
> 
> Is this proposal only for GUI applications ? If not, how console
> applications (part of NSS, e.g. certutil, or 3-rd party) are intended to by
> localized ?
> 
> Error strings are assumed to be printed to console, which is described by
> locale strings, e.g. lt_LT.ISO8859-13 lt_LT.UTF-8 ru_RU.ISO8859-5
> ru_RU.ANSI1251 ...
> 
> Who should map "locale language" (lt_LT ru_RU sh_BA) to PRLanguageCode ?
> 
> Who should care about converting UTF-8, outputted by PR_ErrorToString, to
> "locale encoding" (ISO8859-13 ISO8859-5 ANSI1251) ?

The proposal is for NSS to support the NSPR PR_ErrorToString API in prerror.h - NSS should install a string table that maps error codes to strings.  It will then be _possible_ for NSS developers and application developers to provide localized error messages.  Note that the API doesn't automatically figure out the locale based on the environment - the programmer of the API must specify the language to use:
NSPR_API(const char *) PR_ErrorToString(PRErrorCode code,
    PRLanguageCode language);

It is up to the application programmer to specify the language to use.  It is up to the application programmer to install additional localized tables.  It is up to the application programmer to map the locale from the environment to a language code.

Comment 41

[Konstantin Andreev]

(In reply to comment #38 from Elio Maldonado)
> In Fedora and RHEL we maintain various packages that would benefit from this patch. 

Elio, could you, please, explain, how Fedore/RHEL would benefit from this ?

(In reply to comment #40 from Rich Megginson)
> It is up to the application programmer to :
>   1) specify the language to use.  
>   2) install additional localized tables. 
>   3) map the locale from the environment to a language code.

And (4): to convert UTF-8 to "locale encoding", IIUC. The most annoying task. Without this being a library feature, language-specific error messages seems to be useless for console applications.

Comment 42

[Elio Maldonado]

(In reply to comment #41)
> (In reply to comment #38 from Elio Maldonado)
> 
> Elio, could you, please, explain, how Fedore/RHEL would benefit from this ?

Some backround first to make item 1 below clear. In Fedora and RHEL 6 we have split nss into three packages: nss, nss-softokn, and nss-util. We did so to be able to ship the crypto by itself. For RHEL softoken is FIPS 140-validated and we want keep updating NSS while keeping softoken at the last version that got FIPS validated. nss-util is just the common utilies shared by softoken and the rest of nss. 

1) Testing-wise:

We run the test suite only when we build the nss package. I would very much like to run the crypto subset of the tests when I build nss-softoken right away. I can't do so because the tools rely on the internal secutil library for several services, error reprting being one. When we build nss-softokn we cannot rely on higher levels of nss. With this patch we not only move this error logging support into NSS proper itself and make it avialbel to other applications besides oor oen tests but also split the installation of the tables for SSL errors from the those for the lower-level errors. This is good for the crypto-only tests which now can just count just only the lower lavel error table. 

2) Some cert management tools/utilies that are cients of nss:
I co-maintain some certificate/key managing tools for Apache. Apache can use NSS via a helper package called mod_nss (analogous to mod_ssl). I needed good error reporting and ended copying the error tables and some code from the tools support library. With this patch I could thow away all that duplicated code and data and rely on nss for my error reporting needs.

My needs are basic, other packages would benefit even more.
> 
> (In reply to comment #40 from Rich Megginson)
> > It is up to the application programmer to :
> >   1) specify the language to use.  
> >   2) install additional localized tables. 
> >   3) map the locale from the environment to a language code.
> 
> And (4): to convert UTF-8 to "locale encoding", IIUC. The most annoying
> task. Without this being a library feature, language-specific error messages
> seems to be useless for console applications.

Comment 43

[Rich Megginson]

(In reply to comment #41)
> (In reply to comment #38 from Elio Maldonado)
> > In Fedora and RHEL we maintain various packages that would benefit from this patch. 
> 
> Elio, could you, please, explain, how Fedore/RHEL would benefit from this ?
> 
> (In reply to comment #40 from Rich Megginson)
> > It is up to the application programmer to :
> >   1) specify the language to use.  
> >   2) install additional localized tables. 
> >   3) map the locale from the environment to a language code.
> 
> And (4): to convert UTF-8 to "locale encoding", IIUC. The most annoying
> task. Without this being a library feature, language-specific error messages
> seems to be useless for console applications.

So you want a charset conversion API embedded inside NSS?

The reason for this bug is that every single application that uses NSS has to make it's own table of standard error messages e.g. look at the cmd line programs included with NSS - they have tables of error code -> error message mappings which are extremely useful, but they are private to the cmd line programs and cannot be used by other application code.  So the application developer (e.g. me) has to copy/paste these tables around to several places, which is error and bitrot prone.

If these tables were included in the NSS core library, and made available via the PR_ErrorToString API, that would be a very good start.  I don't want to hold up this work because NSS does not have an embedded charset conversion API.

Comment 44

[Konstantin Andreev]

Elio, thank you for explanation.

Rich, let me highlight: non-English message is in particular encoding.

Application encoding is out of the library control, so localization API that addresses only {language}, not {language,encoding} is as useful, as building without roof.

But you are right, it is hardly possible to have charset conversion in NSPR, so current approach, unfortunately, is near the best one can do.

I have some notes concerning error-reporting API.

- IMO, this kind of map is very useful at least for developers:

   -8173 ~> "SEC_ERROR_NO_MEMORY" (literal symbolic name of error code)

- Given that error codes come from different layers (nspr, nss, ssl), it would be useful to have indication, which layer the translation comes from. E.g.

   -8173 ~> "nss"

And I have a note regarding error reporting. Usually error message, displayed to a user looks like:

|  appname: invalid unit of measurements.     (1)

This works well until libs/applications gets internationalized/localized. Russian user will see:

|  appname: неверная единица измерения.       (2)

However, when the Russian user goes to Google for support, or "User's manual (English)" it finds close to nothing, because most support is in English, and user is not aware, what is original (not localized) error message.

This is typical localization mistake, thus, I always "unset LANG" before launching gcc.

The solution is to include unique error code right in a message:

|  appname: [1234] invalid ...                (3)

or even symbolic name:

|  appname: [1234/SEC_INVALID_UOM] invalid .. (4)

The sense string "appname: [1234]" will be easily locatable both in Google and English textbook.

Given that NSS, NSPR are about to have localized error reporting, I kindly invite you to use (3) or (4) form of reporting in this patch.

If you will do so, you will introduce a useful pattern, that, with time, will spread over NSS code.

Comment 45

[Robert Relyea]

Comment on attachment 492588 [details] [diff] [review]
patch v5

r-

I generally like this patch, both the idea and most of the implementation. We cannot, however, suddenly require a new init call for SSL to make this work.

We should use the existing SSL implicit initialization to load the tables.

Another comment is a positive vote for Konstantin Andreev's idea. I would have r+ this patch if it wasn't for SSL_Init, but I would have suggested a follow on.

I think it's OK to get the localized string from the table, but we should also include a function that gets the string and includes the number and the symbolic name (the latter is the most important of the two).

It would also be good to have the equivalent of perror() as well of strerror(). 

bob

Comment 46

[Elio Maldonado]

Attachment: patch V6 - Removed SSL_init, more useful logging and a perror-like api

Comment 47

[Elio Maldonado]

Attachment: V6 - minus some unwanted pieces that snuck in

Comment 48

[Elio Maldonado]

Comment on attachment 546073 [details] [diff] [review]
V6 - minus some unwanted pieces that snuck in

In NSS_Strerror(PRErrorCode errNum)
.....
>+    count = PR_snprintf(ebuf, EBUFF_SIZE, "[%d %s] %s",
This line
>+    if (count != 1) return NULL;
should have been
>+    if (count == -1) return NULL;

and in sslock.c SSL_OptionSetDefault should trigger ssl_init
Need to test some more.

Comment 49

[Elio Maldonado]

Attachment: V7 - better tested

Comment 50

[Elio Maldonado]

Attachment: demo app that uses the APIs to log errors

Comment 51

[Elio Maldonado]

Attachment: makefile for the demo app

Comment 52

[Elio Maldonado]

Attachment: This what the error logs of all erros look like

Comment 53

[Robert Relyea]

Comment on attachment 546137 [details] [diff] [review]
V7 - better tested

r+ except for fipstest.c, secutil.c, secutil.h also see nits...

fipsttest.c -
   This patch has nothing to do with error codes. I think it crept in from some other bug.

secutil.h -
   This patch is fine, but needs the changes to secutil.c, so I've excluded it.

secutil.c - 
   1. It's usually a good idea to search the source code when you see a function you aren't sure what it was supposed to do. This applies to SECU_GetString. Even though it's not declared 'static', a quick grep will tell you that it's not called from anywhere else other than this file. It's called from SECU_ErrorString, only when the error number could not be found. Trying to get the error number again seems self-defeating.
   2. i looks like you moved the static SECUErrorBuf array ahead of SECU_GetString, but then you don't take advange of this fact in SECU_GetString.  You also don't delete it's previous declaration, so you declare it twice. The whole thing looks like an incomplete change.
   3. SECU_ErrorStringRaw and SECU_ErrorString are basically identical, except the former provides nothing if the error is not recognized.

I suggest we do one of the following.
1) ignore these changes for now, and phase out the use of SECU_ErrorString in favor of SECUStrerror.
2) Same as 1), except update the signatures as given in your version of secutil.h.
3) Remove SECU_GetString(), and make SECU_ErrorStringRaw and SECU_ErrorString call SECU_Strerror().

---------------------------------------------------

Nits on the rest of the patch.
ssl_init should not run if ssl_inited is 1. This is thread safe because it is only set to one after the table has been initialized. If more than one thread clears the first if, they will still be subject to the callone in ssl_InitializePRErrorTable(). Such a test will short circuit the common case that the error table has already been initialized without the overhead of 2 more calls and the NSPR pr_once code.

bob

Comment 54

[Robert Relyea]

Oh, I almost forget. The NSS_Strerror() is not thread safe since it uses a static buffer. We may want a version that is thread safe that returns a buffer the caller can free.

Comment 55

[Elio Maldonado]

(In reply to comment #54)

How about adding something like this?

/* NSS_StrerrorTS is a thread safe version of NSS_Strerror.
 * It formats output into a buffer allocated at run time.
 * The buffer is allocated with PR_smprintf thus the string
 * returned should be freed with PR_smprintf_free.
 */
char *
NSS_StrerrorTS(PRErrorCode errNum)
{
    static int initDone;

    if (!initDone) {
	/* nspr_InitializePRErrorTable(); done by PR_Init */
	PRStatus rv = NSS_InitializePRErrorTable();
	/* If this calls fails for insufficient memory, just return NULL */
	if (rv != PR_SUCCESS) return NULL;
	    initDone = 1;
    }

    return PR_smprintf("[%d %s] %s",
		errNum, PR_ErrorToName(errNum),
		PR_ErrorToString(errNum, PR_LANGUAGE_I_DEFAULT));
}

Comment 56

[Robert Relyea]

Yes, something very much like that...

bob.

Comment 57

[Elio Maldonado]

(In reply to comment #53)
> 
> I suggest we do one of the following.
> 1) ignore these changes for now, and phase out the use of SECU_ErrorString
> in favor of SECUStrerror.
> 2) Same as 1), except update the signatures as given in your version of
> secutil.h.
Which use PRErrorCode while in various places with the tools there is a combination of types for the errnum being passes to this functions, they use PRErrorCode, int, int16, and PrInt. Cleaning this is something I would rateher defer. It does matter as I found out. On option 1 al all.sh passes with outpu.log clean. To my surprise with option 2 all test pass but I see error prints on output.log "  ERROR -8032: [-8032 SEC_ERROR_POLICY_VALIDATION_FAILED] Cert chain fails policy validation"
The happen in the chain.sh. I haven't determine the exact cause. I do know it's not caused by the patch to the library. I goes away with option 1 (where i don't change the type of the parameter to those two functions. I'll look into this later.

> 3) Remove SECU_GetString(), and make SECU_ErrorStringRaw and
> SECU_ErrorString call SECU_Strerror().

At first this was my preferred option but given the weirdness above I think we should defer (3) and go with option (1). Just for the time being until I can investigate further. I can still do (3) as part of Bug 66472 and can be more ambitious and clean up the tools further.

Comment 58

[Elio Maldonado]

The problems had nothing to do with mismatched types after all. I did a grep on FAILED with context and was able to very that in output.log shows that this error is expected. We also need to keep the simpler function that doesn't have the enhanced reporting as the one that the other calls, otherwise the fips suite has a failure on an error test. all.sh passes on Fedora and Windows. I'm going with option 3.

Comment 59

[Elio Maldonado]

Patch committed into trunk:
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/cmd/lib/Makefile
Checking in mozilla/security/nss/cmd/lib/Makefile;
/cvsroot/mozilla/security/nss/cmd/lib/Makefile,v  <--  Makefile
new revision: 1.5; previous revision: 1.4
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/cmd/lib/manifest.mn
Checking in mozilla/security/nss/cmd/lib/manifest.mn;
/cvsroot/mozilla/security/nss/cmd/lib/manifest.mn,v  <--  manifest.mn
new revision: 1.14; previous revision: 1.13
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/cmd/lib/secerror.c
Checking in mozilla/security/nss/cmd/lib/secerror.c;
/cvsroot/mozilla/security/nss/cmd/lib/secerror.c,v  <--  secerror.c
new revision: 1.4; previous revision: 1.3
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/cmd/lib/secutil.c
Checking in mozilla/security/nss/cmd/lib/secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.108; previous revision: 1.107
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/cmd/lib/secutil.h
Checking in mozilla/security/nss/cmd/lib/secutil.h;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.h,v  <--  secutil.h
new revision: 1.36; previous revision: 1.35
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/cmd/modutil/install.c
Checking in mozilla/security/nss/cmd/modutil/install.c;
/cvsroot/mozilla/security/nss/cmd/modutil/install.c,v  <--  install.c
new revision: 1.5; previous revision: 1.4
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/cmd/modutil/instsec.c
Checking in mozilla/security/nss/cmd/modutil/instsec.c;
/cvsroot/mozilla/security/nss/cmd/modutil/instsec.c,v  <--  instsec.c
new revision: 1.4; previous revision: 1.3
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/cmd/pk12util/pk12util.c
Checking in mozilla/security/nss/cmd/pk12util/pk12util.c;
/cvsroot/mozilla/security/nss/cmd/pk12util/pk12util.c,v  <--  pk12util.c
new revision: 1.46; previous revision: 1.45
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/cmd/selfserv/selfserv.c
Checking in mozilla/security/nss/cmd/selfserv/selfserv.c;
/cvsroot/mozilla/security/nss/cmd/selfserv/selfserv.c,v  <--  selfserv.c
new revision: 1.95; previous revision: 1.94
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/cmd/signtool/sign.c
Checking in mozilla/security/nss/cmd/signtool/sign.c;
/cvsroot/mozilla/security/nss/cmd/signtool/sign.c,v  <--  sign.c
new revision: 1.16; previous revision: 1.15
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/cmd/signtool/util.c
Checking in mozilla/security/nss/cmd/signtool/util.c;
/cvsroot/mozilla/security/nss/cmd/signtool/util.c,v  <--  util.c
new revision: 1.29; previous revision: 1.28
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/cmd/signtool/verify.c
Checking in mozilla/security/nss/cmd/signtool/verify.c;
/cvsroot/mozilla/security/nss/cmd/signtool/verify.c,v  <--  verify.c
new revision: 1.6; previous revision: 1.5
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/cmd/signver/signver.c
Checking in mozilla/security/nss/cmd/signver/signver.c;
/cvsroot/mozilla/security/nss/cmd/signver/signver.c,v  <--  signver.c
new revision: 1.14; previous revision: 1.13
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/cmd/strsclnt/strsclnt.c
Checking in mozilla/security/nss/cmd/strsclnt/strsclnt.c;
/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c,v  <--  strsclnt.c
new revision: 1.69; previous revision: 1.68
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/cmd/vfychain/vfychain.c
Checking in mozilla/security/nss/cmd/vfychain/vfychain.c;
/cvsroot/mozilla/security/nss/cmd/vfychain/vfychain.c,v  <--  vfychain.c
new revision: 1.34; previous revision: 1.33
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/lib/nss/nssinit.c
Checking in mozilla/security/nss/lib/nss/nssinit.c;
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision: 1.108; previous revision: 1.107
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/lib/ssl/manifest.mn
Checking in mozilla/security/nss/lib/ssl/manifest.mn;
/cvsroot/mozilla/security/nss/lib/ssl/manifest.mn,v  <--  manifest.mn
new revision: 1.19; previous revision: 1.18
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/lib/ssl/sslerrstrs.c
RCS file: /cvsroot/mozilla/security/nss/lib/ssl/sslerrstrs.c,v
done
Checking in mozilla/security/nss/lib/ssl/sslerrstrs.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslerrstrs.c,v  <--  sslerrstrs.c
initial revision: 1.1
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/lib/ssl/sslerrstrs.h
RCS file: /cvsroot/mozilla/security/nss/lib/ssl/sslerrstrs.h,v
done
Checking in mozilla/security/nss/lib/ssl/sslerrstrs.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslerrstrs.h,v  <--  sslerrstrs.h
initial revision: 1.1
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/lib/ssl/sslinit.c
RCS file: /cvsroot/mozilla/security/nss/lib/ssl/sslinit.c,v
done
Checking in mozilla/security/nss/lib/ssl/sslinit.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslinit.c,v  <--  sslinit.c
initial revision: 1.1
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/lib/ssl/sslsnce.c
Checking in mozilla/security/nss/lib/ssl/sslsnce.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsnce.c,v  <--  sslsnce.c
new revision: 1.56; previous revision: 1.55
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/lib/ssl/sslsock.c
Checking in mozilla/security/nss/lib/ssl/sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.72; previous revision: 1.71
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/lib/ssl/sslutil.h
RCS file: /cvsroot/mozilla/security/nss/lib/ssl/sslutil.h,v
done
Checking in mozilla/security/nss/lib/ssl/sslutil.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslutil.h,v  <--  sslutil.h
initial revision: 1.1
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/lib/util/errstrs.c
RCS file: /cvsroot/mozilla/security/nss/lib/util/errstrs.c,v
done
Checking in mozilla/security/nss/lib/util/errstrs.c;
/cvsroot/mozilla/security/nss/lib/util/errstrs.c,v  <--  errstrs.c
initial revision: 1.1
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/lib/util/errstrs.h
RCS file: /cvsroot/mozilla/security/nss/lib/util/errstrs.h,v
done
Checking in mozilla/security/nss/lib/util/errstrs.h;
/cvsroot/mozilla/security/nss/lib/util/errstrs.h,v  <--  errstrs.h
initial revision: 1.1
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/lib/util/manifest.mn
Checking in mozilla/security/nss/lib/util/manifest.mn;
/cvsroot/mozilla/security/nss/lib/util/manifest.mn,v  <--  manifest.mn
new revision: 1.23; previous revision: 1.22
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/lib/util/nssutil.def
Checking in mozilla/security/nss/lib/util/nssutil.def;
/cvsroot/mozilla/security/nss/lib/util/nssutil.def,v  <--  nssutil.def
new revision: 1.15; previous revision: 1.14
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/lib/util/nssutil.h
Checking in mozilla/security/nss/lib/util/nssutil.h;
/cvsroot/mozilla/security/nss/lib/util/nssutil.h,v  <--  nssutil.h
new revision: 1.19; previous revision: 1.18
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/tests/pkcs11/netscape/suites/security/ssl/sslc.c
Checking in mozilla/security/nss/tests/pkcs11/netscape/suites/security/ssl/sslc.c;
/cvsroot/mozilla/security/nss/tests/pkcs11/netscape/suites/security/ssl/sslc.c,v  <--  sslc.c
new revision: 1.3; previous revision: 1.2
done
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/tests/pkcs11/netscape/suites/security/ssl/sslt.c
Checking in mozilla/security/nss/tests/pkcs11/netscape/suites/security/ssl/sslt.c;
/cvsroot/mozilla/security/nss/tests/pkcs11/netscape/suites/security/ssl/sslt.c,v  <--  sslt.c
new revision: 1.4; previous revision: 1.3
done

Comment 60

[Elio Maldonado]

Attachment: Patch that was applied

Comment 61

[Elio Maldonado]

Another commit:
[emaldona@emaldonadesktop ERRLOGGING]$ cvs commit -m "Fix Bug 6172051 - Add localizable error messages for NSS error codes, r=rrelyea" mozilla/security/nss/cmd/bltest/blapitest.c 
Checking in mozilla/security/nss/cmd/bltest/blapitest.c;
/cvsroot/mozilla/security/nss/cmd/bltest/blapitest.c,v  <--  blapitest.c
new revision: 1.62; previous revision: 1.61
done

Comment 62

[Elio Maldonado]

In my system I had an ipv6 related workaround in selfserv.c for Fedora only that I was not supposed to include. This commit reverts the uninntended change.

[emaldona@emaldonadesktop selfserv]$ cvs commit -m "Bug 172051 - Revert an unwanted change - a fedora-only workaround that should not have been included in previous commit" selfserv.c
Checking in selfserv.c;
/cvsroot/mozilla/security/nss/cmd/selfserv/selfserv.c,v  <--  selfserv.c
new revision: 1.96; previous revision: 1.95
done

Comment 63

[Elio Maldonado]

Attachment: Supplemental patch for testing

This change makes SEC_Strerror call nss_Strerror and works as it this before. No bogus failures of invalid certificate chain caused by the preappending we do to the error string. Both results.html and output.log show no failures. Tinderbox does look at and show the contents output.log

Comment 64

[Robert Relyea]

Comment on attachment 553878 [details] [diff] [review]
Supplemental patch for testing

r- while this does fix tinderbox, it doesn't follow NSS export guidelines. We are exporting an essentially internal function.

I'd rather change the signature of NSS_Strerror() to include a format specifier which tells it whether or not to include the error number. SECU_Strerror() can use NSS_Strerror without the error number (use an enum in case we need to add additional format options in the future.

bob

Comment 65

[Elio Maldonado]

Attachment: A possible fix

This takes care of most of the errors but not all. I still get a failure reported on fips.sh in the error test case when we call dbtest to initialize a corrupted database. It's supposed to fail but gets reported as error. I'm currently investigating how to deal with that. 
It should be noted that dbtest doesn't SECU_strerror but instead
    if (rv != SECSuccess) {
        SECU_PrintPRandOSError(progName);
        ret=NSS_INITIALIZE_FAILED_ERR;
    } else {
      ....

Comment 66

[Elio Maldonado]

(In reply to Elio Maldonado from comment #65)
> reported on fips.sh in the error test case when we call dbtest to initialize
> a corrupted database. 

The test failing is actually - Init NSS with a corrupted library (dbtest -r)

Comment 67

[Elio Maldonado]

Attachment: Fixes all failures - addresses review request

Takes care of all failures, NSS_Strerror takes a format argument as recommended. Naming of the enumerated type could be better. I need to check it in before the review to turn Tinderbox green again.

Comment 68

[Elio Maldonado]

Committed changes to trunk:
Checking in mozilla/security/nss/cmd/bltest/blapitest.c;
/cvsroot/mozilla/security/nss/cmd/bltest/blapitest.c,v  <--  blapitest.c
new revision: 1.63; previous revision: 1.62
done
Checking in mozilla/security/nss/cmd/lib/secerror.c;
/cvsroot/mozilla/security/nss/cmd/lib/secerror.c,v  <--  secerror.c
new revision: 1.5; previous revision: 1.4
done
Checking in mozilla/security/nss/lib/util/errstrs.c;
/cvsroot/mozilla/security/nss/lib/util/errstrs.c,v  <--  errstrs.c
new revision: 1.2; previous revision: 1.1
done
Checking in mozilla/security/nss/lib/util/nssutil.h;
/cvsroot/mozilla/security/nss/lib/util/nssutil.h,v  <--  nssutil.h
new revision: 1.20; previous revision: 1.19
done
Checking in mozilla/security/nss/cmd/modutil/install.c;
/cvsroot/mozilla/security/nss/cmd/modutil/install.c,v  <--  install.c
new revision: 1.6; previous revision: 1.5
done

Comment 69

[Robert Relyea]

Comment on attachment 554006 [details] [diff] [review]
Fixes all failures - addresses review request

r+ rrelyea

Comment 70

[Wan-Teh Chang]

I am sorry I have to reopen this bug.  I reviewed the patch
last Saturday and found serious problems with the new functions
added to libnssutil3.so.  I feel strongly about these problems
that I am willing to do the work of fixing them.

1. NSS_Strerror is not thread-safe because it may return a
result in a file-static buffer.

2. NSS_StrerrorTS is still not thread-safe because it calls
NSS_Strerror without locking.

The version Elio showed in comment 55 is thread-safe.

3. NSS_StrerrorTS and NSS_Perror are not used by NSS itself
(neither the libraries or the command-line tools), as shown
by these two MXR queries:

http://mxr.mozilla.org/security/ident?i=NSS_StrerrorTS
http://mxr.mozilla.org/security/ident?i=NSS_Perror

Note: I believe this is why we didn't notice NSS_StrerrorTS
incorrectly applies the "[%d %s] %s" format string twice.

4. The most serious problem is that I believe these three
functions should be added to mozilla/security/nss/cmd/lib
instead of mozilla/security/nss/lib/util, because they are
only used by the command-line tools:
http://mxr.mozilla.org/security/ident?i=NSS_Strerror

Clients of the NSS libraries can easily build their own
versions of the Strerror and Perror functions, in the
error message format they like.  formatSimple is equivalent
to PR_ErrorToString(errNum, PR_LANGUAGE_I_DEFAULT), and
formatIncludeErrorCode may not be suitable for every NSS
client.

Here is my proposal:

1. Remove NSS_Strerror, NSS_StrerrorTS, and NSS_Perror.

2. Implement SECU_Strerror using the current code for
NSS_Strerror(errNum, formatSimple), which does not need
the static buffer, and is therefore thread-safe.

Elio, Bob, is this OK with you?

Also, I think NSS_InitializePRErrorTable should be called
by NSS initialization.

Comment 71

[Elio Maldonado]

(In reply to Wan-Teh Chang from comment #70)
> I am sorry I have to reopen this bug.  I reviewed the patch
> last Saturday and found serious problems with the new functions
> added to libnssutil3.so.  I feel strongly about these problems
> that I am willing to do the work of fixing them.
> 
> 1. NSS_Strerror is not thread-safe because it may return a
> result in a file-static buffer.
Yes, it is not and that's the reason for NSS_StreoorTS. Now, if you invoke NSS_Strerror(errorCode, formatSimple); then it should be thread-safe as it's basically the internal nss_Strerror. I should have stated so in the comments in the haeder. 

> 
> 2. NSS_StrerrorTS is still not thread-safe because it calls
> NSS_Strerror without locking.
Oh, thank you for pointing that out. Must admit concurrency is not yet my strong suit.

> 
> The version Elio showed in comment 55 is thread-safe.
It has the disadvantage that the caller is responsible for to deallocating the buffer 
> 
> 3. NSS_StrerrorTS and NSS_Perror are not used by NSS itself
> (neither the libraries or the command-line tools), as shown
> by these two MXR queries:
Yes, we don't use it but Bob requested them. The former for thread-safely and the price I mentioned and the later to emulate the Posix perror.
The tools are using to use NSS_Strerror(errCode, format_Simple), otherwise we fail some tests.

> 
> http://mxr.mozilla.org/security/ident?i=NSS_StrerrorTS
> http://mxr.mozilla.org/security/ident?i=NSS_Perror
> 
> Note: I believe this is why we didn't notice NSS_StrerrorTS
> incorrectly applies the "[%d %s] %s" format string twice.
> 
> 4. The most serious problem is that I believe these three
> functions should be added to mozilla/security/nss/cmd/lib
> instead of mozilla/security/nss/lib/util, because they are
> only used by the command-line tools:
> http://mxr.mozilla.org/security/ident?i=NSS_Strerror
> 
> Clients of the NSS libraries can easily build their own
> versions of the Strerror and Perror functions, in the
> error message format they like.  formatSimple is equivalent
> to PR_ErrorToString(errNum, PR_LANGUAGE_I_DEFAULT), and
> formatIncludeErrorCode may not be suitable for every NSS
> client.
> 
> Here is my proposal:
> 
> 1. Remove NSS_Strerror, NSS_StrerrorTS, and NSS_Perror.
> 
> 2. Implement SECU_Strerror using the current code for
> NSS_Strerror(errNum, formatSimple), which does not need
> the static buffer, and is therefore thread-safe.
That's my favorite version. 

I would prefer having less functions.
> 
> Elio, Bob, is this OK with you?

Not okay if you move things back inside into nss/cmd/lib. We need these functions (or at least one them) in lib/util as a public API of NSS. Rich and other folks would very much desire to use this functions and not have to copy code from the nss tools.

Having them inside cmd/lib prevents me from improving the way we test for Fedora and RHEL-6
where we have split nss into tree packages nss-util, nss-softoken, and nss (the rest of it). It's in my plans to have blapitest be built and run as part of the nss-softokn build and test where I can't rely on the higher layers of nss.

> 
> Also, I think NSS_InitializePRErrorTable should be called
> by NSS initialization.

If I remember correctly it does in lib/nss/nssinit.c, I need to review the code again.

Let's talk some more.

Comment 72

[Robert Relyea]

 
> Elio, Bob, is this OK with you?

I think now is the time to fix the API's.
I'm not completely clear on your proposal. Are you saying applications should call PR_ErrorToString directly?

I think we need at least one NSS_Error string function. Currently at Red Hat we have a half a dozen apps which are sort of trying to implement their own because we don't have one.

I also prefer the non-simple format as the default.

Beyond that I'm pretty flexible about the spellings and signature.

bob

Comment 73

[Rich Megginson]

(In reply to Robert Relyea from comment #72)
>  
> > Elio, Bob, is this OK with you?
> 
> I think now is the time to fix the API's.
> I'm not completely clear on your proposal. Are you saying applications
> should call PR_ErrorToString directly?

This is what openldap tls_m.c does - for example:
  char *token_name = PK11_GetTokenName( slot );
  PRErrorCode errcode = PR_GetError();
  Debug( LDAP_DEBUG_ANY,
         "TLS: could not authenticate to the security token %s - error %d:%s.\n",
         token_name ? token_name : DEFAULT_TOKEN_NAME, errcode,
         PR_ErrorToString( errcode, PR_LANGUAGE_I_DEFAULT ) );

Makes sense to me - what if errcode is not from secerr.h or sslerr.h?  What if the problem is OOM or access denied or some other nspr error code?  In that case, the application would need extra logic to figure out which err2string mapping function to use.  Using PR_ErrorToString directly, I don't have to worry about it.

> 
> I think we need at least one NSS_Error string function. Currently at Red Hat
> we have a half a dozen apps which are sort of trying to implement their own
> because we don't have one.
> 
> I also prefer the non-simple format as the default.
> 
> Beyond that I'm pretty flexible about the spellings and signature.
> 
> bob

Comment 74

[Wan-Teh Chang]

The currently internal function nss_Strerror essentially
is a three-line function:

static char *
nss_Strerror(PRErrorCode errNum)
{
    If (NSS_InitializePRErrorTable() != PR_SUCCESS)
        return NULL;
    return (char *) PR_ErrorToString(errNum, PR_LANGUAGE_I_DEFAULT);
}

If NSS initialization can take care of the NSS_InitializePRErrorTable()
call, then nss_Strerror() is just the PR_ErrorToString call.

Do you object to having applications call PR_ErrorToString directly?
The only value added by nss_Strerror is:
- potentially call NSS_InitializePRErrorTable(), if we can't do that
  in NSS initialization
- no need to pass the PR_LANGUAGE_I_DEFAULT argument

Seems like a small value to me.  Why can't an application call
NSS_InitializePRErrorTable() just once, and then call
PR_ErrorToString directly?

As for the non-simple format, the "[%d %s] %s" format may not be
suitable for all applications, not to mention it is the source of
the thread-safety problem.  The NSS API only needs to provide
the components of an error message -- the integer error code value,
the symbolic error code name, and the error string.  It is up to an
application to format an error message using these ingredients.

Comment 75

[Robert Relyea]

> Do you object to having applications call PR_ErrorToString directly?

1) I prefer applications having an NSS version to call. 
2) Did you miss that I prefer the non-simple default semantics?


> As for the non-simple format, the "[%d %s] %s" format may not be
> suitable for all applications, not to mention it is the source of
> the thread-safety problem. 

It may not be suitable for all applications, but it's most suitable for the command line versions that are most likely to use this function. The code can be implemented in a thread safe manner.

bob

Comment 76

[Wan-Teh Chang]

Re: an NSS version of PR_ErrorToString: I suspect you want to
hide NSS's dependency on NSPR, which puts off many prospective
NSS users.  That's fine by me.  If so, it may be better to use
the PORT_ prefix to look similar to the related PORT_SetError
and PORT_GetError functions.

Re: thread safety: NSS_StrerrorTS, as specified, requires the
caller to free the return value.  That's very inconvenient.
It means you can't do this:
  fprintf(stderr, "Key generation failed: %s\n",
          NSS_StrerrorTS(PORT_GetError(), formatSimple));

You have to save it in a local variable so you can free it:
  char *message = NSS_StrerrorTS(PORT_GetError(), formatSimple);
  fprintf(stderr, "Key generation failed: %s\n", message);
  PR_smprintf_free(message);

In contrast, you can easily avoid malloc+free if you
format the message yourself:
  int err = PORT_GetError();
  fprintf(stderr, "Key generation failed: [%d %s] %s\n",
          err, PR_ErrorToName(err),
          PR_ErrorToString(err, PR_LANGUAGE_I_DEFAULT));

Re: non-simple format: the NSS command-line tools are using
the simple format.  This MXR query shows both callers of
NSS_Strerror in NSS pass 'simpleFormat' as the second argument:
http://mxr.mozilla.org/security/ident?i=NSS_Strerror

I don't think NSS clients need our help with formatting an
error message.  That's not the job of the NSS API.  NSS only
needs to provide the three things they need to format an
error message: the error code, the symbolic name, and the
error string.

Comment 77

[Elio Maldonado]

(In reply to Wan-Teh Chang from comment #76)
> Re: an NSS version of PR_ErrorToString: I suspect you want to
> hide NSS's dependency on NSPR, which puts off many prospective
> NSS users.  That's fine by me.  If so, it may be better to use
> the PORT_ prefix to look similar to the related PORT_SetError
> and PORT_GetError functions.

I hadn't thought about but yes, some are put-off. For consistency, using the PORT_ prefix sounds good to me.

> 
> Re: thread safety: NSS_StrerrorTS, as specified, requires the
> caller to free the return value.  That's very inconvenient.
> It means you can't do this:
>   fprintf(stderr, "Key generation failed: %s\n",
>           NSS_StrerrorTS(PORT_GetError(), formatSimple));
> 
> You have to save it in a local variable so you can free it:
>   char *message = NSS_StrerrorTS(PORT_GetError(), formatSimple);
>   fprintf(stderr, "Key generation failed: %s\n", message);
>   PR_smprintf_free(message);

Gotting to above,
NSS_StrerrorTS(PORT_GetError(), formatSimple);
gives you the same as calling nss_strerror(PORT_GetError());
but with extra work inside.

> 
> In contrast, you can easily avoid malloc+free if you
> format the message yourself:
>   int err = PORT_GetError();
>   fprintf(stderr, "Key generation failed: [%d %s] %s\n",
>           err, PR_ErrorToName(err),
>           PR_ErrorToString(err, PR_LANGUAGE_I_DEFAULT));

Indeed. Client code (and test if they need it) can do it.
I didn't to muck around with the test too much at this time.

> 
> Re: non-simple format: the NSS command-line tools are using
> the simple format.  This MXR query shows both callers of
> NSS_Strerror in NSS pass 'simpleFormat' as the second argument:
> http://mxr.mozilla.org/security/ident?i=NSS_Strerror

Yes, as I said above I avoided touch the tests too much at this time.
> 
> I don't think NSS clients need our help with formatting an
> error message.  That's not the job of the NSS API.  NSS only
> needs to provide the three things they need to format an
> error message: the error code, the symbolic name, and the
> error string.

I agree with you on this. 

The nss-Strerror code implements the simple case in a thread-safe manner and with simple code. I could rename it PORT_StringError and make it public. What do you think?

I agree with you that clients that need special formatting can easily do it themselves with a small amount of code as you show above. We could suggest how to do with some code snippet in the documentation, either in the Wiki or the header comments themselves.

Comment 78

[Robert Relyea]

>   int err = PORT_GetError();
>   fprintf(stderr, "Key generation failed: [%d %s] %s\n",
>           err, PR_ErrorToName(err),
>           PR_ErrorToString(err, PR_LANGUAGE_I_DEFAULT));

I would be OK with a macro PORT_PERROR(x) == fprintf(stderr,"%s: [%d %s] %s\n", PORT_GetError(), PORT_ErrorToName(PORT_GetError()), PORT_ErrorToString(PORT_GetError(), PR_LANGUAGE_I_DEFAULT));

and Macros PORT_ErrorToString() and PORT_ErrorToName().

> Re: non-simple format: the NSS command-line tools are using
> the simple format.  This MXR query shows both callers of
> NSS_Strerror in NSS pass 'simpleFormat' as the second argument:

yes, unfortunately the NSS tests were blowing up because some of them actually parsed the error string output. For Programmers, the error symbol is probably the most important thing. What we are trying to avoid is the numerous places where you get 'App failed: -8105'.

In any case, long term I would like certutil to return the error symbol.

I'm OK with hiding some complexity in macros, but the common case should be relatively simple (Programmers are used to using some flavor of perror, for instance).

bob

Comment 79

[Elio Maldonado]

Attachment: A clean simple API + suggested macros + a bit of test cleanup

Comment 80

[Rich Megginson]

Comment on attachment 557244 [details] [diff] [review]
A clean simple API + suggested macros + a bit of test cleanup

https://bugzilla.mozilla.org/attachment.cgi?id=557244&action=diff#mozilla/security/nss/lib/util/errstrs.c_sec1

I think you will have to use a PR_CallOnce here - just testing for initDone is not sufficient if two threads can call NSS_Strerror at the same time.

Comment 81

[Elio Maldonado]

I once had some comment to the effect that NSS_InitializePRErrorTable(); is idempotent
I sse that in Comment 6 Wan-Teh suggested deleting the test for initDone. Shall I drop it then?
In the ssl code that get's called from many places I should keep that as Bob suggested.

Comment 82

[Elio Maldonado]

NSS_InitializePRErrorTable has only two direct callers, NSS_Strerror and nssInitModules which is what all versions of NSS_Init call. Had to convince myself of what I bet was obvious to all of you. The calling chain is
nss_InitializePRErrorTableOnce
  NSS_InitializePRErrorTable --- idempotent
     nssInitModules -> nss_Init -> NSS_Init {all variants} of it
  NSS_strerror

By the time we get NSS_Strerror NSS_Init has already install the tables. Dropping unneeded stuff we end up with the one liner
     return (char *) PR_ErrorToString(errNum, PR_LANGUAGE_I_DEFAULT);
which we may as well making it the macro

#define PORT_StringError(errNum) PR_ErrorToString(errNum, PR_LANGUAGE_I_DEFAULT);

What Bob and Wan-Teh were getting at :-) New patch coming next.

Comment 83

[Elio Maldonado]

Attachment: macros + test cleanup

Comment 84

[Elio Maldonado]

Comment on attachment 557515 [details] [diff] [review]
macros + test cleanup

Review of attachment 557515 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozilla/security/nss/lib/util/secport.h
@@ +164,5 @@
> + * describing the last error encountered during a call
> + * to an NSS library function.
> + */
> +#define PORT_PERROR(s) { \
> +    PRErrorCode err = PORT_getError(), \

Bad case: Change PORT_getError() to PORT_GetError()
Caught while working on another bug.

Comment 85

[Elio Maldonado]

and the comma to a semi-colon.

Comment 86

[Elio Maldonado]

In the PORT_ERROR(S) macro, instead of
		fprintf(stderr, "%s : [%d : %s] %s\n", \
I could drop the brackets 
		fprintf(stderr, "%s : %d : %s %s\n", \
and that may make it easier for shell, perl, or awk scripts to parse and split the pieces.
I still need to test this. I also question the wisdom of including the macro here. Printing is something that's better left for the tools, users can write one as they see fits best for them. Let me resubmit the patch for review without this macro. I can add it later.

Comment 87

[Elio Maldonado]

Attachment: macros + test cleanup (without the perror-like macro)

Comment 88

[Wan-Teh Chang]

Comment on attachment 559144 [details] [diff] [review]
macros + test cleanup  (without the perror-like macro)

Elio: thank you for writing the patch.  Just one nit and
one issue to ask your opinion of.

In mozilla/security/nss/lib/util/nssutil.def:

>-;+NSS_3.13 {    # NSS 3.13 release
>+;+NSSUTIL_3.13 {         # NSS Utilities 3.13 release
> ;+    global:
> NSSUTIL_GetVersion;
> NSS_InitializePRErrorTable;
> NSS_Strerror;
>-NSS_StrerrorTS;
>-NSS_Perror;
> ;+    local:
> ;+       *;
> ;+};

NSS_Strerror should also be removed.

In mozilla/security/nss/lib/util/secport.h:

>+#define PORT_ErrorToString(err) PR_ErrorToString(err, PR_LANGUAGE_I_DEFAULT)

I like the Strerror name, so we can also name this function
PORT_Strerror.  But perhaps ErrorToString and ErrorToName
make it easier to tell the difference between them.

Comment 89

[Wan-Teh Chang]

Comment on attachment 559144 [details] [diff] [review]
macros + test cleanup  (without the perror-like macro)

One more nit in mozilla/security/nss/lib/util/secport.h:

>+/* Returns a UTF-8 encoded constant error string for "errNum".
>+ * Returns NULL if initialization of the error tables fails
>+ * due to insufficient memory.
>+ *
>+ * This string must not be modified by the application, but may be
>+ * modified by a subsequent call to NSS_Perror() or NSS_Strerror().
>+ */
>+#define PORT_ErrorToString(err) PR_ErrorToString(err, PR_LANGUAGE_I_DEFAULT)

"errNum" in the comment does not match the actual macro argument "err".

The comment mentions NSS_Perror and NSS_Strerror.  NSS_Perror is gone,
and NSS_Strerror has been renamed.

By the way, I don't think we need to provide a "perror" macro or function.

Comment 90

[Elio Maldonado]

It's more than a nit, the second paragraph of the comment must be fixed.

I initially liked PORT_Strerror because it remindins people of strerror from which it is partially inspired but having PORT_ErrorToString and PORT_ErrorToName makes the difference between the two clear and. they will be often used in pair. That's way I went that way in nameing.

I agreee, let's not provide the "perror" for various reasons stated earlier. I can have a revison soon, unless Bob is in the middle of reviweing this one and would rathar have me wait.

Comment 91

[Elio Maldonado]

Attachment: address wtc's review comments

Comment 92

[Robert Relyea]

> I agreee, let's not provide the "perror" for various reasons stated earlier.
>  I can have a revison soon, unless Bob is in the middle of reviweing this one and would
>  rathar have me wait.

Actually, I'd like a PORT_PERROR() macro, which includes the symbol and and error number, but that can be added later, so I won't hold up the patch.

Comment 93

[Robert Relyea]

Comment on attachment 560392 [details] [diff] [review]
address wtc's review comments

r+ rrelyea

Comment 94

[Elio Maldonado]

Patch committed to TRUNK:
Checking in mozilla/security/nss/cmd/bltest/blapitest.c;
/cvsroot/mozilla/security/nss/cmd/bltest/blapitest.c,v  <--  blapitest.c
new revision: 1.64; previous revision: 1.63
done
Checking in mozilla/security/nss/cmd/lib/manifest.mn;
/cvsroot/mozilla/security/nss/cmd/lib/manifest.mn,v  <--  manifest.mn
new revision: 1.15; previous revision: 1.14
done
Checking in mozilla/security/nss/cmd/lib/secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.109; previous revision: 1.108
done
Checking in mozilla/security/nss/cmd/lib/secutil.h;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.h,v  <--  secutil.h
new revision: 1.37; previous revision: 1.36
done
Checking in mozilla/security/nss/lib/util/errstrs.c;
/cvsroot/mozilla/security/nss/lib/util/errstrs.c,v  <--  errstrs.c
new revision: 1.3; previous revision: 1.2
done
Checking in mozilla/security/nss/lib/util/nssutil.def;
/cvsroot/mozilla/security/nss/lib/util/nssutil.def,v  <--  nssutil.def
new revision: 1.16; previous revision: 1.15
done
Checking in mozilla/security/nss/lib/util/nssutil.h;
/cvsroot/mozilla/security/nss/lib/util/nssutil.h,v  <--  nssutil.h
new revision: 1.21; previous revision: 1.20
done
Checking in mozilla/security/nss/lib/util/secport.h;
/cvsroot/mozilla/security/nss/lib/util/secport.h,v  <--  secport.h
new revision: 1.24; previous revision: 1.23
done

Comment 95

[Elio Maldonado]

A file I needed to remove as part of the patch, Changes committed to trunk:
Removing mozilla/security/nss/cmd/lib/secerror.c;
/cvsroot/mozilla/security/nss/cmd/lib/secerror.c,v  <--  secerror.c
new revision: delete; previous revision: 1.5
done

Comment 96

[Elio Maldonado]

Fixed a comment, errNum changed to err as per usage in the macro as wtc had requested.
Checking in mozilla/security/nss/lib/util/secport.h;
/cvsroot/mozilla/security/nss/lib/util/secport.h,v  <--  secport.h
new revision: 1.25; previous revision: 1.24
done

Comment 97

[Elio Maldonado]

Attachment: Make NSS_InitializePRErrorTable return type a SECStatus

Comment 98

[Elio Maldonado]

Comment on attachment 564350 [details] [diff] [review]
Make NSS_InitializePRErrorTable return type a SECStatus

I forgot some other recommendations will sned another one

Comment 99

[Wan-Teh Chang]

Comment on attachment 564350 [details] [diff] [review]
Make NSS_InitializePRErrorTable return type a SECStatus

In mozilla/security/nss/lib/util/nssutil.h, please also
remove #include "prerror.h".

Comment 100

[Wan-Teh Chang]

Attachment: Do not add headers to PRIVATE_EXPORT

Elio, feel free to include this patch into your patch.
I verified that the headers are not included by files
in other directories, so they don't need to be added
to PRIVATE_EXPORTS.

Comment 101

[Wan-Teh Chang]

Attachment: Do not add headers to PRIVATE_EXPORT (v2)

Hmm... in fact errstrs.h should be removed from CVS,
because the nss_InitializePRErrorTable function it declares
does not exist:
http://mxr.mozilla.org/security/ident?i=nss_InitializePRErrorTable

Comment 102

[Elio Maldonado]

Comment on attachment 564368 [details] [diff] [review]
Do not add headers to PRIVATE_EXPORT (v2)

Indeed, that must be a remnant of long obsoleted versions of the patch.

Comment 103

[Elio Maldonado]

(In reply to Wan-Teh Chang from comment #99)
> In mozilla/security/nss/lib/util/nssutil.h, please also remove #include "prerror.h".
I see, not needed because nssutil.h no longer declares any new error-related functions since they are now simple macros in secport.h. Will do so.

Comment 104

[Elio Maldonado]

Attachment: emaldona + wtc patches

Comment 105

[Wan-Teh Chang]

Comment on attachment 564403 [details] [diff] [review]
emaldona + wtc patches

Thanks for the patch.  Please fix the following four problems.

1. In mozilla/security/nss/lib/nss/nssinit.c:

In function nss_InitModules:

>     SECStatus rv = SECFailure;
>-    PRStatus status = PR_SUCCESS;

Note that rv is initialized SECFailure.

>+    rv = NSS_InitializePRErrorTable();
>+    if (rv == SECFailure) {
>+	PORT_SetError(SEC_ERROR_NO_MEMORY);
> 	return SECFailure;
>     }

This will break code that assumes rv is initialized to
SECFailure.  (The code does assume that.)

You can either fix that code (change "return rv" to
"return SECFailure"), or change this code to not use rv:
    if (NSS_InitializePRErrorTable() != SECSuccess) {
        PORT_SetError(SEC_ERROR_NO_MEMORY);
        return rv;
    }

2. In mozilla/security/nss/lib/util/errstrs.c:

>-PRStatus
>+SECStatus
> NSS_InitializePRErrorTable(void)
> {
>-    return PR_CallOnce(&once, nss_InitializePRErrorTableOnce);
>+    return (PR_SUCCESS == PR_CallOnce(&once, nss_InitializePRErrorTableOnce));

This is also wrong, because this is a boolean expression but the
return type is not PRBool.  There are two solutions.  You can
say
    return PR_SUCCESS == PR_CallOnce(...) ? SECSuccess : SECFailure;
or you can just use an ugly (but workig) cast:
    return (SECStatus)PR_CallOnce(&once, nss_InitializePRErrorTableOnce);

3. In mozilla/security/nss/lib/util/nssutil.h, also remove
#include "prerror.h".

4. You also need to cvs remove errstrs.h.  Pass the -N option to cvs diff
to include the removed file in the patch.

Comment 106

[Elio Maldonado]

Attachment: elio +wtc patches V2: addresses wtc's review requests

Comment 107

[Elio Maldonado]

committed to trunk: - Part 1.
nss/nssinit.c mozilla/security/nss/lib/ssl/manifest.mn mozilla/security/nss/lib/util/errstrs.h mozilla/security/nss/lib/util/manifest.mn 
Checking in mozilla/security/nss/lib/nss/nssinit.c;
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision: 1.111; previous revision: 1.110
done
Checking in mozilla/security/nss/lib/ssl/manifest.mn;
/cvsroot/mozilla/security/nss/lib/ssl/manifest.mn,v  <--  manifest.mn
new revision: 1.20; previous revision: 1.19
done
Checking in mozilla/security/nss/lib/util/manifest.mn;
/cvsroot/mozilla/security/nss/lib/util/manifest.mn,v  <--  manifest.mn
new revision: 1.24; previous revision: 1.23
done

Comment 108

[Elio Maldonado]

Part2 committed to trunk:
nclude, r=wtc" mozilla/security/nss/lib/nss/nssinit.c mozilla/security/nss/lib/util/errstrs.c mozilla/security/nss/lib/util/nssutil.h mozilla/security/nss/lib/util/secport.h
Checking in mozilla/security/nss/lib/nss/nssinit.c;
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision: 1.112; previous revision: 1.111
done
Checking in mozilla/security/nss/lib/util/errstrs.c;
/cvsroot/mozilla/security/nss/lib/util/errstrs.c,v  <--  errstrs.c
new revision: 1.4; previous revision: 1.3
done
Checking in mozilla/security/nss/lib/util/nssutil.h;
/cvsroot/mozilla/security/nss/lib/util/nssutil.h,v  <--  nssutil.h
new revision: 1.22; previous revision: 1.21
done
Checking in mozilla/security/nss/lib/util/secport.h;
/cvsroot/mozilla/security/nss/lib/util/secport.h,v  <--  secport.h
new revision: 1.26; previous revision: 1.25
done

Comment 109

[Elio Maldonado]

and the header removal
Removing mozilla/security/nss/lib/util/errstrs.h;
/cvsroot/mozilla/security/nss/lib/util/errstrs.h,v  <--  errstrs.h
new revision: delete; previous revision: 1.1
done

Comment 110

[Wan-Teh Chang]

Comment on attachment 564428 [details] [diff] [review]
elio +wtc patches V2: addresses wtc's review requests

r=wtc.

Comment 111

[Wan-Teh Chang]

Comment on attachment 564428 [details] [diff] [review]
elio +wtc patches V2: addresses wtc's review requests

Elio: the way you checked in this patch was strange.  You
checked in this patch in two parts as your comment 107 and
comment 108 show.  But the (err) change in secport.h was
not checked in.

Comment 112

[Elio Maldonado]

(In reply to Wan-Teh Chang from comment #111)
Yes, intended combined patch got meeesd up and ended up doing it stages and missed that change.
Now fixed with:
Checking in mozilla/security/nss/lib/util/secport.h;
/cvsroot/mozilla/security/nss/lib/util/secport.h,v  <--  secport.h
new revision: 1.27; previous revision: 1.26
done