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

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."