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

2019 год подходит к концу, и команда PVS-Studio подводит итоги уходящего года. В начале 2019 года мы расширили возможности анализатора, поддержав язык Java. Поэтому список наших публикаций про проверку открытых проектов пополнился обзорами Java проектов. За год было найдено немало ошибок, и мы решили подготовить Top 10 самых интересных из них.

Десятое место: знаковый byte

Источник: Анализ исходного кода RPC фреймворка Apache Dubbo статическим анализатором PVS-Studio

V6007 Expression 'endKey[i] < 0xff' is always true. OptionUtil.java(32)

public static final ByteSequence prefixEndOf(ByteSequence prefix) {
  byte[] endKey = prefix.getBytes().clone();
  for (int i = endKey.length - 1; i >= 0; i--) {
    if (endKey[i] < 0xff) {                                           // <=
      endKey[i] = (byte) (endKey[i] + 1);
      return ByteSequence.from(Arrays.copyOf(endKey, i + 1));
    }
  }
  return ByteSequence.from(NO_PREFIX_END);
}

Многие программисты считают, что тип, имеющий имя byte, будет беззнаковым. И действительно, часто в разных языках это именно так. Например, в C# тип byte беззнаковый. В Java это не так.

В условии endKey[i] < 0xff автор метода сравнивает переменную типа byte с числом 255(0xff), представленным в шестнадцатеричном представлении. Видимо, при написании метода, разработчик забыл, что диапазон значений типа byte в Java равен [-128, 127]. Данное условие всегда истинно, поэтому цикл for всегда будет обрабатывать только последний элемент массива endKey.

Девятое место: два в одном

Источник: PVS-Studio for Java отправляется в путь. Следующая остановка — Elasticsearch

V6007 Expression '(int)x < 0' is always false. BCrypt.java(429)

V6025 Possibly index '(int) x' is out of bounds. BCrypt.java(431)

private static byte char64(char x) {
  if ((int)x < 0 || (int)x > index_64.length)
    return -1;
  return index_64[(int)x];
}

Сегодня у нас акция! Сразу две ошибки в одном методе. Причина первой ошибки – тип char, который в Java беззнаковый, из-за чего условие (int)x < 0 всегда ложно. Вторая ошибка — это банальный выход за границу массива index_64, когда (int)x == index_64.length. Такая ситуация возможна из-за условия (int)x > index_64.length. Чтобы избавиться от выхода за границу массива, необходимо заменить в условии '>' на '>='. Корректное условие будет таким: (int)x >= index_64.length.

Восьмое место: решение и его последствия

Источник: Анализ кода CUBA Platform с помощью PVS-Studio

V6007 Expression 'previousMenuItemFlatIndex >= 0' is always true. CubaSideMenuWidget.java(328)

protected MenuItemWidget findNextMenuItem(MenuItemWidget currentItem) {
  List<MenuTreeNode> menuTree = buildVisibleTree(this);
  List<MenuItemWidget> menuItemWidgets = menuTreeToList(menuTree);

  int menuItemFlatIndex = menuItemWidgets.indexOf(currentItem);
  int previousMenuItemFlatIndex = menuItemFlatIndex + 1;
  if (previousMenuItemFlatIndex >= 0) {                          // <=
      return menuItemWidgets.get(previousMenuItemFlatIndex);
  }
  return null;
}

Автор метода findNextMenuItem хочет избавиться от -1, возвращаемой методом indexOf, в случае, если список menuItemWidgets не содержит currentItem. Для этого он добавляет к результату indexOf (переменная menuItemFlatIndex) единицу и сохраняет полученное значение в переменной previousMenuItemFlatIndex, которая далее используется в методе. Это решение проблемы с -1 оказывается неудачным, потому что приводит сразу к нескольким ошибкам:

код return null никогда не будет выполнен, потому что выражение previousMenuItemFlatIndex >= 0 всегда истинно, а значит возврат из метода findNextMenuItem всегда будет происходить внутри if;
исключение IndexOutOfBoundsException будет выброшено, когда список menuItemWidgets окажется пуст, потому что произойдет обращение к первому элементу пустого списка;
исключение IndexOutOfBoundsException произойдет, когда аргумент currentItem окажется последним в списке menuItemWidget.

Седьмое место: создание файла из ничего

Источник: Huawei Cloud: в PVS-Studio сегодня облачно

V6008 Potential null dereference of 'dataTmpFile'. CacheManager.java(91)

@Override
public void putToCache(PutRecordsRequest putRecordsRequest)
{
  .... 
  if (dataTmpFile == null || !dataTmpFile.exists())
  {
    try
    {
      dataTmpFile.createNewFile();  // <=
    }
    catch (IOException e)
    {
      LOGGER.error("Failed to create cache tmp file, return.", e);
      return;
    }
  }
  ....
}

При написании метода putToCache программист допустил опечатку в условии dataTmpFile == null || !dataTmpFile.exists() перед созданием нового файла dataTmpFile.createNewFile(). Опечатка — это использование оператора '==' вместо '!='. Из-за этой опечатки будет выброшено исключение NullPointerException при вызове метода createNewFile. Условие после исправления опечатки выглядит так:

if (dataTmpFile != null || !dataTmpFile.exists())

«Ошибку нашли, исправили. Можно расслабиться», – подумаете вы. Но как бы не так!

Исправив одну ошибку, мы обнаружили другую. Сейчас NullPointerException может произойти при вызове dataTmpFile.exists(). Теперь, чтобы избавиться от исключения, необходимо в условии заменить оператор '||' на '&&'. Условие, при котором исчезнут все ошибки, будет таким:

if (dataTmpFile != null && !dataTmpFile.exists())

Шестое место: очень странная логическая ошибка

Источник: PVS-Studio для Java

V6007 [CWE-570] Expression '«0».equals(text)' is always false. ConvertIntegerToDecimalPredicate.java 46

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';
}

Этот метод интересен тем, что в нем присутствует явная логическая ошибка. Если метод satisfiedBy не вернет значение после первого if, то становится известно, что строка text состоит минимум из двух символов. Из-за этого первая же проверка «0».equals(text) в следующем if оказывается бессмысленной. Что же на самом деле разработчик имел в виду, остается загадкой.

Пятое место: вот это поворот!

Источник: PVS-Studio в гостях у Apache Hive

V6034 Shift by the value of 'bitShiftsInWord — 1' could be inconsistent with the size of type: 'bitShiftsInWord — 1' = [-1… 30]. UnsignedInt128.java(1791)

private void shiftRightDestructive(int wordShifts,
                                   int bitShiftsInWord,
                                   boolean roundUp) 
{
  if (wordShifts == 0 && bitShiftsInWord == 0) {
    return;
  }

  assert (wordShifts >= 0);
  assert (bitShiftsInWord >= 0);
  assert (bitShiftsInWord < 32);
  if (wordShifts >= 4) {
    zeroClear();
    return;
  }

  final int shiftRestore = 32 - bitShiftsInWord;

  // check this because "123 << 32" will be 123.
  final boolean noRestore = bitShiftsInWord == 0;
  final int roundCarryNoRestoreMask = 1 << 31;
  final int roundCarryMask = (1 << (bitShiftsInWord - 1));  // <=
  ....
}

При входных аргументах wordShifts = 3 и bitShiftsInWord = 0, переменная roundCarryMask, в которой хранится результат побитового сдвига (1 << (bitShiftsInWord — 1)), окажется отрицательным числом. Возможно, разработчик не ожидал такого поведения.

Четвертое место: а исключения выйдут погулять?

Источник: PVS-Studio в гостях у Apache Hive

V6051 The use of the 'return' statement in the 'finally' block can lead to the loss of unhandled exceptions. ObjectStore.java(9080)

private List<MPartitionColumnStatistics> 
getMPartitionColumnStatistics(....)
throws NoSuchObjectException, MetaException 
{
  boolean committed = false;

  try {
    .... /*some actions*/
    
    committed = commitTransaction();
    
    return result;
  } 
  catch (Exception ex) 
  {
    LOG.error("Error retrieving statistics via jdo", ex);
    if (ex instanceof MetaException) {
      throw (MetaException) ex;
    }
    throw new MetaException(ex.getMessage());
  } 
  finally 
  {
    if (!committed) {
      rollbackTransaction();
      return Lists.newArrayList();
    }
  }
}

Объявление метода getMPartitionColumnStatistics врет нам, говоря, что он может выбросить исключение. При возникновении любого исключения в try переменная committed остается равной false, поэтому в блоке finally оператор return возвращает значение из метода, а все выброшенные исключения теряются и не могут быть обработаны вне метода. Таким образом, любое исключение, возникшие в этом методе, никогда не сможет выбраться из него.

Третье место: кручу, верчу, новую маску получить хочу

Источник: PVS-Studio в гостях у Apache Hive

V6034 Shift by the value of 'j' could be inconsistent with the size of type: 'j' = [0...63]. IoTrace.java(272)

public void logSargResult(int stripeIx, boolean[] rgsToRead)
{
  ....
  for (int i = 0, valOffset = 0; i < elements; ++i, valOffset += 64) {
    long val = 0;
    for (int j = 0; j < 64; ++j) {
      int ix = valOffset + j;
      if (rgsToRead.length == ix) break;
      if (!rgsToRead[ix]) continue;
      val = val | (1 << j);                // <=
    }
    ....
  }
  ....
}

Еще одна ошибка, связанная с побитовым сдвигом, но на этот раз в деле замешан не только он. Во внутреннем цикле for в качестве счетчика цикла используется переменная j [0...63]. Этот счетчик участвует в побитовом сдвиге 1 << j. Ничто не предвещает беды, однако тут в дело вступает целочисленный литерал '1' типа int (32-битное значение). Из этого следует, что результаты побитового сдвига начнут повторяться после того, как j окажется больше 31. Если описанное поведение нежелательно, то единицу необходимо представить как long, например, 1L << j или (long)1 << j.

Второе место: Порядок инициализации

Источник: Huawei Cloud: в PVS-Studio сегодня облачно

V6050 Class initialization cycle is present. Initialization of 'INSTANCE' appears before the initialization of 'LOG'. UntrustedSSL.java(32), UntrustedSSL.java(59), UntrustedSSL.java(33)

public class UntrustedSSL {
  private static final UntrustedSSL INSTANCE = new UntrustedSSL();
  private static final Logger LOG = LoggerFactory.getLogger(UntrustedSSL.class);
  .... 
  private UntrustedSSL() 
  {
    try
    {
      ....
    }
    catch (Throwable t) {
      LOG.error(t.getMessage(), t);           // <=
    }
  }
}

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

Анализатор указал, что статическое поле LOG разыменовывается в конструкторе в тот момент, когда оно инициализировано значением — null, что приводит к возникновению цепочки исключений NullPointerException -> ExceptionInInitializerError.

«Почему же в момент вызова конструктора статическое поле LOG равно null?» – спросите вы.

Исключение ExceptionInInitializerError является подсказкой. Дело в том, что данный конструктор используется для инициализации статического поля INSTANCE, объявленного в классе раньше, чем поле LOG. Поэтому, на момент вызова конструктора, поле LOG все еще не инициализировано. Для корректной работы кода необходимо инициализировать поле LOG до вызова конструктора.

Первое место: копипаст-ориентированное программирование

Источник: Качество кода Apache Hadoop: production VS test

V6072 Two similar code fragments were found. Perhaps, this is a typo and 'localFiles' variable should be used instead of 'localArchives'. LocalDistributedCacheManager.java(183), LocalDistributedCacheManager.java(178), LocalDistributedCacheManager.java(176), LocalDistributedCacheManager.java(181)

public synchronized void setup(JobConf conf, JobID jobId) throws IOException {
  ....
  // Update the configuration object with localized data.
  if (!localArchives.isEmpty()) {
    conf.set(MRJobConfig.CACHE_LOCALARCHIVES, StringUtils
        .arrayToString(localArchives.toArray(new String[localArchives  // <=
            .size()])));
  }
  if (!localFiles.isEmpty()) {
    conf.set(MRJobConfig.CACHE_LOCALFILES, StringUtils
        .arrayToString(localFiles.toArray(new String[localArchives     // <=
            .size()])));
  }
  ....
}

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

localArchives на localFiles;
MRJobConfig.CACHE_LOCALARCHIVES на MRJobConfig.CACHE_LOCALFILES.

Однако даже при такой простой операции была допущена ошибка, так как в строке, на которую указал анализатор, во втором if все еще используется пер��менная localArchives, хотя, скорее всего, подразумевалось использование localFiles.

Заключение

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

Наша команда не остановится на достигнутом и продолжит совершенствовать и улучшать анализатор. Ждите в следующем году новые диагностики и статьи с еще более интересными ошибками.

Я смотрю вы любите приключения! Сначала топ 10 ошибок в С# проектах за 2019 год победили, а теперь и Java смогли одолеть! Добро пожаловать на следующий уровень в статью про лучшие ошибки 2019 года в C++ проектах.

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

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