On 6/20/2012 12:59 PM, Abhishek Koneru wrote: > Please find attached the patch with fixes for remaining issues w.r.t > resource leaks as shown in Coverity for Dogtag 10. Some comments: 1. In CertificateAuthority.java:1336 the code should check for null value in case the FileInputStream constructor fails. 2. Same thing in PWsdrCache.java:337,439. 3. In HttpClient.java:95 the in.close() should be done in a finally block in case the in.read() fails. 4. Formatting issue in HttpClient.java:109-237. 5. In HttpClient.java:166,180 the socket shouldn't be closed there because the "is" and "dos" streams are still using the socket. I think it should be closed in the finally block in line 240. 6. Like in the previous patch, in OCSPClient.java:259 the close() should be called in a finally block. 7. Same thing in PasswordCache.java:198. 8. In StorageKeyUnit.java:280 the patch should check for null "fi" in case an error happens before it's initialized. 9. Formatting issue in Utils.java:171,179. Also remove the auto-generated comment in line 175. 10. In Utils.java:168 the original code catches and swallows the exception. Ideally the exception should be handled by the caller. It looks like the copy() method isn't actually used, so you can modify the catch block to re-throw the exception, and the close() should be done in a finally block. 11. In PresenceServerExtension.java:314 the close() is no longer necessary. 12. To be consistent we should close the streams in a finally block in the following code: - PresenceServerExtension.java:326 - CRLDistributionPoint.java:341 - CRLDistributionPointsExtension.java:297 - FreshestCRLExtension.java:302 - IssuingDistributionPoint.java:313 - IssuingDistributionPointExtension.java:419

On 6/22/2012 11:12 AM, Abhishek Koneru wrote: > Please find attached the patch with fixes for review comments given for > PATCH 15 for review. A few more issues: 13. In PasswordCache.java:204 the "r" should be checked for null because the FileReader() constructor could fail. 14. In HttpClient.java:173-180 there's a new try-catch block that swallows the exception, changing the original behavior. The original code would let the caller handle the exception. This code was added in the first patch but only partially reverted in the second patch. It's always good to compare the cumulative patch against the original code (e.g. git diff <orig>). The try-catch block in line 121-170 doesn't seem to be correct either. I think it should let the exception be handled by the caller. Feel free to fix this too, or just leave it as is because this is outside the scope of this patch.