Топ 10 ошибок в проектах Java за 2020 год

Новый год неумолимо приближается — а, значит, настало время подводить итоги. Продолжая традицию, мы прошлись по нашим статьям о проверках Java-проектов из мира open-source за этот год и составили рейтинг 10 самых интересных ошибок.

За уходящий год мы (Java-команда PVS-Studio) разобрали в наших статьях ошибки из пяти open-source проектов и совсем немного рассказали про нашу внутреннюю кухню:

• АНБ, Ghidra и единороги
• Под капотом PVS-Studio для Java: разработка диагностик
• Проверка кода XMage и почему недоступны специальные редкие карточки для коллекции Dragon's Maze
• Проверка WildFly — сервера JavaEE приложений
• Единороги на страже вашей безопасности: исследуем код Bouncy Castle
• Big/Bug Data: анализируем исходный код Apache Flink

Предлагаем читателю сначала ознакомиться с этими статьями и составить свой личный рейтинг, чтобы потом сравнить его с нашим и сказать, что мы неправы :).

Десятое место: "Обманчивое равенство"

Источник: Big/Bug Data: анализируем исходный код Apache Flink

V6001 There are identical sub-expressions 'processedData' to the left and to the right of the '==' operator. CheckpointStatistics.java(229)

@Override
public boolean equals(Object o) 
{
  ....
  CheckpointStatistics that = (CheckpointStatistics) o;
  return id == that.id &&
    savepoint == that.savepoint &&
    triggerTimestamp == that.triggerTimestamp &&
    latestAckTimestamp == that.latestAckTimestamp &&
    stateSize == that.stateSize &&
    duration == that.duration &&
    alignmentBuffered == that.alignmentBuffered &&
    processedData == processedData &&                // <=
    persistedData == that.persistedData &&
    numSubtasks == that.numSubtasks &&
    numAckSubtasks == that.numAckSubtasks &&
    status == that.status &&
    Objects.equals(checkpointType, that.checkpointType) &&
    Objects.equals(
      checkpointStatisticsPerTask, 
      that.checkpointStatisticsPerTask);
}

Простая и очень обидная ошибка из-за невнимательности: поле processedData сравнивается с самим собой. Из-за этой ошибки сравнение объектов типа CheckpointStatistics иногда будет выдавать ложноположительный результат. Но основная опасность этой опечатки состоит в том, что equals крайне активно используется в коллекциях, и некорректная реализация этого метода может привести к очень странному поведению, на отладку которого уйдёт огромное количество времени.

Хочу заметить, что ошибаться в функциях сравнения для разработчиков привычное дело. Мой коллега даже написал большую статью "Зло живет в функциях сравнения" с множеством примеров и объяснений.

Девятое место: "Недостижимый код"

Источник: Единороги на страже вашей безопасности: исследуем код Bouncy Castle.

V6019 Unreachable code detected. It is possible that an error is present. XMSSTest.java(170)

public void testSignSHA256CompleteEvenHeight2() {
    ....
    int height = 10;
    ....
    for (int i = 0; i < (1 << height); i++) {
        byte[] signature = xmss.sign(new byte[1024]);
        switch (i) {
            case 0x005b:
                assertEquals(signatures[0], Hex.toHexString(signature));
                break;
            case 0x0822:
                assertEquals(signatures[1], Hex.toHexString(signature));
                break;
            ....
        }
    }
}

Ветвь switch для значения i == 0x0822(2082) оказалась недостижимой. Как же так получилось?

Если обратить внимание на условие цикла 1 << height, где height всегда равен 10, то всё сразу встанет на свои места. Согласно условию цикла, счётчик i в цикле for не может быть больше, чем 1024 (1 << 10). Естественно, выполнение рассматриваемой ветви switch никогда не произойдет.

Восьмое место: "Проаннотированный метод"

Источник: Под капотом PVS-Studio для Java: разработка диагностик.

V6009 Collection is empty. The call of the 'clear' function is senseless. MetricRepositoryRule.java(90)

protected void after()
{
  this.metricsById.clear();
  this.metricsById.clear();
}

Часть наших диагностик сильно полагается на механизм аннотирования методов. Аннотации предоставляют дополнительную информацию анализатору об используемых методах, например:

• Чистый ли это метод,
• Какие накладываются ограничения на аргументы,
• Возвращаемый результат,
• … и всякое прочее.

Некоторые аннотации анализатор выводит сам из исходного кода, некоторые мы проставляем вручную (например, для методов стандартной библиотеки). История этой ошибки началась с того, что мы не в полной мере проаннотировали метод Map#clear. После того, как мы это заметили и исправили, на наших тестовых проектах повылезали новые срабатывания, среди которых был и наш интересный случай.

На первый взгляд, повторная очистка словаря – не ошибка. И мы бы даже решили, что это случайно продублированная строка, если бы не обратили внимание на поля класса:

private final Map<String, Metric> metricsByKey = new HashMap<>();
private final Map<Long, Metric> metricsById = new HashMap<>();

У класса есть два поля с похожими именами metricsById и metricsByKey. Это и наталкивает на мысль, что автор кода хотел очистить оба словаря, но… этого не произошло. Таким образом, два словаря, которые хранят связанные данные, будут рассинхронизированы после вызова after.

Седьмое место: "Ожидание / реальность"

Источник: Проверка WildFly — сервера JavaEE приложений.

V6058 The 'equals' function compares objects of incompatible types: String, ModelNode. JaxrsIntegrationProcessor.java(563)

// Send value to RESTEasy only if it's not null, empty string, or the 
// default value.
private boolean isTransmittable(AttributeDefinition attribute,
                                ModelNode modelNode) {
  if (modelNode == null || ModelType
      .UNDEFINED.equals(modelNode.getType())) {
    return false;
  }
  String value = modelNode.asString();
  if ("".equals(value.trim())) {
    return false;
  }
  return !value.equals(attribute.getDefaultValue());        // <=
}

Обратив внимание на предшествующий методу комментарий, можно ожидать, что метод вернет true, если:

• modelNode не null,
• строковое представление modelNode не пустое,
• modelNode – не значение по умолчанию.

Несмотря на авторский комментарий и корректную, на первый взгляд, логику, поведение метода будет отличаться. Причиной этому служит проверка modelNode на равенство со значением по умолчанию в последней строчке метода.

Строковое представление modelNode сравнивается с объектом типа ModelNode, и, как можно догадаться, такое сравнение всегда будет возвращать отрицательный результат из-за несовместимости типов.

Последствия ошибки: непредвиденное разрешение к отправке значения modelNode, когда оно равно значению по умолчанию (attribute.getDefaultValue()).

Шестое место: "Копипаст-ориентированное программирование"

Источник: Проверка кода XMage и почему недоступны специальные редкие карточки для коллекции Dragon's Maze.

V6072 Two similar code fragments were found. Perhaps, this is a typo and 'playerB' variable should be used instead of 'playerA'. SubTypeChangingEffectsTest.java(162), SubTypeChangingEffectsTest.java(158), SubTypeChangingEffectsTest.java(156), SubTypeChangingEffectsTest.java(160)

@Test
public void testArcaneAdaptationGiveType() {
    addCard(Zone.HAND, playerA, "Arcane Adaptation", 1);
    addCard(Zone.BATTLEFIELD, playerA, "Island", 3);

    addCard(Zone.HAND, playerA, "Silvercoat Lion");
    addCard(Zone.BATTLEFIELD, playerA, "Silvercoat Lion");
    addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion");   // <=

    addCard(Zone.HAND, playerB, "Silvercoat Lion");
    addCard(Zone.BATTLEFIELD, playerB, "Silvercoat Lion");
    addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion");   // <=

    ....

    for (Card card : playerB.getGraveyard().getCards(currentGame)) {
        if (card.isCreature()) {
            Assert.assertEquals(card.getName() + " should not have ORC type",
                    false, card.getSubtype(currentGame).contains(SubType.ORC));
            Assert.assertEquals(card.getName() + " should have CAT type",
                    true, card.getSubtype(currentGame).contains(SubType.CAT));
        }
    }
}

В этом году, как и в прошлом (Топ 10 ошибок за 2019), классная copy-paste ошибка от диагностического правила V6072 заслуживает место в десятке.

Характер ошибки заключается в том, что когда разработчику нужно сделать схожие действия для различных переменных, то он копирует добросовестно написанный ранее код и изменяет название переменной. Но делает это уже не совсем добросовестно и забывает доизменить переменные.

В данном фрагменте кода так и произошло. Автор теста имитировал игру между игроками, раскидывая между ними одинаковые карты по игровым зонам, но из-за copy-paste игроку playerA дважды досталась одна и та же карта. Из-за этого игровая зона Zone.GRAVEYARD игрока playerB осталась без тестирования. Подробное описание ошибки можно почитать в самой статье.

Пятое место: "Ненормальное распределение"

Источник: Big/Bug Data: анализируем исходный код Apache Flink

V6048 This expression can be simplified. Operand 'index' in the operation equals 0. CollectionUtil.java(76)

public static <T> 
Collection<List<T>> partition(Collection<T> elements, int numBuckets) 
{
  Map<Integer, List<T>> buckets = new HashMap<>(numBuckets);
  
  int initialCapacity = elements.size() / numBuckets;

  int index = 0;
  for (T element : elements) 
  {
    int bucket = index % numBuckets;                                 // <=
    buckets.computeIfAbsent(bucket, 
                            key -> new ArrayList<>(initialCapacity))

           .add(element); 
  }

  return buckets.values();
}

Ошибка была обнаружена в утилитном методе partition, который разбивает переданную коллекцию elements на numBuckets коллекций. Суть ошибки в том, что индекс коллекции bucket, в которую хотят поместить каждый рассматриваемый элемент, имеет константное значение (0). Причиной этому служит то, что разработчик забыл инкрементировать переменную index на каждой итерации цикла.

Вследствие чего метод partition будет всегда возвращать коллекцию elements, обернутую в другую коллекцию. А это – вряд ли задуманное поведение.

Четвертое место: "Бомба замедленного действия"

Источник: АНБ, Ghidra и единороги.

V6008 Null dereference of 'selectedNode' in function 'setViewPanel'. OptionsPanel.java(266)

private void processSelection(OptionsTreeNode selectedNode) {
  if (selectedNode == null) {
    setViewPanel(defaultPanel, selectedNode); // <=
    return;
  }
  ....
}
private void setViewPanel(JComponent component, OptionsTreeNode selectedNode) {
  ....
  setHelpLocation(component, selectedNode);
  ....
}
private void setHelpLocation(JComponent component, OptionsTreeNode node) {
  Options options = node.getOptions();
  ....
}

В приведенном фрагменте кода явно напортачили. Если вы проследите за selectedNode из processSelection(), когда selectedNode == null, то сразу же обнаружите, что при таком исходе нас ждет неминуемый NullPointerException. О чем и предупреждает нас анализатор.

Но, изучив немного код, автор статьи пришел к выводу, что выполнение программы никогда не встретится с NullPointerException, так как processSelection() вызывается всего в двух местах, перед вызовом которых selectedNode явно проверяется на null.

Несмотря на это, такой код – бомба замедленного действия, поскольку другой разработчик может увидеть, что метод явно обрабатывает случай selectedNode == null, и решить, что это валидное значение, что потом выльется в падение приложения.

Третье место: "Всегда false"

Источник: Проверка кода XMage и почему недоступны специальные редкие карточки для коллекции Dragon's Maze.

V6007 Expression 'filter.getMessage().toLowerCase(Locale.ENGLISH).startsWith("Each ")' is always false. SetPowerToughnessAllEffect.java(107)

@Override
public String getText(Mode mode) {
  StringBuilder sb = new StringBuilder();
  ....
  if (filter.getMessage().toLowerCase(Locale.ENGLISH).startsWith("Each ")) {
    sb.append(" has base power and toughness ");
  } else {
    sb.append(" have base power and toughness ");
  }
  ....
  return sb.toString();
}

Кто же сравнивает строку, приведенную в нижний регистр, со строкой, которая начинается с заглавной буквы? Отсюда и всегда ложный результат проверки сообщения.

Результат дефекта не критический, но тоже неприятный: где-то будет фигурировать неграмотно составленный текст.

Второе место: "2-в-1"

Источник: АНБ, Ghidra и единороги.

V6007 Expression 'index >= 0' is always true. ExternalNamesTableModel.java(105)

V6019 Unreachable code detected. It is possible that an error is present. ExternalNamesTableModel.java(109)

public void setValueAt(Object aValue, int row, int column) {
  ....
  int index = indexOf(newName);
  if (index >= 0) {                  // <=
    Window window = tool.getActiveWindow();
    Msg.showInfo(getClass(), window, "Duplicate Name",
                 "Name already exists: " + newName);
    return;
  }

  ExternalPath path = paths.get(row); // <=
  ....
}

private int indexOf(String name) {
  for (int i = 0; i < paths.size(); i++) {
    ExternalPath path = paths.get(i);
    if (path.getName().equals(name)) {
      return i;
    }
  }
  return 0;
}

Метод indexOf всегда возвращает неотрицательное число. А всё из-за того, что автор метода в случае отсутствия искомого newName по ошибке возвращает 0, а не -1. Такая ошибка приводит к тому, что поток выполнения программы всегда будет заходить в then-ветку условного оператора if (index >= 0), в котором будет выдавать сообщение о существующем newName и успешно выходить из метода, даже тогда, когда в реальности newName не был найден.

Но и это ещё не всё. Так как then-ветка условного оператора прекращает выполнение метода, то до кода после условного оператора дело так и не дойдет.

Об этом и предупреждает нас анализатор.

Первое место: "А то ли мы проверили?"

Источник: Под капотом PVS-Studio для Java: разработка диагностик.

V6080 Consider checking for misprints. It's possible that an assigned variable should be checked in the next condition. Menu.java(40)

public class Menu
{
  private Map<String, List<String>> menus = new HashMap<String, List<String>>();

  public void putMenuItem(String menu, String item)
  {
    List<String> items = menus.get(menu);
    if (item == null)                      // <=
    {
      items = new ArrayList<String>();
      menus.put(menu, items);
    }
    items.add(item);
  }
  ....
}

По задумке автора предполагалось создать коллекцию по ключу menu, если таковой ещё не было. Но проверка не той переменной разрушила всю задумку, прорубив лазеечку для NullPointerException. Метод выбросит исключение, когда в словаре ключ menu будет отсутствовать, и значение item, которое хотели добавить, не будет null.

Заключение

Проверки open-source проектов с помощью PVS-Studio из года в год доказывают, что такой рубеж защиты, как статический анализ кода, должен обязательно присутствовать в разработке. Каким бы вы мастером своего дела ни были, ошибки обязательно найдут лазеечку в ваш проект, и причин этому множество: устали, завал на работе или вовсе отвлеклись на котиков. А если вы работаете в команде, то количество возможностей попасть ошибкам в код вырастает пропорционально количеству коллег.

Если вам понравился наш обзор, то не ждите следующего конца года. Статьи о проверках начнутся сразу же с первого месяца 2021, а если вам не терпится – смелее скачивайте анализатор и самостоятельно проверяйте open-source проекты.


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Maxim Stefanov. Top-10 Bugs in Java Projects in 2020.