[Pki-devel] [PATCH]10-2 Fixes for Some of the Coverity Issues under NULL_RETURNS category for Dog Tag 10 for review and commit

Please find attached the fixes addressing all the review comments. The build passed the smoke test. Regards, Abhishek Koneru On Fri, 2012-06-01 at 12:20 -0500, Endi Sukma Dewata wrote: > 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. >