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


    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.
    PVS-Studio
    278,36
    Static Code Analysis for C, C++, C# and Java
    Поделиться публикацией

    Комментарии 9

      0
      Автор метода findNextMenuItem хочет избавиться от -1, возвращаемой методом indexOf, в случае, если список menuItemWidgets не содержит currentItem


      Не, тут дело похоже в другом.

      Судя по названию метода «findNextMenuItem» и названию переменной «previousMenuItemFlatIndex» предположу что дело было так:
      1. Был метод «findPreviousMenuItem» где было что-то вроде:

          int previousMenuItemFlatIndex = menuItemFlatIndex - 1;
          if (previousMenuItemFlatIndex >= 0) {                         
              return menuItemWidgets.get(previousMenuItemFlatIndex);
          }
      2. Кто-то взял и скопировал этот метод, назвав его «findNextMenuItem»
      3. Заменил " — 1" на " + 1" (чтобы взять следующий элемент вместо предыдущего), а все остальное — названия переменных и проверки — заменить забыл.


        +2

        Картинки со звуком!
        Сам хит-парад ошибок, увы, скучный. Копипаст и невнимательность.
        Хотелось ошибок хотя бы такого уровня.
        Кстати, PVS-Studio ошибку из статьи по ссылке найдёт?

          0

          Ну вот как раз подобный кейс со скоррелированным состоянием в том же методе, который разбирается на пятом месте (смотри мой комментарий ниже). Почему-то в хит-парад попал не он, а ложное срабатывание =)

          +1
           if (text == null || text.length() < 2) {
              return false;
            }
            if ("0".equals(text) || "0L".equals(text) || "0l".equals(text)) {// <=
              return false;
            }

          Я ж вам про это говорил вроде. Это вообще не ошибка, просто перестраховка. Вы смотрите хоть немного логику метода. Да, если на входе строка "0", она отсекается двумя разными способами, в обоих случаях return false; произойдёт, поэтому без разницы, в какую ветку мы зайдём. Может сбить с толку, если не вчитаться в код, но ошибки здесь нет. Сама Идея этот код подсвечивает ещё с 2017-го года (и не заводите волынку, что раз мы его не исправили, то нищитово).

            0

            del

            +1
            final int roundCarryMask = (1 << (bitShiftsInWord - 1));  // <=

            Это тоже вообще не ошибка. Я потому не делаю подобные unsound-варнинги, потому что в них сбалансировать false-positive/false-negative очень сложно. Здесь откровенно мусорный варнинг, который только отвлечёт программистов от реальных проблем. Видите выше noRestore = bitShiftsInWord == 0? А ниже посмотрите на использования noRestore. Вы сразу увидите, что когда bitShiftsInWord == 0, результат битового сдвига (переменная roundCarryMask) не используется вообще. Поэтому абсолютно наплевать, какое там значение.


            Почему-то вы, кстати, молчите про другую вещь в том же методе:


            if (wordShifts == 0 && bitShiftsInWord == 0) {
                return;
            }
            ...
            final boolean noRestore = bitShiftsInWord == 0;
            ...
            switch (wordShifts) {
            case 0:
              // noRestore is always false
              roundCarry = (noRestore ? 0 : (this.v[0] & roundCarryMask)) != 0;
              ...

            Здесь очевидно noRestore всегда ложно, потому что случай wordShifts == 0 && bitShiftsInWord == 0 был обработан выше. Идея радостно подсвечивает эти ветки и предлагает автоматически упростить код в один клик. PVS-Studio не может так? ;-)

              +2

              В целом:


              • Идея репортит 10, 9 (первый кейс), 8, 7, 6, 4.
              • 5 репортить и не стоит, смотрите выше
              • 9.2 репортить было бы хорошо, но это тоже unsound warning (а вдруг снаружи метода всегда проверяется, что x != index_64.length?). У меня была черновая реализация, но возникают помимо хороших варнингов реально очень мутные false-positive, где голову сломаешь перед тем как докажешь, что анализатор неправ. Я поэтому убрал этот код. Возможно, стоит вернуться.
              • 3 должен репортиться инспекцией Integer multiplication or shift implicitly cast to long, но почему-то не срабатывает. Проверю, починю, спасибо!
              • 2 — это интересная штука, у нас прямого аналога нет. Кое-какие циклы инициализации репортятся косвенно, но прямо такой нету.
              • 1 — варнинг крутой, но как я понял инспекция у вас эвристическая. Возникает вопрос, сколько мусора она репортит. Вообще преаллоцированные массивы в toArray() в наши дни — антипаттерн. У нас на это есть инспекция, которая подсвечивает верхний по дефолту, но как раз молчит в нижнем, потому что вдруг пользователь реально хотел массив другой длины (чтобы были null'ы в хвосте).
                0
                А разве в пятом случае описанная ситуация не будет обработана строчкой assert (bitShiftsInWord >= 0);?
                  0
                  assert бросит AssertionError в случае, если bitShiftsInWord будет отрицательным. При рассматриваемых входных данных (wordShifts = 3 и bitShiftsInWord = 0) assert промолчит, т.к. 0 >= 0 true =)

                  Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                  Самое читаемое