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.