417024 - Convert libpkix error code into nss error code

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Alexei Volkov]

I've not found the bug related to the problem. Creating a new one...

Need to have a way to convert libpkix error codes into nss error codes to provide returned code consistency in the future NSS releases. Nss-libpkix wrapper functions(CERT_PKIXVerifyCert and cert_VerifyCertChainPkix) currently return static error codes that does not corresponds to an actual cert chain validation problem.

Comment 1

[Alexei Volkov]

Attachment: Patch v1

Patch adds a third column into a table of pkix error codes(file pkix_errorstring.h). The third column make association between libpkix and pr error codes.

Patch adds additional member (prCode) into PKIX_Error structure. PKIX_Error_Create sets an pr error code associated with pkix error when the function creates a bug. pkix_Throw function can overwrite undefined prCode value(if prCode is eq 0) for the leaf error(when "cause" eq NULL) by using a a value returned by PORT_GetError.

cert_PkixErrorToNssCode asserts that every list of errors has at least one element with defined error(undefined nss error is set to SEC_ERROR_LIBPKIX_INTERNAL in optimized build).

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 303430 [details] [diff] [review]
Patch v1

Some parts of this patch can be committed now, others cannot.

We believe that the changes to the following files are independent of
the changes to the other files, and that committing these changes by
themselves will not break anything.  A test build will prove or 
disprove that hypothesis.  Assuming it is true, then
r+ for changes to these files:

1) Adding new NSS error codes
   cmd/lib/SECerrs.h
   lib/util/secerr.h

2) Files where the ONLY change is to change some old libPKIX error code
   to use some other old (already existing) PKIX error, such as 
   PKIX_OUTOFMEMORY instead.

   lib/libpkix/pkix_pl_nss/module/pkix_pl_ldapcertstore.c
   lib/libpkix/pkix_pl_nss/pki/pkix_pl_cert.c
   lib/libpkix/pkix_pl_nss/pki/pkix_pl_crl.c
   lib/libpkix/pkix_pl_nss/pki/pkix_pl_crlentry.c
   lib/libpkix/pkix_pl_nss/pki/pkix_pl_generalname.c
   lib/libpkix/pkix_pl_nss/pki/pkix_pl_nameconstraints.c
   lib/libpkix/pkix_pl_nss/pki/pkix_pl_publickey.c
   lib/libpkix/pkix_pl_nss/pki/pkix_pl_x500name.c 
   lib/libpkix/pkix_pl_nss/system/pkix_pl_bytearray.c

3) Others
   lib/libpkix/pkix_pl_nss/pki/pkix_pl_ocsprequest.c
   lib/libpkix/pkix_pl_nss/system/pkix_pl_hashtable.c
   lib/libpkix/pkix_pl_nss/system/pkix_pl_monitorlock.c 
   lib/libpkix/pkix_pl_nss/system/pkix_pl_primhash.c  

We found some definite problems.  R- for the following files:

a) lib/libpkix/pkix_pl_nss/module/pkix_pl_ldapdefaultclient.c
   Don't delete that default case in function 
   pkix_pl_LdapDefaultClient_Destroy( 

b) lib/libpkix/pkix_pl_nss/system/pkix_pl_common.c
   Don't report false out of memory errors.

c) Don't remove PKIX_ERROR calls in 
   lib/libpkix/pkix_pl_nss/system/pkix_pl_rwlock.c

d) lib/libpkix/pkix/certsel/pkix_certselector.c
   new PKIX_ERROR in the middle of a basic block code, 
   can't be right because PKIX_ERROR is a goto.

e) lib/libpkix/pkix/top/pkix_validate.c
   cert can be NULL

The rest are all interdependent, and need to be reviewed together.  
Hopefully the next patch will be about half the size of this one.

Comment 3

[Alexei Volkov]

Attachment: Patch v2(files with interdependent changes)

Comment 4

[Alexei Volkov]

Attachment: Patch v3(files that required one more review)

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 304601 [details] [diff] [review]
Patch v3(files that required one more review)

this looks good

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 304598 [details] [diff] [review]
Patch v2(files with interdependent changes)

r+ = nelson, with two changes:

1. This patch makes the following change in 6 places:

>-        PORT_SetError(SEC_ERROR_IO);   /* need pkix->nss error map */
>+        cert_PkixErrorToNssCode(error, &nssErrorCode, plContext);
>+        PORT_SetError(nssErrorCode);
>         goto cleanup;

It could factor that into one place with one label that does those
two lines. 

2. in Index: lib/libpkix/pkix/top/pkix_build.c
after the label cleanup, we see:

>         }
>+        if (!*pValResult && !verifyError) {
>+            if (finalError) {
>+                pkixErrorResult = finalError;
>+                pkixErrorCode = finalError->errCode;
>+                finalError = NULL;
>+                goto cleanup;
>+            }

That goto creates an unintended loop.  Please fix it.

Comment 7

[Alexei Volkov]

both patched have been submitted. Patch v2 was also submitted with additional change(reverse to prior patch to pkix_pl_primhash.c): the function that adds key/value into hash continues to throw errors in case of the collisions. Fix requires more changes that will be made in a patch to bug 417645.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

This checkin caused the builds to fail on HPUX.
I see why.  There are two lines that are incorrect.  
Most compiler overlook this error, but not HPUX.
I missed them in review.  The lines are:

pkix_error.c,    line 55 -- extern const int const PKIX_PLErrorIndex[];
pkix_pl_error.c, line 50 -- const SECErrorCodes const PKIX_PLErrorIndex[] =

There are three problems.
1) These two declarations disagree on the type of the array elements.
They must agree.  

2) The "extern" declaration in pkix_error.c needs to be in a header file
that is included by both pkix_error.c and pkix_pl_error.c.  This will detect
any future inconsistencies.  In fact, all such "extern" declarations that
are found in .c files need to be moved into header files for this reason.

3) The double const on those lines is incorrect.  remove the second one.

Note that nearby is another similar declaration, which reads:
   const char * const PKIX_ErrorText[] =
That declaration is correct.  It is the presence of the '*' that makes 
it correct.  That declaration says that the contents of the array are 
all constant, and that the chars to which they point are also constant.
But since the declaration of PKIX_PLErrorIndex doesn't include any '*',
both const words apply to the same values, the array members.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Attachment: fix HPUX builds

This patch doesn't address all the issues I raised, but it should get
HPUX building again.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 306205 [details] [diff] [review]
fix HPUX builds

pkix/util/pkix_error.c;             new 1.11; previous 1.10
pkix_pl_nss/system/pkix_pl_error.c; new 1.2;  previous 1.1

If the HPUX nightly builds succeed again tonight, this bug can be
marked fixed, again.

Comment 11

[Alexei Volkov]

Attachment: Fix build issue on hp

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 306626 [details] [diff] [review]
Fix build issue on hp

This will also fix it.  
Going forward, we will only add new members to the end of structures.

Comment 13

[Alexei Volkov]

/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/system/pkix_pl_lifecycle.c,v  <--  pkix_pl_lifecycle.c
new revision: 1.18; previous revision: 1.17

Comment 14

[Alexei Volkov]

Build succeeded on HPUX. closing the bug.