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

On 5/31/2012 5:25 PM, Abhishek Koneru wrote: 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