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

Endi Sukma Dewata edewata at redhat.com
Mon Jun 11 20:47:21 UTC 2012


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




More information about the Pki-devel mailing list