В седьмой версии статического анализатора PVS-Studio мы добавили поддержку языка Java. Пришло время немного рассказать, как мы начинали делать поддержку языка Java, что у нас получилось и какие дальнейшие планы. И, конечно, в статье будут приведены первые испытания анализатора на открытых проектах.
Для Java разработчиков, которые ранее не слышали об инструменте PVS-Studio, приведу совсем краткое его описание.
PVS-Studio - это инструмент для выявления ошибок и потенциальных уязвимостей в исходном коде программ, написанных на языках С, C++, C# и Java. Работает в среде Windows, Linux и macOS.
PVS-Studio выполняет статический анализ кода и генерирует отчёт, помогающий программисту находить и устранять дефекты. Тем, кто интересуется, как именно PVS-Studio ищет ошибки, предлагаю ознакомиться со статьёй "Технологии, используемые в анализаторе кода PVS-Studio для поиска ошибок и потенциальных уязвимостей".
Я бы мог придумать умную историю, как мы два года размышляли о том, какой следующий язык поддержать в PVS-Studio. О том, что Java - это разумный выбор, основанный на высокой популярности этого языка и так далее.
Однако как это бывает в жизни, всё решил не глубокий анализ, а эксперимент :). Да, мы размышляли, в какую сторону дальше развивать анализатор PVS-Studio. Рассматривались такие языки программирования, как: Java, PHP, Python, JavaScript, IBM RPG. Причём мы склонялись именно к языку Java, но окончательный выбор так ещё и не был сделан. Тех, у кого взгляд застрял на незнакомом IBM RPG, отсылаю к вот этой заметке, из которой всё станет ясно.
В конце 2017 года коллега Егор Бредихин посмотрел, какие есть готовые библиотеки разбора кода (проще говоря - парсеры) под интересные нам новые направления. И наткнулся на несколько проектов для разбора Java кода. На основе Spoon, ему довольно быстро удалось сделать прототип анализатора с парой диагностик. Более того, стало понятно, что мы сможем использовать в Java анализаторе некоторые механизмы C++ анализатора с помощью SWIG. Мы посмотрели на то, что получилось, и поняли, что наш следующий анализатор будет для Java.
Спасибо Егору за его начинание и активную работу, проделанную им над Java анализатором. Как именно шла разработка он описал в статье "Разработка нового статического анализатора: PVS-Studio Java".
В мире существует множество бесплатных и коммерческих статических анализаторов кода для Java. Перечислять их все в статье не имеет смысла, и я просто оставлю ссылку на "List of tools for static code analysis" (смотрите раздел Java и Multi-language).
Однако я знаю, что в первую очередь нас спросят про IntelliJ IDEA, FindBugs и SonarQube (SonarJava).
IntelliJ IDEA
В IntelliJ IDEA встроен очень мощный статический анализатор кода. Причем анализатор развивается, а его авторы внимательно следят за нашей деятельностью. С IntelliJ IDEA нам будет сложнее всего. Превзойти IntelliJ IDEA в диагностических возможностях мы не сможем, по крайней мере, сейчас. Поэтому мы постараемся сконцентрироваться на других наших преимуществах.
Статический анализ в IntelliJ IDEA – это, в первую очередь, одна из фишек среды разработки, что накладывает на него определённые ограничения. Мы же свободны в том, что можем делать с нашим анализатором. Например, мы можем быстро адаптировать анализатор под конкретные нужды заказчика. Быстрая и глубокая поддержка - наше конкурентное преимущество. Наши клиенты напрямую общаются непосредственно с программистами, разрабатывающими ту или иную часть PVS-Studio.
В PVS-Studio много возможностей по интеграции его в цикл разработки больших старых проектов. Это интеграция с SonarQube. Это массовое подавление сообщений анализатора, что позволяет сразу начать использовать анализатор в большом проекте для отслеживания ошибок только в новом или изменённом коде. PVS-Studio встраивается в процесс непрерывной интеграции. Думаю, именно эти и другие возможности помогут нашему анализатору найти место под солнцем в Java мире.
FindBugs
Проект FindBugs заброшен. Но его следует вспомнить по той причине, что это, пожалуй, наиболее известный бесплатный статический анализатор Java кода.
Преемником FindBugs можно назвать проект SpotBugs. Однако он менее популярен, и что с ним будет, тоже пока не совсем понятно.
В общем, мы считаем, что, хотя FindBugs был и остаётся крайне популярным, да к тому же ещё и бесплатным анализатором, думать про него нам не следует. Этот проект просто тихо и спокойно уйдёт в прошлое.
P.S. Кстати, теперь PVS-Studio также можно использовать бесплатно при работе с открытыми проектами.
SonarQube (SonarJava)
Мы считаем, что не конкурируем с SonarQube, а дополняем его. PVS-Studio интегрируется с SonarQube, что позволяет разработчикам находить большее количество ошибок и потенциальных уязвимостей в своих проектах. Как интегрировать в SonarQube инструмент PVS-Studio и другие анализаторы, мы регулярно рассказываем на мастер-классах, которые проводим в рамках различных конференций (пример).
Мы сделали доступными для пользователей самые популярные способы интеграции анализатора в сборочную систему:
На этапе тестирования мы встретили много пользователей, имеющих самописные сборочные системы, особенно в мобильной разработке. Им понравилась возможность запускать анализатор напрямую, перечислив исходники и classpath.
Подробную информацию о всех способах запуска анализатора вы можете найти на странице документации "Как запустить PVS-Studio Java".
Мы не могли обойти стороной платформу контроля качества кода SonarQube, популярную среди Java разработчиков, поэтому добавили поддержку языка Java в наш плагин для SonarQube.
У нас есть много идей, требующих дополнительного изучения, но некоторые планы, характерные для любого из наших анализаторов, выглядят так:
Возможно, мы найдём время адаптировать плагин IntelliJ IDEA для CLion. Привет C++ разработчикам, читающим про анализатор Java :-)
Я буду не я, если не покажу в статье какие-то ошибки, найденные с помощью нового анализатора. Можно было бы взять какой-то большой открытый Java-проект и написать классическую статью с разбором ошибок, как мы обычно и делаем.
Однако я сразу предвижу вопросы, а сможем ли мы найти что-то в таких проектах как IntelliJ IDEA, FindBugs и так далее. Поэтому у меня просто нет выхода, и я начну именно с этих проектов. Итак, я решил бегло проверить и выписать несколько интересных примеров ошибок из следующих проектов:
Написать про баги в этих проектах является сложной задачей. Дело в том, что эти проекты очень качественные. Собственно, это не удивительно. Как показывают наши наблюдения, код статических анализаторов всегда хорошо протестирован и проверен с помощью других инструментов.
Несмотря на всё это, мне придётся начать именно с этих проектов. Второго шанса написать что-то про них у меня не будет. Я уверен, что после выхода релиза PVS-Studio для Java, разработчики перечисленных проектов возьмут PVS-Studio на вооружение и начнут его использовать для регулярных или, по крайней мере, для периодических проверок своего кода. Например, я знаю, что Тагир Валеев, один из разработчиков JetBrains, занимающийся статическим анализатором кода IntelliJ IDEA, в тот момент, пока я пишу статью, уже во всю играется с Beta-версией PVS-Studio. Он написал нам уже около 15 писем с баг-репортами и рекомендациями. Спасибо, Тагир!
К счастью, мне не требуется найти как можно больше ошибок в одном конкретном проекте. Сейчас моя задача показать, что анализатор PVS-Studio для Java появился не зря и сможет пополнить линейку других инструментов, предназначенных для улучшения качества кода. Я просто бегло просмотрел отчёты анализатора и выписал несколько ошибок, которые показались мне интересными. По возможности я старался выписывать ошибки разного типа. Давайте посмотрим, что получилось.
private static boolean checkSentenceCapitalization(@NotNull String value) {
List<String> words = StringUtil.split(value, " ");
....
int capitalized = 1;
....
return capitalized / words.size() < 0.2; // allow reasonable amount of
// capitalized words
}Предупреждение PVS-Studio: V6011 [CWE-682] The '0.2' literal of the 'double' type is compared to a value of the 'int' type. TitleCapitalizationInspection.java 169
По задумке, функция должна возвращать истину, если менее 20% слов начинаются с заглавной буквы. На самом деле, проверка не работает, так как происходит целочисленное деление. В результате деления можно получить только два значения: 0 или 1.
Функция вернёт ложное значение, только если все слова будут начинаться с заглавной буквы. Во всех остальных случаях при делении будет получаться 0, и функция будет возвращать истинное значение.
public int findPreviousIndex(int current) {
int count = myPainter.getErrorStripeCount();
int foundIndex = -1;
int foundLayer = 0;
if (0 <= current && current < count) {
current--;
for (int index = count - 1; index >= 0; index++) { // <=
int layer = getLayer(index);
if (layer > foundLayer) {
foundIndex = index;
foundLayer = layer;
}
}
....
}Предупреждение PVS-Studio: V6007 [CWE-571] Expression 'index >= 0' is always true. Updater.java 184
Вначале посмотрите на условие (0 <= current && current < count). Оно выполняется только в том случае, если значение переменной count больше 0.
Теперь посмотрим на цикл:
for (int index = count - 1; index >= 0; index++)Переменная index инициализируется выражением count - 1. Так как переменная count больше 0, то начальное значение переменной index всегда больше или равно 0. Получается, что цикл будет выполняться до тех пор, пока не произойдёт переполнение переменной index.
Скорее всего, это просто опечатка и должен выполняться не инкремент, а декремент переменной:
for (int index = count - 1; index >= 0; index--)@NonNls public static final String BEFORE_STR_OLD = "before:";
@NonNls public static final String AFTER_STR_OLD = "after:";
private static boolean isBeforeOrAfterKeyword(String str, boolean trimKeyword) {
return (trimKeyword ? LoadingOrder.BEFORE_STR.trim() :
LoadingOrder.BEFORE_STR).equalsIgnoreCase(str) ||
(trimKeyword ? LoadingOrder.AFTER_STR.trim() :
LoadingOrder.AFTER_STR).equalsIgnoreCase(str) ||
LoadingOrder.BEFORE_STR_OLD.equalsIgnoreCase(str) || // <=
LoadingOrder.BEFORE_STR_OLD.equalsIgnoreCase(str); // <=
}Предупреждение PVS-Studio: V6001 [CWE-570] There are identical sub-expressions 'LoadingOrder.BEFORE_STR_OLD.equalsIgnoreCase(str)' to the left and to the right of the '||' operator. Check lines: 127, 128. ExtensionOrderConverter.java 127
Старый добрый эффект последней строки. Программист поторопился и, размножив строчку кода, забыл её исправить. В результате, дважды строка str сравнивается с BEFORE_STR_OLD. Скорее всего, одно из сравнений должно быть с AFTER_STR_OLD.
public synchronized boolean isIdentifier(@NotNull String name,
final Project project) {
if (!StringUtil.startsWithChar(name,'\'') &&
!StringUtil.startsWithChar(name,'\"')) {
name = "\"" + name;
}
if (!StringUtil.endsWithChar(name,'"') &&
!StringUtil.endsWithChar(name,'\"')) {
name += "\"";
}
....
}Предупреждение PVS-Studio: V6001 [CWE-571] There are identical sub-expressions '!StringUtil.endsWithChar(name,'"')' to the left and to the right of the '&&' operator. JsonNamesValidator.java 27
Данный фрагмент кода проверяет, что имя взято в одинарные или двойные кавычки. Если это не так, то двойные кавычки добавляются автоматически.
Из-за опечатки, окончание имени проверяется только на наличие двойных кавычек. В результате имя, взятое в одинарные кавычки, будет обработано некорректно.
Имя
'Abcd'из-за добавления лишних двойных кавычек превратится в:
'Abcd'"static Context parse(....) {
....
for (int i = offset; i < endOffset; i++) {
char c = text.charAt(i);
if (c == '<' && i < endOffset && text.charAt(i + 1) == '/'
&& startTag != null
&& CharArrayUtil.regionMatches(text, i + 2, endOffset, startTag))
{
endTagStartOffset = i;
break;
}
}
....
}Предупреждение PVS-Studio: V6007 [CWE-571] Expression 'i < endOffset' is always true. EnterAfterJavadocTagHandler.java 183
Подвыражение i < endOffset в условии оператора if не имеет смысла. Переменная i и так всегда меньше endOffset, что следует из условия выполнения цикла.
Скорее всего, программист хотел защититься от выхода за границу строки при вызове функций:
В этом случае подвыражение для проверки индекса должно быть таким: i < endOffset - 2.
public static String generateWarningMessage(....)
{
....
if (buffer.length() > 0) {
if (buffer.length() > 0) {
buffer.append(" ").append(
IdeBundle.message("prompt.delete.and")).append(" ");
}
}
....
}Предупреждение PVS-Studio: V6007 [CWE-571] Expression 'buffer.length() > 0' is always true. DeleteUtil.java 62
Это может быть как безобидным избыточным кодом, так и серьезной ошибкой.
Если проверка-дубликат появилась случайно, например, в ходе рефакторинга, то ничего плохого в этом нет. Вторую проверку можно просто удалить.
Но возможен и другой сценарий. Вторая проверка должна быть совсем другой, и код ведёт себя не так, как задумано. Тогда это настоящая ошибка.
Примечание. Кстати, разных избыточных проверок находится очень много. Причём часто видно, что это не ошибка. Однако и назвать сообщения анализатора ложными срабатываниями тоже нельзя. Для пояснения приведу вот такой пример, также взятый из IntelliJ IDEA:
private static boolean isMultiline(PsiElement element) {
String text = element.getText();
return text.contains("\n") || text.contains("\r") || text.contains("\r\n");
}Анализатор говорит, что функция text.contains("\r\n") всегда возвращает ложь. И действительно, если не найден символ "\n" и "\r", то и нет смысла искать "\r\n". Это не ошибка, и код плох только тем, что работает чуть медленнее, выполняя бессмысленный поиск подстроки.
Как быть с подобным кодом, в каждом конкретном случае уже решать программистам. Я при написании статей, как правило, просто не обращаю внимания на подобный код.
public boolean satisfiedBy(@NotNull PsiElement element) {
....
@NonNls final String text = expression.getText().replaceAll("_", "");
if (text == null || text.length() < 2) {
return false;
}
if ("0".equals(text) || "0L".equals(text) || "0l".equals(text)) {
return false;
}
return text.charAt(0) == '0';
}Предупреждение PVS-Studio: V6007 [CWE-570] Expression '"0".equals(text)' is always false. ConvertIntegerToDecimalPredicate.java 46
Этот код точно содержит логическую ошибку. Но я затрудняюсь сказать, что именно хотел проверить программист, и как следует исправлять недочёт. Поэтому здесь я только укажу на бессмысленную проверку.
В начале проверятся, что строка должна содержать не менее двух символов. Если это не так, то функция возвращает false.
Далее следует проверка "0".equals(text). Она бессмысленна, так как строка не может содержать только один символ.
В общем, что-то здесь не так и код следует поправить.
public static String getXMLType(@WillNotClose InputStream in) throws IOException
{
....
String s;
int count = 0;
while (count < 4) {
s = r.readLine();
if (s == null) {
break;
}
Matcher m = tag.matcher(s);
if (m.find()) {
return m.group(1);
}
}
throw new IOException("Didn't find xml tag");
....
}Предупреждение PVS-Studio: V6007 [CWE-571] Expression 'count < 4' is always true. Util.java 394
По задумке, поиск xml-тега должен осуществляться только в первых четырёх строчках файла. Но из-за того, что забыли инкрементировать переменную count, будет прочитан весь файл.
Во-первых, это может оказаться очень медленной операцией, а во-вторых, где-то в середине файла может быть найдено нечто, что будет воспринято как xml-тег, но при этом таковым являться не будет.
private void reportBug() {
int priority = LOW_PRIORITY;
String pattern = "NS_NON_SHORT_CIRCUIT";
if (sawDangerOld) {
if (sawNullTestVeryOld) {
priority = HIGH_PRIORITY; // <=
}
if (sawMethodCallOld || sawNumericTestVeryOld && sawArrayDangerOld) {
priority = HIGH_PRIORITY; // <=
pattern = "NS_DANGEROUS_NON_SHORT_CIRCUIT";
} else {
priority = NORMAL_PRIORITY; // <=
}
}
bugAccumulator.accumulateBug(
new BugInstance(this, pattern, priority).addClassAndMethod(this), this);
}Предупреждение PVS-Studio: V6021 [CWE-563] The value is assigned to the 'priority' variable but is not used. FindNonShortCircuit.java 197
Значение переменной priority выставляется в зависимости от значения переменной sawNullTestVeryOld. Однако это не играет никакой роли. Далее переменной priority в любом случае будет присвоено другое значение. Явная ошибка в логике работы функции.
public class RuleDto {
....
private final RuleDefinitionDto definition;
private final RuleMetadataDto metadata;
....
private void setUpdatedAtFromDefinition(@Nullable Long updatedAt) {
if (updatedAt != null && updatedAt > definition.getUpdatedAt()) {
setUpdatedAt(updatedAt);
}
}
private void setUpdatedAtFromMetadata(@Nullable Long updatedAt) {
if (updatedAt != null && updatedAt > definition.getUpdatedAt()) {
setUpdatedAt(updatedAt);
}
}
....
}PVS-Studio: V6032 It is odd that the body of method 'setUpdatedAtFromDefinition' is fully equivalent to the body of another method 'setUpdatedAtFromMetadata'. Check lines: 396, 405. RuleDto.java 396
В методе setUpdatedAtFromMetadata используется поле definition. Скорее всего, должно использоваться поле metadata. Это очень похоже на последствия неудачного Copy-Paste.
private final Map<JavaPunctuator, Tree.Kind> assignmentOperators =
Maps.newEnumMap(JavaPunctuator.class);
public KindMaps() {
....
assignmentOperators.put(JavaPunctuator.PLUSEQU, Tree.Kind.PLUS_ASSIGNMENT);
....
assignmentOperators.put(JavaPunctuator.PLUSEQU, Tree.Kind.PLUS_ASSIGNMENT);
....
}Предупреждение PVS-Studio: V6033 [CWE-462] An item with the same key 'JavaPunctuator.PLUSEQU' has already been added. Check lines: 104, 100. KindMaps.java 104
Дважды в карту помещается одна и та же пара ключ-значение. Скорее всего, это получилось по невнимательности, и на самом деле никакой настоящей ошибки нет. Однако этот код в любом случает требуется проверить, так как, возможно, забыли добавить какую-то другую пару.
Да какое тут может быть заключение?! Приглашаю всех, не откладывая, скачать PVS-Studio и попробовать проверить свои рабочие проекты на языке Java! Скачать PVS-Studio.
Спасибо всем за внимание. Надеюсь, скоро мы порадуем читателей циклом статей, посвящённых проверке различных открытых Java-проектов.