Open Bug 400915 Opened 17 years ago Updated 1 year ago

Remove all use of error strings from PKIX library code

Categories

(NSS :: Libraries, defect, P2)

Tracking

(Not tracked)

People

(Reporter: nelson, Unassigned)

References

Details

(Whiteboard: PKIX)

Attachments

(2 files)

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.
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)
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
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)
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 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 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 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-
Depends on: 457980, 390527
Unsetting target milestone in unresolved bugs whose targets have passed.
Target Milestone: 3.12 → ---
Whiteboard: PKIX

The bug assignee is inactive on Bugzilla, and this bug has priority 'P2'.
:beurdouche, could you have a look please?

For more information, please visit auto_nag documentation.

Assignee: nelson → nobody
Flags: needinfo?(bbeurdouche)
Severity: normal → S3

We have modified the bot to only consider P1 as high priority, so I'm cancelling the needinfo here.

Flags: needinfo?(bbeurdouche)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: