[Pki-devel] [PATCH 11-2] -- Re: [PATCH 11] Dogtag 10 - Fixes for Coverity issues relating to NULL_RETURNS Category -- for review

[Endi Sukma Dewata]

On 6/12/2012 4:53 PM, Abhishek Koneru wrote:
> Submitting the PATCH 11-2 with fixes for review comments given.
> Build passed the smoke test.

>> 1. The link to JSS jar file should have been
>> /usr/share/java/jss/jss4.jar. This link is created by scripts/dev_setup.

The .classpath is not fixed yet.

>> 12. There's a formatting issue in DBSubsystem.java:411. Also, it would
>> be better to include which entry that has a problem:
>>
>>     throw new Exception(
>>         "Missing attribute " + PROP_NEXT_RANGE + " in entry " + dn);

The new code shows the constant name (PROP_NEXT_RANGE) instead of the 
actual attribute name (nextRange):

        throw new Exception(
            "Missing Attribute PROP_NEXT_RANGE in Entry" + dn);

>> 20. In RecoveryService.java:200 it might be better to throw
>> EKRAException with user message CMS_KRA_INVALID_KEYRECORD. This may make
>> some other changes unnecessary.

The new code gets the message from LogMessages instead of UserMessages:

   throw new EKRAException(
       CMS.getLogMessage("CMS_KRA_INVALID_KEYRECORD"));


Some more issues:

28. In IAuthToken.java:218 the @return tag should be fixed, the method 
no longer returns null on error.

29. In IService.java:47 the new @throws tag is no longer needed.

30. Formatting issue in CMCAuth.java:907.

31. In ChallengeRevocationServlet1.java:135 the @throws tag doesn't 
match the exception.

32. In AuthTokenTest.java:144 include an exception message, for example: 
Unable to get key as String array.

33. In RecoveryService.java:132 the new @throws tag is no longer needed.

34. In CMSLDAP.java:212 it should throw the original exception object.

35. In HTTPClient.java:424 the inner try-catch block is actually not 
needed anymore. The finally-clause (lines 432-441) can be moved to the 
outer try-catch block at line 447. The null assignments (lines 438-440) 
are not necessary because these objects will be garbage collected 
anyway. Usually objects that require closing are handled like this:

   // declare variables and initialize with null
   Socket socket = null;
   OutputStream rawos = null;
   BufferedOutputStream os = null;
   PrintStream ps = null;

   try {
       // create objects
       socket = new Socket(hostname, port);
       rawos = socket.getOutputStream();
       os = new BufferedOutputStream(rawos);
       ps = new PrintStream(os);
       ...

   } catch (Exception e) {
       ...
       throw e;

   } finally {
       // close in reverse order and ignore any errors
       if (ps != null)
           try { ps.close(); }
           catch (Exception e) { e.printStackTrace(); }
       if (rawos != null)
           try { rawos.close(); }
           catch (Exception e) { e.printStackTrace(); }
       if (os != null)
           try { os.close(); }
           catch (Exception e) { e.printStackTrace(); }
       if (socket != null)
           try { socket.close(); }
           catch (Exception e) { e.printStackTrace(); }
   }

You probably can just close the outermost stream (the one created last, 
i.e. ps), it should automatically close the other streams and the socket 
too.

36. Formatting issue in HTTPClient.java:1196.

37. In DerInputStream.java:115 the @throws tag doesn't match the exception.

-- 
Endi S. Dewata