[PATCH 11] Dogtag 10 - Fixes for Coverity issues relating to NULL_RETURNS Category -- for review
Please find attached [PATCH 11] with fixes for Coverity issues of category NULL RETURNS for DogTag 10. The build passed smoke test. Regards, Abhishek Koneru
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." -- Endi S. Dewata
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."
Submitting the PATCH 11-2 with fixes for review comments given. 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."
On 6/12/2012 4:53 PM, Abhishek Koneru wrote:
Submitting the PATCH 11-2 with fixes for review comments given. Build passed the smoke test.
1. The link to JSS jar file should have been /usr/share/java/jss/jss4.jar. This link is created by scripts/dev_setup.
The .classpath is not fixed yet.
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);
The new code shows the constant name (PROP_NEXT_RANGE) instead of the actual attribute name (nextRange): throw new Exception( "Missing Attribute PROP_NEXT_RANGE in Entry" + dn);
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.
The new code gets the message from LogMessages instead of UserMessages: throw new EKRAException( CMS.getLogMessage("CMS_KRA_INVALID_KEYRECORD")); Some more issues: 28. In IAuthToken.java:218 the @return tag should be fixed, the method no longer returns null on error. 29. In IService.java:47 the new @throws tag is no longer needed. 30. Formatting issue in CMCAuth.java:907. 31. In ChallengeRevocationServlet1.java:135 the @throws tag doesn't match the exception. 32. In AuthTokenTest.java:144 include an exception message, for example: Unable to get key as String array. 33. In RecoveryService.java:132 the new @throws tag is no longer needed. 34. In CMSLDAP.java:212 it should throw the original exception object. 35. In HTTPClient.java:424 the inner try-catch block is actually not needed anymore. The finally-clause (lines 432-441) can be moved to the outer try-catch block at line 447. The null assignments (lines 438-440) are not necessary because these objects will be garbage collected anyway. Usually objects that require closing are handled like this: // declare variables and initialize with null Socket socket = null; OutputStream rawos = null; BufferedOutputStream os = null; PrintStream ps = null; try { // create objects socket = new Socket(hostname, port); rawos = socket.getOutputStream(); os = new BufferedOutputStream(rawos); ps = new PrintStream(os); ... } catch (Exception e) { ... throw e; } finally { // close in reverse order and ignore any errors if (ps != null) try { ps.close(); } catch (Exception e) { e.printStackTrace(); } if (rawos != null) try { rawos.close(); } catch (Exception e) { e.printStackTrace(); } if (os != null) try { os.close(); } catch (Exception e) { e.printStackTrace(); } if (socket != null) try { socket.close(); } catch (Exception e) { e.printStackTrace(); } } You probably can just close the outermost stream (the one created last, i.e. ps), it should automatically close the other streams and the socket too. 36. Formatting issue in HTTPClient.java:1196. 37. In DerInputStream.java:115 the @throws tag doesn't match the exception. -- Endi S. Dewata
Please find attached PATCH 11-3 for review and commit. All the fixes for review comments included. --Abhishek Koneru On Tue, 2012-06-12 at 23:15 -0500, Endi Sukma Dewata wrote:
On 6/12/2012 4:53 PM, Abhishek Koneru wrote:
Submitting the PATCH 11-2 with fixes for review comments given. Build passed the smoke test.
1. The link to JSS jar file should have been /usr/share/java/jss/jss4.jar. This link is created by scripts/dev_setup.
The .classpath is not fixed yet.
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);
The new code shows the constant name (PROP_NEXT_RANGE) instead of the actual attribute name (nextRange):
throw new Exception( "Missing Attribute PROP_NEXT_RANGE in Entry" + dn);
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.
The new code gets the message from LogMessages instead of UserMessages:
throw new EKRAException( CMS.getLogMessage("CMS_KRA_INVALID_KEYRECORD"));
Some more issues:
28. In IAuthToken.java:218 the @return tag should be fixed, the method no longer returns null on error.
29. In IService.java:47 the new @throws tag is no longer needed.
30. Formatting issue in CMCAuth.java:907.
31. In ChallengeRevocationServlet1.java:135 the @throws tag doesn't match the exception.
32. In AuthTokenTest.java:144 include an exception message, for example: Unable to get key as String array.
33. In RecoveryService.java:132 the new @throws tag is no longer needed.
34. In CMSLDAP.java:212 it should throw the original exception object.
35. In HTTPClient.java:424 the inner try-catch block is actually not needed anymore. The finally-clause (lines 432-441) can be moved to the outer try-catch block at line 447. The null assignments (lines 438-440) are not necessary because these objects will be garbage collected anyway. Usually objects that require closing are handled like this:
// declare variables and initialize with null Socket socket = null; OutputStream rawos = null; BufferedOutputStream os = null; PrintStream ps = null;
try { // create objects socket = new Socket(hostname, port); rawos = socket.getOutputStream(); os = new BufferedOutputStream(rawos); ps = new PrintStream(os); ...
} catch (Exception e) { ... throw e;
} finally { // close in reverse order and ignore any errors if (ps != null) try { ps.close(); } catch (Exception e) { e.printStackTrace(); } if (rawos != null) try { rawos.close(); } catch (Exception e) { e.printStackTrace(); } if (os != null) try { os.close(); } catch (Exception e) { e.printStackTrace(); } if (socket != null) try { socket.close(); } catch (Exception e) { e.printStackTrace(); } }
You probably can just close the outermost stream (the one created last, i.e. ps), it should automatically close the other streams and the socket too.
36. Formatting issue in HTTPClient.java:1196.
37. In DerInputStream.java:115 the @throws tag doesn't match the exception.
On 6/13/2012 11:04 AM, Abhishek Koneru wrote:
Please find attached PATCH 11-3 for review and commit. All the fixes for review comments included.
ACK. Merged & pushed to master. -- Endi S. Dewata
participants (2)
-
Abhishek Koneru -
Endi Sukma Dewata