FindBugs vs CDK

I am always interested in reading posts from PVS-Studio about how they search for bugs in some open source project. I decided that I could also write such a post, only about Java. There is a completely wonderful free static analyzer Java-code FindBugs . About him surprisingly little written on Habré.

In addition to the code analyzer, this article requires a guinea pig. We need a fairly large project, but it is not so common that the developers ideally lick the code. I chose the Chemistry Development Kit project (version 1.4.19), which I used to use. I installed FindBugs as a plugin for Eclipse, because I am so used to it.



FindBugs with default settings found 338 problems. Of course, I will not describe them all, I will focus on interesting ones.
')

1. Unsuccessful attempt to get a random positive number.

In the org.openscience.cdk.math.RandomNumbersTool class:

public static int randomInt(int lo, int hi) { return (Math.abs(random.nextInt()) % (hi - lo + 1)) + lo; } 

The Random.nextInt () method returns any number that is valid in int. The Math.abs method gets the modulus of a number. The problem is that Math.abs does not work for Integer.MIN_VALUE, because, as you know, the opposite of this number does not fit into the int. However, Random.nextInt () may well produce this number (about once per 4 billion), then the whole method will work incorrectly.

2. The result BufferedReader.readLine () is not checked for null

It occurs many times, for example, in this form (org.openscience.cdk.io.CIFReader):

 private void skipUntilEmptyOrCommentLine(String line) throws IOException { // skip everything until empty line, or comment line while (line != null && line.length() > 0 && line.charAt(0) != '#') { line = input.readLine().trim(); } } 

The input stream is not checked for readiness by the ready () method, and the result of readLine () is not checked for null. As a result, a malformed input file will cause a NullPointerException.

3. Used & instead of &&

A trivial error of this type (org.openscience.cdk.atomtype.CDKAtomTypeMatcher):

 if (atom.getFormalCharge() != null & atom.getFormalCharge().intValue() == 1) {...} 

If the first check fails, the second check will be performed anyway and will result in a NullPointerException.

4. Comparing String, Integer or Double Objects ==

It occurs many times. For example, here (org.openscience.cdk.AtomType.compare):

 return (getAtomTypeName() == type.getAtomTypeName()) && (maxBondOrder == type.maxBondOrder) && (bondOrderSum == type.bondOrderSum); 

getAtomTypeName () returns a String, and bondOrderSum is of type Double. The application logic completely assumes that there will be different, but equal equals objects and the comparison will work incorrectly.

It is generally undesirable to use Integer, Double, and so on objects, unless you have a good reason to use them.

5. The programmer has forgotten that the strings are constant.

There are calls to methods of the String class that create a new string. For example (net.sf.cdk.tools.MakeJavafilesFiles.readBlackList):

 while (line != null) { line.trim(); if (line.length() > 0) blacklist.add(line); line = reader.readLine(); } 

Calling line.trim () is useless, as it does not change the line itself, and no one uses the result. The author obviously meant line = line.trim (). Similarly, there are calls to String.substring without saving the result.

6. Comparison of objects of different types

Frequent comparisons are in the spirit of if( atom.equals("H") ) , where the atom is an IAtom type. It is implied, apparently, if( atom.getSymbol().equals("H") ) . In general, this is a mysterious place, since there are more than a dozen such errors, and, in my opinion, they should greatly influence the semantics and distort the result.

7. Using an Uninitialized Field

org.openscience.cdk.dict.EntryReact:

 public void setReactionMetadata(String metadata) { this.reactionInfo.add( metadata ); } 

FindBugs determines that the private field reactionInfo is not initialized in any other method, so this method will always throw a NullPointerException.

8. Incorrect initialization of a static field.

For example, the org.openscience.cdk.qsar.AtomValenceTool class:

 public class AtomValenceTool { private static Map<String,Integer> valencesTable = null; public static int getValence(IAtom atom) { if (valencesTable == null) { valencesTable = new HashMap<String, Integer>(); valencesTable.put("H", 1); valencesTable.put("He", 8); valencesTable.put("Ne", 8); ... } return valencesTable.get(atom.getSymbol()); } } 

When called from different streams, race condition is possible, when valencesTable is no longer null, but not yet full. Then one thread will throw a NullPointerException for a completely correct atom.

9. Breach of contract

The equals () method must return false if the argument is null. The clone () method should never return null. The clone () method not in the final class should call super.clone (), and not create an object manually (otherwise, if you inherit the class, then clone () will break). Such things may not lead to errors, but they should still be avoided.

10. Incorrect use of regular expression

net.sf.cdk.tools.doclets.CDKIOOptionsTaglet.getClassName:

 String path = file.getPath().replaceAll(File.separator, "."); 

The replaceAll method takes a regular expression as an argument. Under Windows, File.separator is a backslash that is specially interpreted in regular expressions, so this code will fall from a PatternSyntaxException.

11. The overridden method from the parent constructor uses an uninitialized variable.

An interesting situation in the class org.openscience.cdk.debug.DebugAtomContainer. There is a field announced

 ILoggingTool logger = LoggingToolFactory.createLoggingTool(DebugAtomContainer.class); 

and there is a method:

 public void addStereoElement(IStereoElement parity) { logger.debug("Adding stereo element: ", parity); super.addStereoElement(parity); } 

The problem is that this method is called in one of the constructors of the base class, when assigning the value to the variable logger has not yet worked. As a result, a NullPointerException will naturally occur.

Conclusion

There were more mistakes, but we will stop. I want to note that the CDK is a good library, which generally works fine. And quite a lot of problems were found, not because programmers are stupid. Normal programmers, everyone writes that. Simply, they, apparently, have not yet used static code analyzers. And you use, it is useful!