[Pki-devel] [PATCH] 18, 19 - Fixes for Guarded_By_Violation[18] and Unfinished NULL_RETURNS and RESOURCE_LEAKS[19] cases in Coverity for DogTag10

[Endi Sukma Dewata]

On 6/28/2012 11:38 AM, Abhishek Koneru wrote:
> Please review the patches 18 and 19 with fixes for
> Guarded_By_Violation[18] and Remaining Null_Returns and ResourceLeaks
> cases in Coverity for DogTag10.

As discussed over IRC, the GUARDED_BY_VIOLATION issue happens if a field 
is synchronized in one location but not in other location. This can be 
fixed by adding synchronized block, synchronizing the whole method, or 
removing the original synchronization (if it's not actually needed). 
However, if we make the synchronization too broad, it may include other 
fields that weren't originally synchronized, which might trigger new 
GUARDED_BY_VIOLATION issues.

For now I think we should focus on synchronizing the offending fields 
only as reported by Coverity because if we start synchronizing other 
fields it may have some unintended consequences. If there are fields 
that should have been synchronized but they aren't, they should 
investigated & fixed in separate tickets.

For patch #18 the following methods should be fixed to lock 'this' 
object and synchronize the following fields only:

* ARequestNotifier.recoverPublishingQueue(): mSearchForRequests
* CrossCertPairSubsystem.init(): mPublisherProcessor
* CertificateRepository.setCertStatusUpdateInterval(): requestRepository
* DBVirtualList.setSortKey(): mRegistry
* KeyRepository.setKeyStatusUpdateInterval(): requestRepository
* MD5.init(): buffer

For patch #19:

1. In CertificateAuthority.java:1899,1943 it throws an exception with 
log message instead of user message. I don't see a suitable user message 
for this so it might be better to just re-throw the original exception 
and let the caller handle it.

2. In FileBasedPublisher.java:384 the fos.close() shouldn't be moved 
because subsequent code depends on it. I think the "fos" instances in 
this method should be handled by separate try-finally blocks.

3. In KeyRepository.java:518 the patch checks for null recList but this 
might not sufficient because in line 534 the recList is read again. It 
might be better to throw EBaseException if it's null.

-- 
Endi S. Dewata