[Pki-devel] [ PATCH 11-2 ] --- Re: [PATCH 11] Dogtag 10 - Fixes for Coverity issues relating to NULL_RETURNS Category -- for review

Tue Jun 12 21:48:53 UTC 2012

Submitting patch 11-2 for review.
All the comments have been addressed. Build passed the smoke test.

Thank you.
Regards,
Abhishek Koneru

On Mon, 2012-06-11 at 15:47 -0500, Endi Sukma Dewata wrote:
> On 6/8/2012 4:53 PM, Abhishek Koneru wrote:
> > Please find attached [PATCH 11] with fixes for Coverity issues of
> > category NULL RETURNS for DogTag 10.
> 
> Some issues:
> 
> 1. The link to JSS jar file should have been 
> /usr/share/java/jss/jss4.jar. This link is created by scripts/dev_setup.
> 
> 2. In AuthToken.java:303,341,343,390 the code catches the exception and 
> throws a new exception but with the same type and message. This is not 
> necessary and it will change the stack trace information (it will no 
> longer show where the original exception happens). It's better to simply 
> not catch the exception and let it be handled by the caller.
> 
> There are cases where catch-and-throw make sense. If you want to do 
> something but still want to keep the original stack trace you can 
> re-throw the same exception object (not create a new one):
> 
>    } catch (Exception e) {
>        // do something
>        throw e;
>    }
> 
> If you want to change the exception type/message you can throw a new 
> exception that wraps the old exception:
> 
>    } catch (OldException e) {
>        throw new NewException(e);
>        // or throw new NewException(e.getMessage());
>    }
> 
> Wrapping is possible if the NewException has a constructor that takes 
> the OldException. This is to allow chaining the stack trace. If such 
> constructor is not available at least try to include the original 
> exception message.
> 
> 3. In CMCAuth.java:877 the exception should throw the 
> CMS_AUTHENTICATION_MANAGER_NOT_FOUND message from UserMessages.
> 
> 4. Similar to #2, in CMCAuth.java:907 it's not necessary to catch and 
> throw a new exception with the same type/message.
> 
> 5. While you're at it, in SubjAltNameExt.java:255, the /* of String */ 
> comment can be removed since we're using generics now.
> 
> 6. In DisplayHtmlServlet.java:65 moving the authenticate() into the 
> try-catch block doesn't seem to be necessary. The authenticate() throws 
> an EBaseException with is already declared by the process(). The 
> try-catch block is intended to deal with template problem, which is not 
> the case with authenticate().
> 
> 7. There's a formatting issue in ChallengeRevocationServlet1.java:138.
> 
> 8. In UpdateCRL.java:123 the authenticate() is moved into a try-catch 
> block which will swallow all exceptions. I think the null return value 
> should be checked before the try-catch block.
> 
> 9. In LDAPSecurityDomainSessionTable.java:195 you can reuse the 
> LDAPAttribute object obtained for null checking. Also, it would be 
> better to indicate which entry is invalid in the exception message.
> 
>    LDAPAttribute sid = entry.getAttribute("cn");
>    if (sid == null) {
>        throw new Exception("Invalid LDAP Entry " + entry.getDN()
>            + ". No session id (cn).");
>    }
>    ret.add(sid.getStringValueArray()[0]);
> 
> Similarly, add the entry DN into the exception message in line 237.
> 
> 10. In AuthSubsystem.java:461 you can iterate over hashtable values 
> directly, thus avoiding the lookup operation:
> 
>    for (AuthManagerProxy proxy : mAuthMgrInsts.values()) {
>        IAuthManager mgr = proxy.getAuthManager();
>        ...
>    }
> 
> 11. In PasswdUserDBAuthentication.java:188 the getUser() will return 
> null only if there's a problem. However, if you throw an exception in 
> line 190 it will be logged as "UID not found", which is not the case. I 
> think the null checking should be moved after the try-catch block like this:
> 
>    try {
>        user = ug.getUser(uid);
>    } catch (...) {
>        ...
>    }
> 
>    if (user == null) {
>        throw new EInvalidCredentials(
>            CMS.getUserMessage("CMS_AUTHENTICATION_INTERNAL_ERROR",
>                "Failure in User Group subsystem."));
>    }
> 
> Ideally the getUser() should throw an exception instead of returning 
> null. That would be fixed under ticket #201.
> 
> 12. There's a formatting issue in DBSubsystem.java:411. Also, it would 
> be better to include which entry that has a problem:
> 
>    throw new Exception(
>        "Missing attribute " + PROP_NEXT_RANGE + " in entry " + dn);
> 
> 13. Formatting issue in KeyRepository.java:254. Also, include the serial 
> number in the exception message in line 272:
> 
>    throw new EBaseException(
>        "Failed to recover Key for Serial Number " + serialNo);
> 
> 14. In ARequestQueue.java:591 the boolean initialization is not 
> necessary because it will be initialized with the return value of 
> serviceRequest(). Also remove the auto-generated TODO comment.
> 
> 15. In AuthTokenTest.java:122 it might be better to use this message:
> 
>    throw new Exception("Unable to get key2 as Date");
> 
> 16. Formatting issue in AuthTokenTest.java:143.
> 
> 17. In DRMTool.java:1645 the readLine() will only return null if the 
> password file is empty, which I suppose is not allowed. So I think it 
> should throw an exception instead of continuing with empty password.
> 
> 18. Same issue in PKCS12Export.java:227,239 as in #17.
> 
> 19. In KRAService.java:101 remove the auto-generated TODO message.
> 
> 20. In RecoveryService.java:200 it might be better to throw 
> EKRAException with user message CMS_KRA_INVALID_KEYRECORD. This may make 
> some other changes unnecessary.
> 
> 21. As explained in #2, in HTTPClient.java:431,448 you can throw the 
> original exception object.
> 
> 22. Also like in #2, in HTTPClient.java:1198 you could leave the 
> exception uncaught or throw the original exception object, but remove 
> the auto-generated TODO message in line 1201.
> 
> 23. In ChallengeException.java:36,45, the original code would have 
> thrown an exception. The new code will continue and return an empty 
> string. It might be better to return the attribute directly and let the 
> caller handle the null value:
> 
>    public StateAttribute getState() {
>        return _res.getAttributeSet().getAttributeByType(
>            Attribute.STATE);
>    }
> 
>    public ReplyMessageAttribute getReplyMessage() {
>        return _res.getAttributeSet().getAttributeByType(
>            Attribute.REPLY_MESSAGE);
>    }
> 
> 24. Same thing as #23 in RejectException.java:36.
> 
> 25. In DerInputBuffer.java:58 and DerInputStream.java:117 to be 
> consistent the code should throw an IOException.
> 
> 26. In IssuingDistributionPointExtension.java:199 add some exception 
> message, for example "Unable to get unaligned bit string."
> 
> 27. Formatting issue in JSSUtil.java:71-72. Also, the exception message 
> probably should say "Cannot decode the given bytes."
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: pki-akoneru-0011-2-Fixes-For-Review-Comments-For-NR2.patch
Type: text/x-patch
Size: 28761 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20120612/eb4a5d5d/attachment.bin>