4:53 p.m.
Please find attached [PATCH 11] with fixes for Coverity issues of
category NULL RETURNS for DogTag 10.
The build passed smoke test.
Regards,
Abhishek Koneru
3:47 p.m.
On 6/8/2012 4:53 PM, Abhishek Koneru wrote:
Please find attached [PATCH 11] with fixes for Coverity issues of
category NULL RETURNS for DogTag 10.
Some issues:
1. The link to JSS jar file should have been
/usr/share/java/jss/jss4.jar. This link is created by scripts/dev_setup.
2. In AuthToken.java:303,341,343,390 the code catches the exception and
throws a new exception but with the same type and message. This is not
necessary and it will change the stack trace information (it will no
longer show where the original exception happens). It's better to simply
not catch the exception and let it be handled by the caller.
There are cases where catch-and-throw make sense. If you want to do
something but still want to keep the original stack trace you can
re-throw the same exception object (not create a new one):
} catch (Exception e) {
// do something
throw e;
}
If you want to change the exception type/message you can throw a new
exception that wraps the old exception:
} catch (OldException e) {
throw new NewException(e);
// or throw new NewException(e.getMessage());
}
Wrapping is possible if the NewException has a constructor that takes
the OldException. This is to allow chaining the stack trace. If such
constructor is not available at least try to include the original
exception message.
3. In CMCAuth.java:877 the exception should throw the
CMS_AUTHENTICATION_MANAGER_NOT_FOUND message from UserMessages.
4. Similar to #2, in CMCAuth.java:907 it's not necessary to catch and
throw a new exception with the same type/message.
5. While you're at it, in SubjAltNameExt.java:255, the /* of String */
comment can be removed since we're using generics now.
6. In DisplayHtmlServlet.java:65 moving the authenticate() into the
try-catch block doesn't seem to be necessary. The authenticate() throws
an EBaseException with is already declared by the process(). The
try-catch block is intended to deal with template problem, which is not
the case with authenticate().
7. There's a formatting issue in ChallengeRevocationServlet1.java:138.
8. In UpdateCRL.java:123 the authenticate() is moved into a try-catch
block which will swallow all exceptions. I think the null return value
should be checked before the try-catch block.
9. In LDAPSecurityDomainSessionTable.java:195 you can reuse the
LDAPAttribute object obtained for null checking. Also, it would be
better to indicate which entry is invalid in the exception message.
LDAPAttribute sid = entry.getAttribute("cn");
if (sid == null) {
throw new Exception("Invalid LDAP Entry " + entry.getDN()
+ ". No session id (cn).");
}
ret.add(sid.getStringValueArray()[0]);
Similarly, add the entry DN into the exception message in line 237.
10. In AuthSubsystem.java:461 you can iterate over hashtable values
directly, thus avoiding the lookup operation:
for (AuthManagerProxy proxy : mAuthMgrInsts.values()) {
IAuthManager mgr = proxy.getAuthManager();
...
}
11. In PasswdUserDBAuthentication.java:188 the getUser() will return
null only if there's a problem. However, if you throw an exception in
line 190 it will be logged as "UID not found", which is not the case. I
think the null checking should be moved after the try-catch block like this:
try {
user = ug.getUser(uid);
} catch (...) {
...
}
if (user == null) {
throw new EInvalidCredentials(
CMS.getUserMessage("CMS_AUTHENTICATION_INTERNAL_ERROR",
"Failure in User Group subsystem."));
}
Ideally the getUser() should throw an exception instead of returning
null. That would be fixed under ticket #201.
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);
13. Formatting issue in KeyRepository.java:254. Also, include the serial
number in the exception message in line 272:
throw new EBaseException(
"Failed to recover Key for Serial Number " + serialNo);
14. In ARequestQueue.java:591 the boolean initialization is not
necessary because it will be initialized with the return value of
serviceRequest(). Also remove the auto-generated TODO comment.
15. In AuthTokenTest.java:122 it might be better to use this message:
throw new Exception("Unable to get key2 as Date");
16. Formatting issue in AuthTokenTest.java:143.
17. In DRMTool.java:1645 the readLine() will only return null if the
password file is empty, which I suppose is not allowed. So I think it
should throw an exception instead of continuing with empty password.
18. Same issue in PKCS12Export.java:227,239 as in #17.
19. In KRAService.java:101 remove the auto-generated TODO message.
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.
21. As explained in #2, in HTTPClient.java:431,448 you can throw the
original exception object.
22. Also like in #2, in HTTPClient.java:1198 you could leave the
exception uncaught or throw the original exception object, but remove
the auto-generated TODO message in line 1201.
23. In ChallengeException.java:36,45, the original code would have
thrown an exception. The new code will continue and return an empty
string. It might be better to return the attribute directly and let the
caller handle the null value:
public StateAttribute getState() {
return _res.getAttributeSet().getAttributeByType(
Attribute.STATE);
}
public ReplyMessageAttribute getReplyMessage() {
return _res.getAttributeSet().getAttributeByType(
Attribute.REPLY_MESSAGE);
}
24. Same thing as #23 in RejectException.java:36.
25. In DerInputBuffer.java:58 and DerInputStream.java:117 to be
consistent the code should throw an IOException.
26. In IssuingDistributionPointExtension.java:199 add some exception
message, for example "Unable to get unaligned bit string."
27. Formatting issue in JSSUtil.java:71-72. Also, the exception message
probably should say "Cannot decode the given bytes."
--
Endi S. Dewata
4:48 p.m.
New subject: [ PATCH 11-2 ] --- Re: [PATCH 11] Dogtag 10 - Fixes for Coverity issues relating to NULL_RETURNS Category -- for review
Submitting patch 11-2 for review.
All the comments have been addressed. Build passed the smoke test.
Thank you.
Regards,
Abhishek Koneru
On Mon, 2012-06-11 at 15:47 -0500, Endi Sukma Dewata wrote:
On 6/8/2012 4:53 PM, Abhishek Koneru wrote:
> Please find attached [PATCH 11] with fixes for Coverity issues of
> category NULL RETURNS for DogTag 10.
Some issues:
1. The link to JSS jar file should have been
/usr/share/java/jss/jss4.jar. This link is created by scripts/dev_setup.
2. In AuthToken.java:303,341,343,390 the code catches the exception and
throws a new exception but with the same type and message. This is not
necessary and it will change the stack trace information (it will no
longer show where the original exception happens). It's better to simply
not catch the exception and let it be handled by the caller.
There are cases where catch-and-throw make sense. If you want to do
something but still want to keep the original stack trace you can
re-throw the same exception object (not create a new one):
} catch (Exception e) {
// do something
throw e;
}
If you want to change the exception type/message you can throw a new
exception that wraps the old exception:
} catch (OldException e) {
throw new NewException(e);
// or throw new NewException(e.getMessage());
}
Wrapping is possible if the NewException has a constructor that takes
the OldException. This is to allow chaining the stack trace. If such
constructor is not available at least try to include the original
exception message.
3. In CMCAuth.java:877 the exception should throw the
CMS_AUTHENTICATION_MANAGER_NOT_FOUND message from UserMessages.
4. Similar to #2, in CMCAuth.java:907 it's not necessary to catch and
throw a new exception with the same type/message.
5. While you're at it, in SubjAltNameExt.java:255, the /* of String */
comment can be removed since we're using generics now.
6. In DisplayHtmlServlet.java:65 moving the authenticate() into the
try-catch block doesn't seem to be necessary. The authenticate() throws
an EBaseException with is already declared by the process(). The
try-catch block is intended to deal with template problem, which is not
the case with authenticate().
7. There's a formatting issue in ChallengeRevocationServlet1.java:138.
8. In UpdateCRL.java:123 the authenticate() is moved into a try-catch
block which will swallow all exceptions. I think the null return value
should be checked before the try-catch block.
9. In LDAPSecurityDomainSessionTable.java:195 you can reuse the
LDAPAttribute object obtained for null checking. Also, it would be
better to indicate which entry is invalid in the exception message.
LDAPAttribute sid = entry.getAttribute("cn");
if (sid == null) {
throw new Exception("Invalid LDAP Entry " + entry.getDN()
+ ". No session id (cn).");
}
ret.add(sid.getStringValueArray()[0]);
Similarly, add the entry DN into the exception message in line 237.
10. In AuthSubsystem.java:461 you can iterate over hashtable values
directly, thus avoiding the lookup operation:
for (AuthManagerProxy proxy : mAuthMgrInsts.values()) {
IAuthManager mgr = proxy.getAuthManager();
...
}
11. In PasswdUserDBAuthentication.java:188 the getUser() will return
null only if there's a problem. However, if you throw an exception in
line 190 it will be logged as "UID not found", which is not the case. I
think the null checking should be moved after the try-catch block like this:
try {
user = ug.getUser(uid);
} catch (...) {
...
}
if (user == null) {
throw new EInvalidCredentials(
CMS.getUserMessage("CMS_AUTHENTICATION_INTERNAL_ERROR",
"Failure in User Group subsystem."));
}
Ideally the getUser() should throw an exception instead of returning
null. That would be fixed under ticket #201.
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);
13. Formatting issue in KeyRepository.java:254. Also, include the serial
number in the exception message in line 272:
throw new EBaseException(
"Failed to recover Key for Serial Number " + serialNo);
14. In ARequestQueue.java:591 the boolean initialization is not
necessary because it will be initialized with the return value of
serviceRequest(). Also remove the auto-generated TODO comment.
15. In AuthTokenTest.java:122 it might be better to use this message:
throw new Exception("Unable to get key2 as Date");
16. Formatting issue in AuthTokenTest.java:143.
17. In DRMTool.java:1645 the readLine() will only return null if the
password file is empty, which I suppose is not allowed. So I think it
should throw an exception instead of continuing with empty password.
18. Same issue in PKCS12Export.java:227,239 as in #17.
19. In KRAService.java:101 remove the auto-generated TODO message.
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.
21. As explained in #2, in HTTPClient.java:431,448 you can throw the
original exception object.
22. Also like in #2, in HTTPClient.java:1198 you could leave the
exception uncaught or throw the original exception object, but remove
the auto-generated TODO message in line 1201.
23. In ChallengeException.java:36,45, the original code would have
thrown an exception. The new code will continue and return an empty
string. It might be better to return the attribute directly and let the
caller handle the null value:
public StateAttribute getState() {
return _res.getAttributeSet().getAttributeByType(
Attribute.STATE);
}
public ReplyMessageAttribute getReplyMessage() {
return _res.getAttributeSet().getAttributeByType(
Attribute.REPLY_MESSAGE);
}
24. Same thing as #23 in RejectException.java:36.
25. In DerInputBuffer.java:58 and DerInputStream.java:117 to be
consistent the code should throw an IOException.
26. In IssuingDistributionPointExtension.java:199 add some exception
message, for example "Unable to get unaligned bit string."
27. Formatting issue in JSSUtil.java:71-72. Also, the exception message
probably should say "Cannot decode the given bytes."
4:53 p.m.
New subject: [PATCH 11-2] -- Re: [PATCH 11] Dogtag 10 - Fixes for Coverity issues relating to NULL_RETURNS Category -- for review
Submitting the PATCH 11-2 with fixes for review comments given.
Build passed the smoke test.
Thank you.
Regards,
Abhishek Koneru
On Mon, 2012-06-11 at 15:47 -0500, Endi Sukma Dewata wrote:
On 6/8/2012 4:53 PM, Abhishek Koneru wrote:
> Please find attached [PATCH 11] with fixes for Coverity issues of
> category NULL RETURNS for DogTag 10.
Some issues:
1. The link to JSS jar file should have been
/usr/share/java/jss/jss4.jar. This link is created by scripts/dev_setup.
2. In AuthToken.java:303,341,343,390 the code catches the exception and
throws a new exception but with the same type and message. This is not
necessary and it will change the stack trace information (it will no
longer show where the original exception happens). It's better to simply
not catch the exception and let it be handled by the caller.
There are cases where catch-and-throw make sense. If you want to do
something but still want to keep the original stack trace you can
re-throw the same exception object (not create a new one):
} catch (Exception e) {
// do something
throw e;
}
If you want to change the exception type/message you can throw a new
exception that wraps the old exception:
} catch (OldException e) {
throw new NewException(e);
// or throw new NewException(e.getMessage());
}
Wrapping is possible if the NewException has a constructor that takes
the OldException. This is to allow chaining the stack trace. If such
constructor is not available at least try to include the original
exception message.
3. In CMCAuth.java:877 the exception should throw the
CMS_AUTHENTICATION_MANAGER_NOT_FOUND message from UserMessages.
4. Similar to #2, in CMCAuth.java:907 it's not necessary to catch and
throw a new exception with the same type/message.
5. While you're at it, in SubjAltNameExt.java:255, the /* of String */
comment can be removed since we're using generics now.
6. In DisplayHtmlServlet.java:65 moving the authenticate() into the
try-catch block doesn't seem to be necessary. The authenticate() throws
an EBaseException with is already declared by the process(). The
try-catch block is intended to deal with template problem, which is not
the case with authenticate().
7. There's a formatting issue in ChallengeRevocationServlet1.java:138.
8. In UpdateCRL.java:123 the authenticate() is moved into a try-catch
block which will swallow all exceptions. I think the null return value
should be checked before the try-catch block.
9. In LDAPSecurityDomainSessionTable.java:195 you can reuse the
LDAPAttribute object obtained for null checking. Also, it would be
better to indicate which entry is invalid in the exception message.
LDAPAttribute sid = entry.getAttribute("cn");
if (sid == null) {
throw new Exception("Invalid LDAP Entry " + entry.getDN()
+ ". No session id (cn).");
}
ret.add(sid.getStringValueArray()[0]);
Similarly, add the entry DN into the exception message in line 237.
10. In AuthSubsystem.java:461 you can iterate over hashtable values
directly, thus avoiding the lookup operation:
for (AuthManagerProxy proxy : mAuthMgrInsts.values()) {
IAuthManager mgr = proxy.getAuthManager();
...
}
11. In PasswdUserDBAuthentication.java:188 the getUser() will return
null only if there's a problem. However, if you throw an exception in
line 190 it will be logged as "UID not found", which is not the case. I
think the null checking should be moved after the try-catch block like this:
try {
user = ug.getUser(uid);
} catch (...) {
...
}
if (user == null) {
throw new EInvalidCredentials(
CMS.getUserMessage("CMS_AUTHENTICATION_INTERNAL_ERROR",
"Failure in User Group subsystem."));
}
Ideally the getUser() should throw an exception instead of returning
null. That would be fixed under ticket #201.
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);
13. Formatting issue in KeyRepository.java:254. Also, include the serial
number in the exception message in line 272:
throw new EBaseException(
"Failed to recover Key for Serial Number " + serialNo);
14. In ARequestQueue.java:591 the boolean initialization is not
necessary because it will be initialized with the return value of
serviceRequest(). Also remove the auto-generated TODO comment.
15. In AuthTokenTest.java:122 it might be better to use this message:
throw new Exception("Unable to get key2 as Date");
16. Formatting issue in AuthTokenTest.java:143.
17. In DRMTool.java:1645 the readLine() will only return null if the
password file is empty, which I suppose is not allowed. So I think it
should throw an exception instead of continuing with empty password.
18. Same issue in PKCS12Export.java:227,239 as in #17.
19. In KRAService.java:101 remove the auto-generated TODO message.
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.
21. As explained in #2, in HTTPClient.java:431,448 you can throw the
original exception object.
22. Also like in #2, in HTTPClient.java:1198 you could leave the
exception uncaught or throw the original exception object, but remove
the auto-generated TODO message in line 1201.
23. In ChallengeException.java:36,45, the original code would have
thrown an exception. The new code will continue and return an empty
string. It might be better to return the attribute directly and let the
caller handle the null value:
public StateAttribute getState() {
return _res.getAttributeSet().getAttributeByType(
Attribute.STATE);
}
public ReplyMessageAttribute getReplyMessage() {
return _res.getAttributeSet().getAttributeByType(
Attribute.REPLY_MESSAGE);
}
24. Same thing as #23 in RejectException.java:36.
25. In DerInputBuffer.java:58 and DerInputStream.java:117 to be
consistent the code should throw an IOException.
26. In IssuingDistributionPointExtension.java:199 add some exception
message, for example "Unable to get unaligned bit string."
27. Formatting issue in JSSUtil.java:71-72. Also, the exception message
probably should say "Cannot decode the given bytes."
11:15 p.m.
New subject: [PATCH 11-2] -- Re: [PATCH 11] Dogtag 10 - Fixes for Coverity issues relating to NULL_RETURNS Category -- for review
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
11:04 a.m.
New subject: [PATCH 11-3] -- Re: [PATCH 11-2] -- Re: [PATCH 11] Dogtag 10 - Fixes for Coverity issues relating to NULL_RETURNS Category -- for review
Please find attached PATCH 11-3 for review and commit.
All the fixes for review comments included.
--Abhishek Koneru
On Tue, 2012-06-12 at 23:15 -0500, Endi Sukma Dewata wrote:
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.
5:16 p.m.
New subject: [PATCH 11-3] -- Re: [PATCH 11-2] -- Re: [PATCH 11] Dogtag 10 - Fixes for Coverity issues relating to NULL_RETURNS Category -- for review
On 6/13/2012 11:04 AM, Abhishek Koneru wrote:
Please find attached PATCH 11-3 for review and commit.
All the fixes for review comments included.
ACK. Merged & pushed to master.
--
Endi S. Dewata
4344
days inactive
4350
days old
6 comments
2 participants
participants (2)
-
Abhishek Koneru -
Endi Sukma Dewata