FindBugs против CDK

Мне всегда интересно читать посты от PVS-Studio о том, как они ищут баги в каком-нибудь опенсорсном проекте. Я решил, что я тоже смогу написать такой пост, только про Java. Существует совершенно замечательный бесплатный статический анализатор Java-кода FindBugs. О нём на удивление мало писали на Хабре.

Помимо анализатора кода для такой статьи требуется подопытный кролик. Нужен довольно большой проект, но при этом не настолько распространённый, чтобы разработчики идеально вылизывали код. Я выбрал проект Chemistry Development Kit (версия 1.4.19), которым доводилось пользоваться. FindBugs я установил как плагин к Eclipse, потому что мне так привычнее.

FindBugs с настройками по умолчанию обнаружил 338 проблем. Конечно, я не буду описывать их все, остановлюсь на интересных.

1. Неудачная попытка получить случайное положительное число

В классе org.openscience.cdk.math.RandomNumbersTool:

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

Метод Random.nextInt() выдаёт любое число, допустимое в int. Метод Math.abs получает модуль числа. Проблема в том, что Math.abs не работает для Integer.MIN_VALUE, потому что, как известно, число, противоположное этому, не помещается в int. Однако Random.nextInt() вполне может выдать это число (примерно один раз на 4 миллиарда), тогда весь метод отработает неверно.

2. Результат BufferedReader.readLine() не проверяется на null

Встречается многократно, например, в таком виде (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();
    }
}

Входной поток не проверяется на готовность методом ready(), и результат readLine() не проверяется на null. В результате неправильно сформированный входной файл вызовет NullPointerException.

3. Используется & вместо &&

Тривиальная ошибка такого вида (org.openscience.cdk.atomtype.CDKAtomTypeMatcher):

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

Если первая проверка завершилась неудачно, вторая будет выполнена всё равно и приведёт к NullPointerException.

4. Сравнение объектов String, Integer или Double по ==

Встречается многократно. Например, тут (org.openscience.cdk.AtomType.compare):

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

getAtomTypeName() возвращает String, а bondOrderSum имеет тип Double. Логика приложения вполне допускает, что тут окажутся разные, но равные по equals объекты и сравнение отработает некорректно.

Вообще нежелательно использовать объекты Integer, Double и так далее, если у вас нет хорошей причины их использовать.

5. Программист забыл, что строки константны

Встречаются вызовы методов класса String, которые создают новую строку. Например (net.sf.cdk.tools.MakeJavafilesFiles.readBlackList):

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

Вызов line.trim() бесполезен, так как саму строку line он не меняет, а результат никто не использует. Автор явно имел в виду line = line.trim(). Аналогично встречаются вызовы String.substring без сохранения результата.

6. Сравнение объектов разных типов

Нередки сравнения в духе if( atom.equals("H") ), где atom типа IAtom. Подразумевается, видимо, if( atom.getSymbol().equals("H") ). Вообще это загадочное место, так как таких ошибок больше десятка, а, на мой взгляд, они должны сильно влиять на семантику и искажать результат.

7. Использование неинициализированного поля

org.openscience.cdk.dict.EntryReact:

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

FindBugs определяет, что приватное поле reactionInfo не инициализируется ни в каком другом методе, поэтому данный метод всегда выдаст NullPointerException.

8. Неправильная инициализация статического поля

К примеру, класс org.openscience.cdk.qsar.AtomValenceTool:

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());
    }
}

При вызове из разных потоков возможен race condition, когда valencesTable уже не null, но ещё не заполнена до конца. Тогда один поток выдаст NullPointerException для вполне корректного атома.

9. Нарушение контрактов

Метод equals() должен возвращать false, если аргумент null. Метод clone() не должен никогда возвращать null. Метод clone() не в final-классе должен super.clone(), а не создавать объект вручную (иначе если унаследовать класс, то clone() сломается). Подобные вещи могут не приводить к ошибкам, но всё же их следует избегать.

10. Неверное использование регулярного выражения

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

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

Метод replaceAll принимает в качестве аргумента регулярное выражение. Под Windows File.separator — это обратный слэш, который специально интерпретируется в регулярных выражениях, поэтому данный код упадёт с PatternSyntaxException.

11. Переопределённый метод из родительского конструктора использует неинициализированную переменную

Интересная ситуация в классе org.openscience.cdk.debug.DebugAtomContainer. Там объявлено поле

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

и есть метод:

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

Проблема в том, что этот метод вызывается в одном из конструкторов базового класса, когда присваивание значения переменной logger ещё не отработало. В результате, естественно, случится NullPointerException.

Заключение

Были и ещё ошибки, но остановимся. Хочу отметить, что CDK — хорошая библиотека, которая в целом нормально работает. И было найдено довольно много проблем не потому, что программисты глупые. Нормальные программисты, все так пишут. Просто они, видимо, ещё не пользовались статическими анализаторами кода. А вы пользуйтесь, это полезно!

Автор: lany

Источник