Please find attached the patch with fixes for issues in the previous
patch[#10] for review.
--Abhishek Koneru
On Mon, 2012-06-04 at 14:43 -0400, Abhishek Koneru wrote:
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.
>