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

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

![0699_Best_10_Java_errors_2019_ru/image1.png](https://import.viva64.com/docx/blog/0699_Best_10_Java_errors_2019_ru/image1.png)

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

Источник: [Анализ исходного кода RPC фреймворка Apache Dubbo статическим анализатором PVS\-Studio](https://pvs-studio.ru/ru/blog/posts/java/0663/)

[V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'endKey\[i\] < 0xff' is always true\. OptionUtil\.java\(32\)

```cpp
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](https://pvs-studio.ru/ru/blog/posts/java/0621/)

[V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression '\(int\)x < 0' is always false\. BCrypt\.java\(429\)

[V6025](https://pvs-studio.ru/ru/docs/warnings/v6025/) Possibly index '\(int\) x' is out of bounds\. BCrypt\.java\(431\)

```cpp
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](https://pvs-studio.ru/ru/blog/posts/java/0626/)

[V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'previousMenuItemFlatIndex \>\= 0' is always true\. CubaSideMenuWidget\.java\(328\)

```cpp
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 сегодня облачно](https://pvs-studio.ru/ru/blog/posts/java/0688/)

[V6008](https://pvs-studio.ru/ru/docs/warnings/v6008/) Potential null dereference of 'dataTmpFile'\. CacheManager\.java\(91\)

```cpp
@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_\. Условие после исправления опечатки выглядит так: 

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

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

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

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

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

Источник: [PVS\-Studio для Java](https://pvs-studio.ru/ru/blog/posts/java/0603/)

[V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) \[CWE\-570\] Expression '"0"\.equals\(text\)' is always false\. ConvertIntegerToDecimalPredicate\.java 46

```cpp
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](https://pvs-studio.ru/ru/blog/posts/java/0657/)

[V6034](https://pvs-studio.ru/ru/docs/warnings/v6034/) Shift by the value of 'bitShiftsInWord — 1' could be inconsistent with the size of type: 'bitShiftsInWord — 1' \= \[\-1\.\.\. 30\]\. UnsignedInt128\.java\(1791\)

```cpp
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](https://pvs-studio.ru/ru/blog/posts/java/0657/)

[V6051](https://pvs-studio.ru/ru/docs/warnings/v6051/) The use of the 'return' statement in the 'finally' block can lead to the loss of unhandled exceptions\. ObjectStore\.java\(9080\)

```cpp
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](https://pvs-studio.ru/ru/blog/posts/java/0657/)

[V6034](https://pvs-studio.ru/ru/docs/warnings/v6034/) Shift by the value of 'j' could be inconsistent with the size of type: 'j' \= \[0\.\.\.63\]\. IoTrace\.java\(272\)

```cpp
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 сегодня облачно](https://pvs-studio.ru/ru/blog/posts/java/0688/)

[V6050](https://pvs-studio.ru/ru/docs/warnings/v6050/) Class initialization cycle is present\. Initialization of 'INSTANCE' appears before the initialization of 'LOG'\. UntrustedSSL\.java\(32\), UntrustedSSL\.java\(59\), UntrustedSSL\.java\(33\)

```cpp
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](https://pvs-studio.ru/ru/blog/posts/java/0697/)

[V6072](https://pvs-studio.ru/ru/docs/warnings/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\)

```cpp
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 упрощает выявление ошибок при написании кода, что значительно сокращает количество ресурсов, затрачиваемых на их исправление\. Постоянное использование анализатора уже упростило жизнь разработчикам из многих [компаний](https://pvs-studio.ru/ru/about/customers/)\. Если вы хотите программировать с большим удовольствием, то попробуйте наш [анализатор](https://pvs-studio.ru/ru/pvs-studio/download/)\.

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

Я смотрю вы любите приключения\! Сначала [топ 10 ошибок в С\# проектах за 2019 год](https://pvs-studio.ru/ru/blog/posts/csharp/0698/) победили, а теперь и Java смогли одолеть\! Добро пожаловать на следующий уровень в статью про [лучшие ошибки 2019 года в C\+\+ проектах](https://pvs-studio.ru/ru/blog/posts/cpp/0700/)\.

![0699_Best_10_Java_errors_2019_ru/image2.png](https://import.viva64.com/docx/blog/0699_Best_10_Java_errors_2019_ru/image2.png)