Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Convert libpkix error code into nss error code



10 years ago
10 years ago


(Reporter: Alexei Volkov, Assigned: Alexei Volkov)


Firefox Tracking Flags

(Not tracked)


(Whiteboard: PKIX NSS312B2)


(4 attachments, 1 obsolete attachment)

215.79 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
7.35 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
1.28 KB, patch
Alexei Volkov
: review+
Details | Diff | Splinter Review
1.32 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review


10 years ago
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.


10 years ago
Whiteboard: PKIX NSS312B2
Version: 3.12 → trunk

Comment 1

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

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 


3) Others

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 

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 

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.
Attachment #303430 - Flags: review?(nelson) → review-

Comment 3

10 years ago
Created attachment 304598 [details] [diff] [review]
Patch v2(files with interdependent changes)
Attachment #303430 - Attachment is obsolete: true
Attachment #304598 - Flags: review?(nelson)

Comment 4

10 years ago
Created attachment 304601 [details] [diff] [review]
Patch v3(files that required one more review)
Attachment #304601 - Flags: review?(nelson)
Comment on attachment 304601 [details] [diff] [review]
Patch v3(files that required one more review)

this looks good
Attachment #304601 - Flags: review?(nelson) → review+
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.
Attachment #304598 - Flags: review?(nelson) → review+

Comment 7

10 years ago
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.
Last Resolved: 10 years ago
Resolution: --- → FIXED
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.

Resolution: FIXED → ---
Created attachment 306205 [details] [diff] [review]
fix HPUX builds

This patch doesn't address all the issues I raised, but it should get
HPUX building again.
Attachment #306205 - Flags: review?(alexei.volkov.bugs)
Priority: -- → P1


10 years ago
Attachment #306205 - Flags: review?(alexei.volkov.bugs) → review+
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

10 years ago
Created attachment 306626 [details] [diff] [review]
Fix build issue on hp
Attachment #306626 - Flags: review?(nelson)
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.
Attachment #306626 - Flags: review?(nelson) → review+

Comment 13

10 years ago
/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

10 years ago
Build succeeded on HPUX. closing the bug.
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.