Почему PVS-Studio не предлагает автоматические правки кода

    Почему PVS-Studio не предлагает автоматические правки кода

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

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

    Причина в том, что PVS-Studio не является анализатором стиля кода. Он не предлагает изменения, связанные с форматированием или именованием. Не предлагает он (по крайней мере на момент написания статьи :) заменить в C++ коде все NULL на nullptr. Это хоть и хорошее предложение, но оно не имеет практически ничего общего с поиском и устранением ошибок.

    PVS-Studio выявляет ошибки и потенциальные уязвимости. Многие ошибки заставляют задуматься и требуют изменения поведения программы. И только программист может решить, как исправить ту или иную ошибку.

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

    Рассмотрим ошибку, которую я разбирал в статье "31 февраля".

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

    Анализатор понимает, что обе проверки являются истинными. Но почему, анализатору непонятно. Он ничего не знает про дни, месяцы и прочие сущности. И научить понимать такое ой как непросто. Единственное, что реально сделать — это чтобы анализатор предлагал упростить функцию:

    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 true;
      } else {
        return true;
      }
    }

    Или, чего уж мелочиться, пусть он предложит такую автоматическую замену:

    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;
      }
      return true;
    }

    Прикольно, но бессмысленно ;). Анализатор убрал код, который с точки зрения языка C++ является лишним. И только человек может понять, является код действительно избыточным (а такое тоже часто бывает), или в коде допущена опечатка и надо заменить month на day.

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

    В августе этого вирусного года я написал статью о проверки библиотеки PMDK. Среди прочего в статье рассматривалась ошибка неправильной защиты от переполнения:

    static DWORD
    get_rel_wait(const struct timespec *abstime)
    {
      struct __timeb64 t;
      _ftime64_s(&t);
      time_t now_ms = t.time * 1000 + t.millitm;
      time_t ms = (time_t)(abstime->tv_sec * 1000 +
        abstime->tv_nsec / 1000000);
    
      DWORD rel_wait = (DWORD)(ms - now_ms);
    
      return rel_wait < 0 ? 0 : rel_wait;
    }

    Раз переменная rel_wait имеет беззнаковый тип, то последующая проверка rel_wait < 0 не имеет смысла. Предупреждение PVS-Studio: V547 [CWE-570] Expression 'rel_wait < 0' is always false. Unsigned type value is never < 0. os_thread_windows.c 359

    Кто-то воодушевился статьёй и начал массово исправлять описанные в ней ошибки: Fix various issues reported by PVS-Studio analysis.

    И как же было предложено исправить код? Весьма бесхитростно: core: simplify windows timer implementation.

    Неудачное исправление кода

    Но код был упрощен, а не исправлен! Это заметили и началась соответствующая дискуссия: ISSUE: os_thread_windows.c — get_rel_wait() will block if abstime is in the past.

    Как видите, даже люди ошибаются в предложенных правках. Куда уж пытаться роботам.

    Да и вообще, желание автоматической правки ошибок — это очень странное желание. Каждая правка, исправляющая баг, требует внимания и обзора кода. Более того, анализатор может выдавать ложные срабатывания, а значит править такой код вообще нельзя. Анализ кода и работа с предупреждениями — это не то место, где надо спешить. Лучше внедрять регулярный анализ кода и потихоньку исправлять ошибки, появляющиеся в новом коде.


    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Why PVS-Studio Doesn't Offer Automatic Fixes.
    PVS-Studio
    Статический анализ кода для C, C++, C# и Java

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

      +3

      "не надо так"… Я прямо даже и не знаю, как сказать.


      А в сях есть способ написать


      dword r = a — b;
      if (was_overflow) r=0;


      чтоб скомпилировалось в проверку флага после вычитания?

        +2
        Как в Rust'е, да? Типа checked_sub. Если я не ошибаюсь, то нет. Вычитание неотрицательных DWORD чисел само по себе уже опасно. Как мы можем сравнивать результат с нолём в отрицательную сторону?

        Я не знаю, как другие бы сделали это лаконично, я бы сперва числа сравнил друг с другом перед вычитанием. Ради интереса, я сейчас изучаю Rust, и стал на эти проблемы чаще обращать своё внимание.
          +2
          В стандартных пока что нет (работы по добавлению ведутся), но в GCC и clang давно уже да.
            0
            Спасибо.

            Интересно, а

            dword r = a-b;
            if (b > a) return 0; else return r;

            соптимизируется на ASM уровне само или нет?
              0
              Несложно проверить через Compiler Explorer.

              GCC (O2):
              mov eax, edi
              mov edx, 0
              sub eax, esi
              cmp edi, esi
              cmovb eax, edx
              ret

              Clang (O2):
              xor eax, eax
              sub edi, esi
              cmovae/cmovge eax, edi (ae for uint32_t, ge for int32_t)
              ret

              Остальные компиляторы можете попробовать по аналогии там же.
                0
                да, уже подёргал там в разных вариациях.
                ни один из них не свернулся в sub + jo/jno…

                При этом вариант с __builtin_sub_overflow сворачивается хорошо
                godbolt.org/z/sq63d3
                  0
                  Зато на ARM получается отлично:
                  subs w8, w0, w1 # вычесть из w0 (a) w1 (b), положить результат в w8 (a-b)
                  csel w0, wzr, w8, lo # сравнить содержимое wzr (ноль) с w8 (a-b), если строго меньше, то положить в w0 (возвращаемое из функции значение) wzr (ноль), иначе w8 (a-b)
                  ret
                    0
                    В каком варианте сырка?
                    Что-то GCC/arm с -O3 мне даже вариант с builtin выдаёт что-то странное.
                      0
                        0
                        Кстати, а чем не устроил вариант выше с cmov? Он быстрее, чем sub/jo, потому что в нем вообще переходов нет (и потому нельзя их неверно предсказать).
                          +1
                          Проблема не в cmov/jo а в двух — sub + cmp, когда sub это уже cmp по сути.
                          то есть да, clang делает хорошо :)

                          я ступил и смотрел только разные варинты записи для GCC.
                  0
                  Даже на -O3 нет: godbolt.org/z/dG4shf
              +2

              А вот компилятор Rust'а предлагает, и в 99% случаев его предложения правильные. Почему? Потому что компилятор ууууумный. А язык правильный.


              Последняя Mother of All Errors, которую я словил, добавив в невинную структуру ещё одно поле состояла из ~10 строчек, в которых мне объяснялось, что из-за этого поля (совсем в другом месте, совсем другую структуру) нельзя больше безопасно передавать между тредами. Причём в ошибке мне даже сказали, что можно ещё в одном месте заменить Fn на FnOnce, и тогда будет хорошо. Я сделал — и стало хорошо.

                +2

                Ну, справедливости ради, многое из того, что проверяет PVS-studio и при этом не завязано на язык (вроде одинаковых веток в условном операторе), сейчас не ловят ни компилятор, ни Clippy, так что есть куда расти.

                0
                Рассмотрим ошибку, которую я разбирал в статье

                Ошибка лютая, конечно.
                Прикольно, но бессмысленно ;). Анализатор убрал код, который с точки зрения языка C++ является лишним.

                А если как-то так поставить вопрос, чтобы это служило индикатором некооректной логики?
                  –1
                  некооректной
                  некорректной
                  0

                  Шикарный пример, спасибо.

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

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