Ответы на задачи со стенда PVS-Studio на конференциях 2018-2019

    Picture 2


    Привет! Несмотря на то, что сезон конференций 2019 года ещё в самом разгаре, мы бы хотели обсудить задачи, которые ранее предлагали посетителям нашего стенда. Осень 2019 года мы начали с новым набором задач, поэтому уже можно обнародовать решение старых задачек за 2018 год, а также первую половину 2019. Тем более, многие из них были взяты из ранее опубликованных статей, а листовки с задачами содержали ссылку или QR-код с информацией о статье.

    Если вы бывали на конференциях, где мы стояли со стендом, то наверняка видели или даже решали какие-то из наших задач. Это всегда фрагменты кода из реальных открытых проектов на языках программирования C, C++, C# или Java. В коде содержатся ошибки, которые мы предлагаем поискать посетителям. За решение (или просто обсуждение ошибки) мы выдаем призы — статусы на рабочий стол, брелоки и т.п.:

    Picture 4

    Хотите такие же? Приходите на наш стенд на следующих конференциях.

    Кстати, в статьях "Время конференций! Подводим итоги 2018 года" и "Конференции. Промежуточные итоги по первому полугодию 2019" содержится описание нашей активности на конференциях в этом и прошлом году.

    Итак, начнём игру «Найди ошибку в коде». Сначала рассмотрим более старые задачки за 2018 год, будем использовать группировку по языкам программирования.

    2018


    С++


    Chromium bug

    static const int kDaysInMonth[13] = {
      0, 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31
    };
    bool ValidateDateTime(const DateTime& time) {
      if (time.year < 1 || time.year > 9999 ||
          time.month < 1 || time.month > 12 ||
          time.day < 1 || time.day > 31 ||
          time.hour < 0 || time.hour > 23 ||
          time.minute < 0 || time.minute > 59 ||
          time.second < 0 || time.second > 59) {
        return false;
      }
      if (time.month == 2 && IsLeapYear(time.year)) {
        return time.month <= kDaysInMonth[time.month] + 1;
      } else {
        return time.month <= kDaysInMonth[time.month];
      }
    }

    Ответ
    Пожалуй, самая «долгоиграющая» задачка из нашего набора. Эту ошибку в проекте Chromium мы предлагали найти посетителям нашего стенда в течение всего 2018 года. Также она была показана в ходе нескольких докладов.

    if (time.month == 2 && IsLeapYear(time.year)) {
      return time.month <= kDaysInMonth[time.month] + 1;  // <= day
    } else {
      return time.month <= kDaysInMonth[time.month];      // <= day
    }

    В теле последнего блока If-else содержатся опечатки в возвращаемом значении. Вместо time.day программист дважды по ошибке указал time.month. Это привело к тому, что всегда будет возвращено значение true. Ошибка подробно разобрана в статье "31 февраля". Отличный пример ошибки, которую непросто обнаружить на code review. Также это хорошая иллюстрация использования технологии анализа потока данных.

    Unreal Engine bug

    bool VertInfluencedByActiveBone(
      FParticleEmitterInstance* Owner,
      USkeletalMeshComponent* InSkelMeshComponent,
      int32 InVertexIndex,
      int32* OutBoneIndex = NULL);
    
    void UParticleModuleLocationSkelVertSurface::Spawn(....)
    {
      ....
      int32 BoneIndex1, BoneIndex2, BoneIndex3;
      BoneIndex1 = BoneIndex2 = BoneIndex3 = INDEX_NONE;
    
      if(!VertInfluencedByActiveBone(
            Owner, SourceComponent, VertIndex[0], &BoneIndex1) &&
         !VertInfluencedByActiveBone(
            Owner, SourceComponent, VertIndex[1], &BoneIndex2) && 
         !VertInfluencedByActiveBone(
            Owner, SourceComponent, VertIndex[2]) &BoneIndex3)
      {
      ....
    }

    Ответ
    Первым делом следует обратить внимание на то, что последний аргумент функции VertInfluencedByActiveBone() имеет значение по умолчанию и может быть не указан. Теперь взгляните на блок if. Упрощенно его можно переписать так:
    if (!foo(....) && !foo(....) && !foo(....) & arg)

    Теперь видно, что допущена ошибка. Из-за опечатки третий вызов функции VertInfluencedByActiveBone() производится с тремя аргументами вместо четырех, а к результату этого вызова применяется оператор & (побитовое И, слева будет результат функции VertInfluencedByActiveBone() типа bool, справа – целочисленная переменная BoneIndex3). Код при этом компилируется. Исправленный вариант кода (добавлена запятая, закрывающая скобка перемещена в другое место):

    if(!VertInfluencedByActiveBone(
          Owner, SourceComponent, VertIndex[0], &BoneIndex1) &&
       !VertInfluencedByActiveBone(
          Owner, SourceComponent, VertIndex[1], &BoneIndex2) && 
       !VertInfluencedByActiveBone(
          Owner, SourceComponent, VertIndex[2], &BoneIndex3))

    Ошибка, которая первоначально была описана в статье "Долгожданная проверка Unreal Engine 4". В статье ошибка озаглавлена как «Самая красивая из найденных ошибок». Я согласен с данным утверждением.

    Android bugs

    void TagMonitor::parseTagsToMonitor(String8 tagNames) {
      std::lock_guard<std::mutex> lock(mMonitorMutex);
    
      // Expand shorthands
      if (ssize_t idx = tagNames.find("3a") != -1) {
        ssize_t end = tagNames.find(",", idx);
        char* start = tagNames.lockBuffer(tagNames.size());
        start[idx] = '\0';
        ....
      }
      ....
    }

    Ответ
    В условии блока if перепутан приоритет операций. Код работает не так, как задумал программист:

    if (ssize_t idx = (tagNames.find("3a") != -1))

    Переменная idx будет получать значения 0 или 1, а выполнение условия будет зависеть от этого значения, что является ошибкой. Исправленный вариант кода:

    ssize_t idx = tagNames.find("3a");
    if (idx != -1)

    Ошибка из статьи "Проверили с помощью PVS-Studio исходные коды Android, или никто не идеален".

    И еще одна задачка на поиск нетривиальной ошибки в Android:

    typedef int32_t  GGLfixed;
    GGLfixed gglFastDivx(GGLfixed n, GGLfixed d)
    {
      if ((d>>24) && ((d>>24)+1)) {
        n >>= 8;
        d >>= 8;
      }
      return gglMulx(n, gglRecip(d));
    }

    Ответ
    Проблема в выражении (d >> 24) + 1.

    Программист хотел проверить, что 8 старших бит переменной d содержат единицы, но при этом не все биты сразу. Другими словами, программист хотел проверить, что в старшем байте находится любое значение, отличное от 0x00 и 0xFF. Сначала он проверил, что старшие биты ненулевые, написав выражение (d>>24). Далее он сдвигает старшие восемь бит в младший байт. При этом он рассчитывает, что самый старший знаковый бит дублируется во всех остальных битах. То есть если переменная d равна 0b11111111'00000000'00000000'00000000, то после сдвига получится значение 0b11111111'11111111'11111111'11111111. Прибавив 1 к значению 0xFFFFFFFF типа int, программист планирует получить 0 (-1+1=0). Таким образом, выражением ((d>>24)+1) он проверяет, что не все старшие восемь бит равны 1.

    Однако при сдвиге старший знаковый бит вовсе не обязательно «размазывается». Стандарт говорит: «The value of E1 >> E2 is E1 right-shifted E2 bit positions. If E1 has an unsigned type or if E1 has a signed type and a non-negative value, the value of the result is the integral part of the quotient of E1/2^E2. If E1 has a signed type and a negative value, the resulting value is implementation-defined».

    Таким образом, это пример поведения, зависящего от реализации (implementation-defined behavior). Как будет работать этот код — зависит от архитектуры микропроцессора и реализации компилятора. После сдвига в старших битах вполне могут оказаться нули, и тогда результат выражения ((d>>24)+1) всегда будет отличен от 0, то есть будет всегда истинным значением.

    Действительно, непростая задачка. Эта ошибка, как и предыдущая, была описана в статье "Проверили с помощью PVS-Studio исходные коды Android, или никто не идеален".

    2019


    С++


    «Во всём виноват GCC»

    int foo(const unsigned char *s)
    {
      int r = 0;
      while(*s) {
        r += ((r * 20891 + *s *200) | *s ^ 4 | *s ^ 3) ^ (r >> 1);
        s++;
      }
      return r & 0x7fffffff;
    }

    Программист утверждает, что данный код работает с ошибкой по вине компилятора GCC 8. Так ли это?

    Ответ
    Функция возвращает отрицательные значения. Причина в том, что компилятор не генерирует код для оператора побитового И (&). Ошибка связана с неопределённым поведением. Компилятор видит, что в переменной r считается некоторая сумма. При этом прибавляются только положительные числа. Переполнения переменной r произойти не должно, иначе это неопределённое поведение, которое компилятор никак не должен рассматривать и учитывать. Итак, компилятор считает, что раз значение в переменной r после окончания цикла не может быть отрицательным, то операция r & 0x7fffffff для сброса знакового бита является лишней и компилятор просто возвращает из функции значение переменной r.

    Ошибка из статьи "Релиз PVS-Studio 6.26".

    QT bug

    static inline const QMetaObjectPrivate *priv(const uint* data)
    { return reinterpret_cast<const QMetaObjectPrivate*>(data); }
    
    bool QMetaEnum::isFlag() const
    {
      const int offset = priv(mobj->d.data)->revision >= 8 ? 2 : 1;
      return mobj && mobj->d.data[handle + offset] & EnumIsFlag;
    }

    Ответ
    С указателем mobj работают небезопасно. Сначала он разыменовывается, а далее проверяется. Классика.

    Ошибка была описана в статье "Третья проверка Qt 5 с помощью PVS-Studio".

    C#


    Infer.NET bug

    public static void 
      WriteAttribute(TextWriter writer,
                     string name,
                     object defaultValue, 
                     object value, 
                     Func<object, string> converter = null)
    {
      if (   defaultValue == null && value == null 
          || value.Equals(defaultValue))
      {
        return;
      }
      string stringValue = converter == null ? value.ToString() : 
                                               converter(value);
      writer.Write($"{name}=\"{stringValue}\" ");
    }

    Ответ
    В выражении value.Equals(defaultValue) возможен доступ по нулевой ссылке value. Это произойдет при таких значениях переменных, когда defaultValue != null, а value == null.

    Ошибка из статьи "Какие ошибки прячутся в коде Infer.NET?"

    FastReport bug

    public class FastString
    {
      private const int initCapacity = 32;
      private void Init(int iniCapacity)
      { sb = new StringBuilder(iniCapacity); .... }
      public FastString() { Init(initCapacity); }
      public FastString(int iniCapacity) { Init(initCapacity); }
      public StringBuilder StringBuilder => sb;
    }
    ....
    Console.WriteLine(new FastString(256).StringBuilder.Capacity);

    Что будет выведено на консоль? Что не так с классом FastString?

    Ответ
    На консоль будет выведено 32. Причина — опечатка в имени переменной, передаваемой в метод Init в конструкторе:

    public FastString(int iniCapacity){ Init(initCapacity); }

    Параметр конструктора iniCapacity не будет использован. Вместо этого в метод Init передают константу initCapacity.

    Ошибка была описана в статье "Самые быстрые отчёты на диком западе. И горстка багов в придачу..."

    Roslyn bug

    private SyntaxNode GetNode(SyntaxNode root)
    {
      var current = root;
      ....
      while (current.FullSpan.Contains(....))
      {
        ....
        var nodeOrToken = current.ChildThatContainsPosition(....);
        ....
        current = nodeOrToken.AsNode();
      }
      ....
    }
    
    public SyntaxNode AsNode()
    {
      if (_token != null)
      {
        return null;
      }
      
      return _nodeOrParent;
    }

    Ответ
    Возможен доступ по нулевой ссылке current в выражении current.FullSpan.Contains(....). Переменная current может получить нулевое значение как результат выполнения метода nodeOrToken.AsNode().

    Ошибка из статьи "Проверяем исходный код Roslyn".

    Unity bug

    ....
    staticFields = packedSnapshot.typeDescriptions
                   .Where(t => 
                          t.staticFieldBytes != null & 
                          t.staticFieldBytes.Length > 0)
                   .Select(t => UnpackStaticFields(t))
                   .ToArray()
    ....

    Ответ
    Допущена опечатка: вместо оператора && использован оператор &. Это приводит к тому, что проверка t.staticFieldBytes.Length > 0 выполняется всегда, даже в случае равенства null переменной t.staticFieldBytes, что, в свою очередь, приведет к доступу по нулевой ссылке.

    Впервые эта ошибка была показана в статье "Анализируем ошибки в открытых компонентах Unity3D".

    Java


    IntelliJ IDEA bug

    private static boolean checkSentenceCapitalization(@NotNull String value) {
      List<String> words = StringUtil.split(value, " ");
      ....
      int capitalized = 1;
      ....
      return capitalized / words.size() < 0.2; // allow reasonable amount of
                                               // capitalized words
    }

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

    Ответ
    Функция должна возвращать истину, если менее 20% слов начинаются с заглавной буквы. Но проверка не работает, так как происходит целочисленное деление, результатом которого будут только значения 0 или 1. Функция вернёт ложное значение, только если все слова будут начинаться с заглавной буквы. В остальных случаях при делении будет получаться 0, а функция будет возвращать истину.

    Ошибка из статьи "PVS-Studio для Java".

    SpotBugs bug

    public static String getXMLType(@WillNotClose InputStream in) throws IOException
    {
      ....
      String s;
      int count = 0;
      while (count < 4) {
        s = r.readLine();
        if (s == null) {
          break;
        }
        Matcher m = tag.matcher(s);
        if (m.find()) {
          return m.group(1);
        }
      }
      throw new IOException("Didn't find xml tag");
      ....
    }

    Предлагается определить, в чём заключается ошибка поиска xml-тега.

    Ответ
    Условие count < 4 будет выполнено всегда, так как переменную count не инкрементируют внутри цикла. Предполагалось, что поиск xml-тега должен осуществляться только в первых четырёх строках файла, но из-за ошибки будет прочитан весь файл.

    Эта ошибка, как и предыдущая, была описана в статье "PVS-Studio для Java".

    На этом – всё. Ждём вас на следующих конференциях. Ищите стенд с единорогом. Выдадим новые интересные задачки и, конечно же, задарим призы. До встречи!



    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Khrenov. Solutions to Bug-Finding Challenges Offered by the PVS-Studio Team at Conferences in 2018-2019.
    • +40
    • 3,1k
    • 5
    PVS-Studio
    816,91
    Static Code Analysis for C, C++, C# and Java
    Поделиться публикацией

    Похожие публикации

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

      0

      А можете поделиться, на основе двух "более новых" анализаторов — С# и Java, какой из этих двух языков более подвержен ошибкам, выявляемым статический анализом? (на основе тех проектов, которые вы сами проверяли, или на основе какой-то статистики от ваших пользователей)

        0
        Но ведь они оба «C-подобные ООП языки» и сравнивать их по тому, где чаще допускают ошибки бессмысленно, разница лишь в синтаксисе и сахаре
          0

          В чем заключается их Си-подобность мне вообще не понятно. Это языки для написания managed-кода, по механизмам очень далекие от C/C++. С# спроектирован Андерсом Хейлсбергом — архитектором Turbo Pascal и Delphi. Java тоже имеет мало общего с C.
          Вопрос скорее о том, что если в C/C++ изначально понятно, что язык провоцирует на различные ошибки, то в Java/C# языки спроектированы так, чтобы большинство ошибок выявилось на этапе компиляции, а небезопасные конктрукции сведены к минимуму. Отсюда и мой вопрос.

            +1
            По личному ощущению они приблизительно равны. Но достоверность такого ощущения так себе :).
              0

              Не пробовали искать партнеров в департаменте Microsoft, который занимается GitHub'ом, чтобы они вас купили "целиком" и проинтегрировали с GitHub'ом как внутренний инструмент CI/CD? Я понимаю, что для фирмы из РФ это довольно неподъемная/нереальная задача, но чем черт не шутит.

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

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