10 самых интересных ошибок в Java проектах за 2024 год

• Предисловие
• 10 место. Назад в будущее
• 9 место. Почему-то не работает
• 8 место. Выстрел в ногу
• 7 место. Даже математики ошибаются
• 6 место. Этот паттерн не работает
• 5 место. Микрооптимизация
• 4 место. Тест не упадёт, если не будет работать
• 3 место. Happy debugging, folks
• 2 место. Когда паттерны подвели
• 1 место. Одна ошибка компенсировала другую
• Заключение

В 2024 году мы проверили много проектов, о чём регулярно писали в свой блог. Теперь же настала новогодняя пора, и самое время рассказывать новогодние истории. Вот и мы решили поведать о самых интересных Java ошибках, встреченных нами в Open Source проектах.

Предисловие

Традицию публикации самых интересных ошибок, найденных с помощью PVS-Studio, мы поддерживаем уже давно, но топов именно Java ошибок не было аж с 2020 года. В этот раз я постарался реанимировать практику. Надеюсь, вы достали мягкий плед и сделали себе тёплый чай, потому что я приготовил для вас 10 увлекательных ошибок. Критерии для попадания в топ простые:

• моё субъективное мнение;
• можно ли рассказать вокруг них интересную историю;
• разнообразие, достоверность и общая небанальность.

То есть можно рассчитывать на 10 занимательных историй, и не без поучительных уроков — Новый Год скоро всё-таки :)

10 место. Назад в будущее

На десятом месте нас встречает первый фрагмент кода.

public Builder setPersonalisation(Date date, ....) {
  ....
  final OutputStreamWriter
    out = new OutputStreamWriter(bout, "UTF-8");
  final DateFormat
    format = new SimpleDateFormat("YYYYMMdd");
  out.write(format.format(date));
    ....
}

И я просто не мог не включить его в предновогоднюю подборку, ведь баг в этом коде позволяет быстрее перенестись в следующий год :) Уже догадались, с чем он связан?

Обратите внимание на аргумент, переданный в конструктор SimpleDateFormat. Кажется ли он вам валидным? На самом деле, если передать туда почти любую дату, скажем, дату на момент написания этого текста (10.12.2024), то в результате выполнения кода вернётся вполне корректная величина — 20241210.

Вот только если на вход попадёт 29.12.2024, то вернётся уже 20251229. Таким нехитрым образом можно отправиться в новый год раньше времени. Путешествовать назад, кстати, тоже можно.

Это происходит из-за того, что символ Y в аргументе для SimpleDateFormat отвечает за год, основанный на номере недели. Если кратко, то неделя считается первой, когда на неё приходятся минимум четыре дня нового года. Таким образом, если ваша неделя начинается в воскресенье, то в новый год можно попасть аж на три дня раньше.

Для исправления достаточно заменить Y в верхнем регистре на нижний — y. Если хотите почитать подробнее, то на эту тему у нас была отдельная заметка.

Вот предупреждение PVS-Studio, которое указало на ошибку:

V6122 Usage of 'Y' (week year) pattern was detected: it was probably intended to use 'y' (year). SkeinParameters.java 246

Из-за специфики с номером недели подобную ошибку очень сложно найти при помощи тестирования. Почему же такой тематический баг оказался на последнем месте? Дело в том, что срабатывание было найдено не в актуальной версии Bouncy Castle, а в нашей тестовой базе. Там находятся старые исходники, и на текущий момент этот баг уже давно исправлен. Вот такой привет из прошлого — снова путешествие во времени :)

9 место. Почему-то не работает

На девятом месте у нас находится срабатывание в коде, взятое из проверки GeoServer:

@Test
public void testStore() {
  Properties newProps = dao.toProperties();
  
  // ....
  Assert.assertEquals(newProps.size(), props.size());
  for (Object key : newProps.keySet()) {
    Object newValue = newProps.get(key);
    Object oldValue = newProps.get(key);              // <=
    Assert.assertEquals(newValue, oldValue);
  }
}

Теперь приведу предупреждение анализатора:

V6027 Variables 'newValue', 'oldValue' are initialized through the call to the same function. It's probably an error or un-optimized code. DataAccessRuleDAOTest.java 110, DataAccessRuleDAOTest.java 111

Что интересного в такой ошибке? Позвольте вернуть то, что скрывалось за четырьмя точками:

@Test
public void testStore() {
  Properties newProps = dao.toProperties();

  // properties equality does not seem to work...
  Assert.assertEquals(newProps.size(), props.size());
  for (Object key : newProps.keySet()) {
    Object newValue = newProps.get(key);
    Object oldValue = newProps.get(key);
    Assert.assertEquals(newValue, oldValue);
  }
}

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

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

8 место. Выстрел в ногу

Если баги из проекта под названием JBullet нельзя называть выстрелами в ногу, то я не знаю, какие можно. Вот сама ошибка из статьи:

@Override
public BroadphasePair findPair(BroadphaseProxy proxy0, BroadphaseProxy proxy1) {
  BulletStats.gFindPairs++;
  if (proxy0.getUid() > proxy1.getUid()) {
    BroadphaseProxy tmp = proxy0;
    proxy0 = proxy1;
    proxy1 = proxy0;
  }
  ....
}

Думаю, вам даже не нужно предупреждение PVS-Studio, чтобы понять, где тут ошибка. Но в любом случае вот оно:

V6026 This value is already assigned to the 'proxy1' variable. HashedOverlappingPairCache.java 233

Да, до неприличного простая ошибка. Хотя такая простота делает её даже более забавной. Тем не менее у неё есть своя история.

Дело в том, что библиотека JBullet является портом с C/C++ библиотеки bullet, и там есть аналогичная функция:

template <typename T>
B3_FORCE_INLINE void b3Swap(T &a, T &b)
{
  T tmp = a;
  a = b;
  b = tmp;
}

Нетрудно заметить, что этот код написан вполне корректно. И, судя по git blame, был написан так изначально. Получается, что ошибка возникла из-за невнимательности при переписывании кода с одного языка на другой.

За сочетание поразительной незамысловатости с богатой историей я и наградил это срабатывание восьмым местом. А ещё, надеюсь, вы оценили, что баг с выстрелом в ногу оказался связан с C++.

7 место. Даже математики ошибаются

Надо признаться, следующее срабатывание греет мне сердце, причём по многим причинам. Вот код, взятый из нашей проверки GeoGebra:

@Override
final public void update() {
  ....
  switch (angle.getAngleStyle()) {
    ....
    case EuclidianStyleConstants.RIGHT_ANGLE_STYLE_L:
      // Belgian offset |_
      if (square == null) {
        square = AwtFactory.getPrototype().newGeneralPath();
      } else {
        square.reset();
      }
      length = arcSize * 0.7071067811865;
      double offset = length * 0.4;
      square.moveTo(
          coords[0] + length * Math.cos(angSt)
              + offset * Math.cos(angSt)
              + offset * Math.cos(angSt + Kernel.PI_HALF),
          coords[1]
              - length * Math.sin(angSt)
                  * view.getScaleRatio()
              - offset * Math.sin(angSt)
              - offset * Math.sin(angSt + Kernel.PI_HALF));
      ....
      break;
    }
}

Предлагаю попробовать вам найти ошибку самим. Чтобы вы не подсматривали, сообщение анализатора и объяснения спрятаны в спойлер.

6 место. Этот паттерн не работает

Следующее срабатывание, которое я взял из первой статьи по мотивам проверки DBeaver, не сразу может показаться примечательным. Вот код:

private List<DBTTaskRun> runs;
....
private void loadRunsIfNeeded() {
    if (runs == null) {
        synchronized (this) {
            if (runs == null) {
                runs = new ArrayList<>(loadRunStatistics());
            }
        }
    }
}

А вот, на что указал анализатор PVS-Studio:

V6082 Unsafe double-checked locking. The field should be declared as volatile. TaskImpl.java 59, TaskImpl.java317

При том, что конкретно в этом срабатывании нет ничего особенного, мне оно всё ещё кажется очень интересным. Дело в том, что применённый выше паттерн Double-Checked Locking не работает. В чём подвох? Это было актуально 20 лет назад :)

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

Паттерн Double-checked locking нужен для того, чтобы реализовать отложенную инициализацию в многопоточной среде. Суть в том, что перед "тяжёлой" проверкой в блоке синхронизации делается "лёгкая" без него, и только при прохождении обеих проверок создаётся ресурс.

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

А вот вам твист: этот паттерн не работал до JDK 5. А с пятой версии появилось ключевое слово volatile, которое решает потенциальную проблему переупорядочивания операций благодаря принципу happens-before. И именно о необходимости добавить это ключевое слово и сигнализирует анализатор.

Впрочем, этот паттерн использовать всё равно не стоит. Производительность железа и JVM с тех пор далеко шагнула вперёд, и synchronized операции больше не такие медленные. При этом в реализации DCL-паттерна легко ошибиться, что чревато вышеописанными последствиями. Это подтверждает и тот факт, что мы до сих пор находим в проектах с долгой историей такие ошибки, допущенные по невнимательности.

5 место. Микрооптимизация

Пятёрку самых интересных ошибок открывает очередное срабатывание из DBeaver, про которое мы писали отдельную статью. Предлагаю вам на него посмотреть:

@Override
public boolean canEditObject(ExasolTableColumn object) {
    ExasolTableBase exasolTableBase = object.getParentObject();
    if (exasolTableBase != null &
        exasolTableBase.getClass().equals(ExasolTable.class)) {
        return true;
    } else {
        return false;
    }
}

И сразу посмотреть на пояснение анализатора:

V6030 The method located to the right of the '&' operator will be called regardless of the value of the left operand. Perhaps, it is better to use '&&'. ExasolTableColumnManager.java 79, DB2TableColumnManager.java 77

Ошибка следующая: разработчик перепутал логическое && с битовым &. Они отличаются тем, что после битового "И" не прекращается выполнение условий в выражении. Это происходит из-за того, что для битового "И" не работает короткая схема вычисления. Из-за её отсутствия, даже несмотря на то, что exasolTableBase != null вернёт false, поток выполнения дойдёт до exasolTableBase.getClass(), и мы получим NPE.

Окей, всего лишь опечатка, расходимся? Как бы ни так. В DBeaver таких срабатываний было много. Очень. И большая часть из них была вполне безобидна. Если интересно, оставлю под катом несколько таких.

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

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

К нашему удивлению, кустарный бенчмарк это подтвердил:

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

И к чему я поднял эту тему? К тому, какую цену несут в себе потенциальные микрооптимизации.

Во-первых, предсказание ветвлений придумали не просто так, и бездумно от него отказываться может быть ещё более накладным. Так, наш бенчмарк, скорее всего, отработал быстрее потому, что там у значений было нормальное распределение, что вряд ли будет наблюдаться в реальных случаях.

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

В-третьих, мы открываем пространство вот для таких ошибок, с которой и начался этот раздел.

В общем, поучительная история о цене микрооптимизаций мне показалась достаточно интересной, чтобы открыть пятёрку нашего топа.

4 место. Тест не упадёт, если не будет работать

Часто панацеей от ошибок самого разного толка считаются автоматические тесты. И каждый раз хочется спросить, кто будет тестировать сами тесты. Ещё одно срабатывание из проверки GeoServer это в очередной раз это демонстрирует. Вот этот код:

@Test
public void testChallenge() throws Exception {
  Map<String, Object> raw = getWorld();
  try {
    executeGetCoverageKvp(raw);
    fail("This should have failed with a security exception");
  } catch (Throwable e) {
    // make sure we are dealing with some security exception
    Throwable se = null;
    while (e.getCause() != null && e.getCause() != e) {         // <=
      e = e.getCause();
      if (SecurityUtils.isSecurityException(e)) {
        se = e;
      }
    }
  
    if (e == null) {                                            // <=
      fail("We should have got some sort of SpringSecurityException");
    } else {
      // some mumbling about not having enough privileges
      assertTrue(se.getMessage().contains("World"));
      assertTrue(se.getMessage().contains("privileges"));
    }
  }
}

А вот и срабатывание анализатора PVS-Studio:

V6060 The 'e' reference was utilized before it was verified against null. ResourceAccessManagerWCSTest.java 186, ResourceAccessManagerWCSTest.java 193

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

Вначале может показаться, что логика теста сама по себе неправильная, ведь переменная e получена из оператора catch и далее не меняется, из-за чего никогда не будет null. Так, можно шальной правкой просто удалить then-ветку условия if(e == nul) как недостижимую. Но это будет в корне неправильно. Уже поняли, в чём подвох?

В коде есть ещё одна переменная, содержащая в себе объект исключения — se. И вот у неё значение меняется внутри тела цикла. Так что можно смело предположить, что вместо e в условии должна быть переменная se.

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

Кажется, из этой истории можно вынести два урока:

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

За такие ценные уроки это срабатывание удостоилось четвёртого места.

3 место. Happy debugging, folks

Тройку призёров открывает срабатывание из проверки NetBeans. Кажется, все предыдущие примеры кода были довольно маленькими, так что давайте посмотрим на что-то более объёмное:

private void mergeParallelInclusions(List<IncludeDesc> inclusions,
                                     IncludeDesc original,
                                     boolean preserveOriginal) {
  IncludeDesc best = null;
  ....
  // 2nd remove incompatible inclusions, move compatible ones to same level
  for (Iterator it=inclusions.iterator(); it.hasNext(); ) {
    IncludeDesc iDesc = (IncludeDesc) it.next();
    if (iDesc != best) {
      if (!compatibleInclusions(iDesc, best, dimension)) {
        it.remove();
      } else if (iDesc.parent == best.parent &&
                 iDesc.neighbor == best.neighbor &&
                (iDesc.neighbor != null || iDesc.index == iDesc.index)) {
        it.remove(); // same inclusion twice (detect for better robustness)
      }
      ....
    }
    ....
  }
  ....
  if (unifyGaps != null) {
    // unify resizability of the border gaps collected for individual inclusions
    for (LayoutInterval[] gaps : unifyGaps) {
      int preferredFixedSide =
                         fixedSideGaps[LEADING] >= fixedSideGaps[TRAILING] ?
                                                   LEADING : TRAILING;
      for (int i=LEADING; i <= TRAILING; i++) {
        if (LayoutInterval.canResize(gaps[i]) && !anyResizingNeighbor[i]
            && (anyResizingNeighbor[i^1] || preferredFixedSide == i)) {
          operations.setIntervalResizing(gaps[i], false);
          if (!LayoutInterval.canResize(gaps[i^1])) {
            operations.setIntervalResizing(gaps[i^i], true);
          }
          break;
        }
      }
    }
  }
}

Ради спортивного интереса могу предложить вам (в последний раз) попытаться найти ошибку самостоятельно, а я пока подожду.

Поискали?

Хорошо. Те из вас, кто нашёл ошибку только в выражении iDesc.neighbor != null || iDesc.index == iDesc.index, проиграли :)

Она там действительно есть, но недостаточно интересная, чтобы попасть в топ. Да, ошибки тут две, я вас немного обманул. Но какие могут быть праздники без шалостей? :)

На самом деле анализатор здесь обнаружил ещё и ошибку в выражении i^i, выдав следующее сообщение:

V6001 There are identical sub-expressions 'i' to the left and to the right of the '^' operator. LayoutFeeder.java 3897

Операция XOR здесь не имеет никакого смысла, ведь исключающее "ИЛИ" с двумя одинаковыми значениями всегда будет равно нулю. Если вы забыли, как работает эта операция, то вот таблица истинности:

a	b	a^b
0	0	0
0	1	1
1	0	1
1	1	0

Иными словами, операция истинна только тогда, когда операнды разные. У нас же все биты будут одинаковыми, так как значение одно и то же.

Почему мне так понравилась эта ошибка? Буквально над ней есть операция i^1, которая визуально похожа до степени смешения. Таким образом, при код-ревью будет невероятно просто пропустить эту ошибку, так как сверху мы уже увидим корректное i^1.

Не знаю, как вам, но мне это напомнило знаменитое

#define true rand() % 2
// Happy debugging, folks!

Иначе сложно объяснить, как это попало в код. Если, конечно, отмести скучную версию с простой опечаткой. И да, если вы всё-таки смогли сами найти именно эту ошибку, то отмечайтесь в комментариях :)

2 место. Когда паттерны подвели

Выше я уже показал ошибки из первой и третьей статей по DBeaver, пропустив вторую. Исправляюсь. Следующая как раз из второй статьи.

Анализатору PVS-Studio не понравилось, что из конструктора класса TextWithOpen вызывают isBinaryContents, который переопределяется в наследниках:

public TextWithOpen(Composite parent, boolean multiFS, boolean secured) {
    super(parent, SWT.NONE);

    ....
    if (!useTextEditor && !isBinaryContents()) {
        ....

        editItem.setEnabled(false);
    }

    ....
}

protected boolean isBinaryContents() {
    return false;
}

Ну, переопределяется и переопределяется, какая разница. По большому счёту это code smell, и в самом по себе факте использования такого подхода ничего критичного нет. По крайней мере, я так думал раньше — о моём сложном отношении с этой ошибкой я и писал в полной статье.

У TextWithOpen много наследников, и одним из них является TextWithOpenFile. Там метод действительно переопределяется, и вместо false возвращается поле, которого нет у родителя:

public class TextWithOpenFile extends TextWithOpen {
    private final boolean binary;
    ....

    @Override
    protected boolean isBinaryContents() {
        return binary;
    }
    ....
}

Так, уже подозрительнее, а как выглядит конструктор этого класса?

public TextWithOpenFile(
    Composite parent,
    String title,
    String[] filterExt,
    int style,
    boolean binary,
    boolean multiFS,
    boolean secured
) {
    super(parent, multiFS, secured);
    this.title = title;
    this.filterExt = filterExt;
    this.style = style;
    this.binary = binary;
}

Заметили? Поле binary инициализируется после того, как вызывается родительский конструктор. Но ведь в этом самом конструкторе идёт обращение к полю наследника!

Собственно, вот и сообщение PVS-Studio:

V6052 Calling overridden 'isBinaryContents' method in 'TextWithOpen' parent-class constructor may lead to use of uninitialized data. Inspect field: binary. TextWithOpenFile.java(77), TextWithOpen.java 59

Получается достаточно забавная картина. На первый взгляд, разработчик повёл себя правильно: вместо спаггетти, которое невозможно поддерживать, попытался реализовать каноничное ООП через шаблонный метод. Но при реализации даже такого простого паттерна можно допустить ошибку, что и случилось. И, на мой взгляд, такая (ошибочная) красота в простоте — это уверенное второе место.

1 место. Одна ошибка компенсировала другую

Ну что же, вот и первое место. Борьба за него была сильна, но выбирать было надо. По итогу я решил взять ещё одно срабатывание из проверки NetBeans. Представляю вам последний фрагмент кода:

private void changeIgnoreStatus (File f) throws IOException {
  File parent = f;
  boolean isDirectory = f.isDirectory() && (! Files.isSymbolicLink(f.toPath()));
  StringBuilder sb = new StringBuilder('/');
  if (isDirectory) {
    sb.append('/');
  }
  boolean cont = true;
  while (cont) {
    sb.insert(0, parent.getName()).insert(0, '/');
    parent = parent.getParentFile();
    String path = sb.toString();
    if (parent.equals(getRepository().getWorkTree())) {
      if (addStatement(new File(parent, Constants.DOT_GIT_IGNORE),
          path, isDirectory, false) &&
          handleAdditionalIgnores(path, isDirectory)) {
        addStatement(new File(parent, Constants.DOT_GIT_IGNORE),
                              path, isDirectory, true);
      }
      cont = false;
    } else {
      cont = addStatement(new File(parent, Constants.DOT_GIT_IGNORE),
                                   path, isDirectory, false);
   }
}

Я почти уверен, что сходу такую ошибку найти невозможно, если сами её раньше не допускали. Не буду томить и сразу покажу предупреждение PVS-Studio:

V6009 Buffer capacity is set to '47' using a char value. Most likely, the '/' symbol was supposed to be placed in the buffer. IgnoreUnignoreCommand.java 107

На самом деле ошибка до смешного элементарная: у конструктора StringBuilder нет перегрузки, которая принимает char. А какой тогда вызывается конструктор? Разработчик, видимо, думал, что будет вызвана перегрузка, принимающая String. Тогда начальное значение StringBuilder-a было бы этим слешем.

Однако происходит неявное приведение типов, и вызывается конструктор, принимающий int. И в нашем случае он отвечает за начальный размер StringBuilder-а. Из-за этого передача char в качестве аргумента функционально не влияет ни на что, ведь в итоговую строку она не попадёт. А если начальный размер будет превышен, то он просто увеличится сам по себе, не вызвав исключений или других побочных эффектов.

Но в заголовке раздела я, кажется, сказал про две ошибки. Где вторая, и какая между ними связь? Чтобы в этом разобраться, придётся вчитаться в тело метода и понять, что этот код делает.

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

• "/folder1/file" для файла;
• "/folder1/folder/" для директории.

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

• "/folder1/file/";
• "/folder1/folder//"

То есть мы бы получили лишний слеш в конце строки. А оказался бы он в конце потому, что код выше каждый раз добавляет новый текст в начало строки.

Таким образом, вторая ошибка заключается в том, что этот слеш вообще передали в конструктор как аргумент. Но так как сделано это было неправильно, то никакого эффекта не возымело. Впрочем, недооценивать такую ошибку я бы не стал, ведь если кто-то решит без проверки исправить символ на строку, то могут возникнуть проблемы.

Вот так первое место в топе ошибок получает код, который работает правильно — новогодние чудеса, а вы чего ожидали? :)

Заключение

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

Если вам интересны другие языки, то приглашаю вас прочитать про топ багов за 2024 год в C# здесь.

Все эти ошибки были найдены с помощью анализатора PVS-Studio. Недавно вышла версия 7.34, и вы можете её попробовать по этой ссылке.

А чтобы следить за выходом новых статей про качество кода, можете подписаться на:

• PVS-Studio в X;
• наш ежемесячный дайджест статей;
• мой личный блог.

С наступающим!