[Pki-devel] [PATCH] Fixes for Some of the Coverity Issues under NULL_RETURNS category for Dog Tag 10 for Review

[Endi Sukma Dewata]

On 5/31/2012 5:25 PM, Abhishek Koneru wrote:
> Please find attached the patch with fixes for some 40-50 defects of type
> NULL_RETURNS in Coverity for Dog Tag 10.
> Please review the patch
> The build passed Smoke Test.

Generally in Java a method should throw an exception if it encounters an 
error so the caller can know what the problem is and can handle it 
properly. A method should return null only if it couldn't find the 
object it's looking for. Some of the current code are doing this in 
C-style, returning null on error, so the caller will have to explicitly 
handle it. Fixing this behavior will require a major change, so for now 
we'd have to stick with the current behavior, but we should have a 
discussion about this.

1. In CAService.java:1503 the serialNoArray could be null if the serial 
number is not found or invalid. The exception message should say that 
too. To be consistent it should throw ECAException with user message 
CMS_CA_MISSING_SERIAL_NUMBER.

Ideally the getExtDataInBigIntegerArray() should throw an exception on 
invalid data.

2. In CRLIssuingPoint.java:1929,1949 the original code would throw an 
exception if the return value is null. The new code logs the error but 
continues processing the next request. I think they should throw an 
exception like in #1. Or, if the error is supposed to be ignored, please 
add a comment there.

Ideally the getExtDataInRevokedCertArray() should throw an exception on 
invalid data.

3. In CRLIssuingPoint.java:1949-1957 the code needs to be formatted 
properly. You can use Eclipse to auto format that section.

4. In StatsEvent.java:37 the mSubEvents should have been a Map instead 
of a Vector so we can look up by name easily. Please open a ticket for 
this, or feel free to include it in the next patch.

5. In CrossCertPairSubsystem.java:191,405 the code will throw an 
exception if getConn() returns null. Instead of checking for the return 
value in all invocations, it would be better to do it inside getConn() 
itself.

Ideally the LdapBoundConnFactory.getConn() should throw an exception if 
it cannot create a connection.

6. In HttpPKIMessage.java:77 instead of checking for null return value 
it might be better to change the RequestTransfer 
.getTransferAttributes() to always return an array (which could be empty 
but not null) like this:

   return v.toArray(new String[v.size()]);

This way the change in RequestTransfer.java:110 is no longer needed.

7. There's a typo in DBRegistry.java:460. To be consistent it should 
throw EDBException. The message should be added into 
base/common/src/UserMessages.properties, something like this:

   CMS_DBS_MISSING_OBJECT_CLASS=Missing object class

While you're at it.. :) the code in 472-481 could be simplified into:

   String[] s = attr.getStringValueArray();

Feel free to include it in your next patch or just file a ticket.

8. The LdapPredicateParser.nextToken() should never return null, so 
instead of checking the return value you can throw the exception in line 
330.

Same thing with PolicyPredicateParser.java:331.

9. In PublisherProcessor.java:495,539 instead of checking for null 
getType(), the comparison can be changed like this:

   publishingType.equals(rule.getType())

Also the loop itself can be simplified as follows:

   Enumeration<ILdapRule> e = mRuleInsts.elements();
   while (e.hasMoreElements()) {
       LdapRule rule = (LdapRule)e.nextElement();
       ...
   }

This way we don't need to worry about null name (which shouldn't happen 
anyway) so getRules() won't need to return null.

10. In LogSubsystem.java:206 the code was changed from catching 
EBaseException to catching all Exception. Any reason for that? This 
might be too broad, we should let real errors (e.g. RuntimeException) be 
handled by the caller. You can leave the e.printStackTrace() there if 
you want.

11. In CertificateInfo.java:197 if sigAlgId == null and algm == null it 
will not create new AlgorithmId so the sigAlgId will remain null, which 
is not what we want. The original code would have thrown an Exception. I 
think you should check the KeyCertUtil.getSigningAlgorithm() in line 
191, if it returns null you should throw NoSuchAlgorithmException.

12. Similar to #5, in UGSubsystem.java instead of checking each 
getConn() invocation you can check it inside getConn() itself and throw 
the exception there.

13. In EnrollmentService.java:738,791 you should use user message 
CMS_BASE_CERT_NOT_FOUND. The message itself in UserMessages.properties 
should be changed to:

     Certificate not found or invalid

Similarly, ideally the getExtDataInCertInfoArray() should throw an 
exception on invalid data.

14. In HttpMessage.java:120 the readLine() will return null if it 
reaches EOF. There's already a code there to detect this, but I'm not 
sure why it was commented out. I think we should uncomment it.

15. In PrettyPrintFormat.toHexString() the original code would throw an 
exception if the input byte array is empty. In the new code it will 
return a string that contain indentations only (line 122). Also there's 
still a possibility of an exception in line 115 if in == null and 
lineLen == 0.

It might be better to just check the array at the beginning of the 
method and throw an exception. Or don't change anything, it will throw 
an exception too. Was this problem reported by Coverity as NULL_RETURNS? 
I don't see any method returning null value here.

16. In KeyUsageExtension.java:213 the original constructor will fail if 
it couldn't get the bitString from the value. Now it will construct the 
KeyUsageExtension with null bitString. It might be better to check the 
return value of getUnalignedBitString() and throw IOException("Invalid 
bit string") or something like that.

-- 
Endi S. Dewata