1047288 - Dead Code in file "nss/lib/certhigh/certvfypkixprint.c" inside function "pkix_Cert2ASCII" on line no. 108

Status: RESOLVED DUPLICATE of bug 1050107

(NSS :: Libraries, defect, P3)

Description

[Sachin Kumar Gupta]

Attachment: Proposed Patch.

Error:

Commenting one line of code, depleted the need of line below it.Both the lines should have been commented however just one line is commented in the current version.

The conditional if of errorResult, on line 108 , is redundant and of no use as the above LOC because of which this check was put,has been commented.

NSS 3.16.3 version Code:

       errorResult = PKIX_PL_Cert_GetSubject(cert, &subject, NULL);
        if (errorResult) goto cleanup;

        if (subject){
                subjectAscii = pkix_Object2ASCII((PKIX_PL_Object*)subject);
        }

/*     errorResult = PKIX_PL_Object_GetRefCount((PKIX_PL_Object*)cert, &refCount, NULL); */
        
      if (errorResult) goto cleanup;

 

Recommended Code:

        errorResult = PKIX_PL_Cert_GetSubject(cert, &subject, NULL);
        if (errorResult) goto cleanup;

        if (subject){
                subjectAscii = pkix_Object2ASCII((PKIX_PL_Object*)subject);
        }

/*   errorResult = PKIX_PL_Object_GetRefCount((PKIX_PL_Object*)cert,&refCount,  NULL); 
        
        if (errorResult) goto cleanup;
 */

Comment 1

[Ryan Sleevi]

Comment on attachment 8466087 [details] [diff] [review]
Proposed Patch.

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

r-, but I appreciate the effort. Just needs more work.

Rather than creating another comment block - which makes it easy to uncomment one line but not the other - please integrate the two so they're part of the same logical block.

An even better change would be to remove this file from the compilation entirely. This also requires updating http://mxr.mozilla.org/nss/source/lib/certhigh/certvfypkix.c

There, you will see the two calls into this file (cert_PrintCert / cert_PrintCertChain) are guarded by #ifdef's DEBUG_volkov

All code with DEBUG_volkov should be removed entirely, and then you can remove these as well, removing the false positives, and helping eliminate truly dead code.

Comment 2

[Sachin Kumar Gupta]

Attachment: New Patch for the raised bug

Ryan, this new patch is as per your guidance to place both the comment lines within same block.

Comment 3

[Sachin Kumar Gupta]

Ryan, I will try to look into the removal of this file from compilation and the changes to be made there-after , and would try to submit the patch for it soon. Thanks for your help.

Comment 4

[Sachin Kumar Gupta]

Ryan, I have filed the patch for certvfypkixprint.c file removal from NSS compilation , in a new bug with id 1050107, you may have a look at it and review the attached patch for the raised bug at :

https://bugzilla.mozilla.org/show_bug.cgi?id=1050107

Comment 5

[Ryan Sleevi]

Comment on attachment 8467681 [details] [diff] [review]
New Patch for the raised bug

I'm setting r- on this, because it's unnecessary given that your patch is deleting certvfypkixprint.c

Comment 6

[Sachin Kumar Gupta]

Ryan,This patch is not deleting certvfypkixprint.c ,it is just commenting the lines in certvfypkixprint.c.

Comment 7

[Sachin Kumar Gupta]

The https://bugzilla.mozilla.org/show_bug.cgi?id=1047288#c6 was in relation to patch with id 8467681.

Comment 8

[Ryan Sleevi]

https://bugzilla.mozilla.org/show_bug.cgi?id=1050107 makes this patch and bug unnecessary, does it not?

Comment 9

[Sachin Kumar Gupta]

Yes but Kai thinks removal of file is not a good idea, I would like to know your suggestion.
Would it be good if we keep the certvfypkixprint.c file allowing its compilation only in debug mode as it is mainly for debug purpose.

Comment 10

[Ryan Sleevi]

No. It's irresponsible to keep debug code from someone who doesn't work on NSS anymore, for a bug that is not even relevant (libpkix not matching legacy). I'm surprised and disappointed that anyone would suggest keeping such code, and even more that an obviously good cleanup was feedback-'d. We should not keep this bug open - removing the code really is the correct and sensible approach.