Remove all use of error strings from PKIX library code

Status

NSS
Libraries
P2
normal
11 years ago
9 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Nelson Bolyard (seldom reads bugmail))

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PKIX)

Attachments

(2 attachments)

We've removed error strings from the PKIX_Error struct,
having replaced them all with error numbers.  
Now it's time to remove all the code that carries those strings around
inside of libPKIX library executable code.

Then we'll remove the big table of strings itself from the shared library.
(Assignee)

Comment 1

11 years ago
Created attachment 285973 [details] [diff] [review]
remove error strings from pkix shared libs, part 1

This patch removes all but two references to PKIX_ErrorText[] from
the source code of the PKIX shared libs.  It is not a complete removal 
of all PKIX error strings, but it is a major step.  I'd like to commit
this (or something like it) before going the next step.

The reviewer will notice that there are numerous places where the old
code detected an error in the fatal error class, and changed the error
string, but did not change the error number.  This patch removes the 
code that changed the error string, but does not insert new code to 
change the error number.  Please advise if that is acceptable, or not.
Attachment #285973 - Flags: review?(alexei.volkov.bugs)
(Assignee)

Comment 2

11 years ago
In some sense, the fact that the unpatched code sets error strings, and 
does not also change the error number at the same time is a bug.  
Perhaps we should change the code to set an error code variable, 
but which one?  Having multiple error code variables is quite confusing.
Priority: -- → P2
(Assignee)

Comment 3

11 years ago
Created attachment 285974 [details] [diff] [review]
cleanup function signature mismatches and other warnings in PKIX test commands

When I went to build the first patch, I was shocked at some of the 
warnings I was getting.  Some test callback functions just didn't 
have the right sets of arguments.  This patch fixes that, and also
cleans up some other warnings.  I could file a separate bug for this,
but I thought I'd just tack it on to this bug.  Hope you don't mind. :)
Attachment #285974 - Flags: review?(alexei.volkov.bugs)
(Assignee)

Comment 4

11 years ago
Comment on attachment 285974 [details] [diff] [review]
cleanup function signature mismatches and other warnings in PKIX test commands

The following new comment obviously needs to be fixed.
> {
>+        // needs work
>         return(NULL);
> }

Comment 5

11 years ago
Comment on attachment 285973 [details] [diff] [review]
remove error strings from pkix shared libs, part 1

r=alexei
Attachment #285973 - Flags: review?(alexei.volkov.bugs) → review+

Comment 6

11 years ago
Comment on attachment 285974 [details] [diff] [review]
cleanup function signature mismatches and other warnings in PKIX test commands

r=alexei.

Unfortunately we can not verify trough libpkix test run that changes are ok. These tests are still broken.
Attachment #285974 - Flags: review?(alexei.volkov.bugs) → review+

Comment 7

11 years ago
Comment on attachment 285973 [details] [diff] [review]
remove error strings from pkix shared libs, part 1

Changing from r+ to r- with the following reason:

Some function that this patch touches(example pkix_Build_VerifyCertificate) customize the way of error handling. Example of the code,  

@@ -1143,23 +1141,18 @@ pkix_Build_VerifyCertificate(
                                     (pkixErrorResult,
                                     &pkixErrorClass,
                                     plContext);
                             if (pkixTempResult) return pkixTempResult;
                             if (pkixErrorClass == PKIX_FATAL_ERROR) {
-                                    pkixErrorMsg = PKIX_ErrorText[
-                                            PKIX_USERCHECKERCHECKFAILED];
                                     PKIX_RETURN(FATAL);

if error is from  fatal class, the function creates another error with new error string (PKIX_USERCHECKERCHECKFAILED) and makes error that was returned to be a cause of the new error.

The with this patch modification new error will not get created, but old one will still be propagated to the upper level function. I don't particularly think that is it too harmful, but it is changing original intention of wrapping the error with a new error from cert chain building function.

Also, we may change a class of the error at those level, and make it non-fatal. But this is should be done in the future.  

Thx to Nelson for noticing this bug.
Attachment #285973 - Flags: review+ → review-
(Assignee)

Updated

10 years ago
Depends on: 457980, 390527
Unsetting target milestone in unresolved bugs whose targets have passed.
Target Milestone: 3.12 → ---
(Assignee)

Updated

9 years ago
Whiteboard: PKIX
You need to log in before you can comment on or make changes to this bug.