3:47 p.m.
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