[Pki-devel] [PATCH]10-4 -- Minor Fix in [PATCH] 10-3

Tue Jun 5 19:18:39 UTC 2012

Please find attached a patch with a minor fix in CertificateInfo class
for review.

Regards,
Abhishek Koneru

On Tue, 2012-06-05 at 12:23 -0400, Abhishek Koneru wrote:
> Please find attached [PATCH] [10-3] for review.
> 
> Regards,
> Abhishsek Koneru
> 
> On Mon, 2012-06-04 at 16:10 -0400, Abhishek Koneru wrote:
> > 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.
> > > > 
> > > 
> > 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: pki-akoneru-0010-4-Fixes-for-NULL_RETURN-cases-review-comments.patch
Type: text/x-patch
Size: 1784 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20120605/0d97de5e/attachment.bin>