Ложные срабатывания в PVS-Studio: как глубока кроличья нора

    Единорог PVS-Studio и GetNamedSecurityInfo

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


    Мы много работаем над тем, чтобы сократить количество ложных срабатываний, выдаваемых анализатором PVS-Studio. К сожалению, статические анализаторы часто не могут отличить корректный код от ошибки, так как им просто не хватает информации. В результате, ложные срабатывания в любом случае есть. Впрочем, это не является проблемой, так как проведя настройку анализатора легко достичь ситуации, когда 9 из 10 предупреждений будут указывать на настоящие ошибки.

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

    Недавно один из наших клиентов написал письмо приблизительно следующего содержания:

    Анализатор почему-то говорит, что указатель всегда нулевой, хотя это явно не так. Более того, анализатор ведёт себя странно и нестабильно на тестовом проекте: то выдаёт предупреждение, то не выдаёт. Синтетический пример, на котором воспроизводится ложное срабатывание:

    #include <windows.h>
    #include <aclapi.h>
    #include <tchar.h>
    
    int main()
    {
      PACL pDACL = NULL;
      PSECURITY_DESCRIPTOR pSD = NULL;
      ::GetNamedSecurityInfo(_T("ObjectName"), SE_FILE_OBJECT,
         DACL_SECURITY_INFORMATION, NULL, NULL, &pDACL, NULL, &pSD);
      auto test = pDACL == NULL; // V547 Expression 'pDACL == 0' is always true.
      return 0;
    }

    Я представляю, как выглядят подобные срабатывания со стороны наших пользователей. Сразу понятно, что функция GetNamedSecurityInfo меняет значение переменной pDACL. Неужели разработчики не смогли написать обработчик для таких простых ситуаций? Более того, непонятно, почему анализатор то выдаёт сообщение, то нет. Может быть, у них самих какой-то баг в инструменте, типа неинициализированный переменной?

    Эх… Непростая работа — поддерживать статический анализатор кода. Но что делать, я сам выбрал себе такую судьбу. Засучив рукава, я приступил расследовать причину возникновения ложного срабатывания.

    Для начала я изучил описание функции GetNamedSecurityInfo и убедился, что её вызов действительно должен привести к изменению значения переменной pDACL. Вот описание 6-ого аргумента функции:
    ppDacl

    A pointer to a variable that receives a pointer to the DACL in the returned security descriptor or NULL if the security descriptor has no DACL. The returned pointer is valid only if you set the DACL_SECURITY_INFORMATION flag. Also, this parameter can be NULL if you do not need the DACL.

    Я знаю, что анализатор PVS-Studio однозначно должен корректно обрабатывать такой простой код и не выдавать бессмысленное предупреждение. Уже в этот момент моя интуиция подсказала мне, что это будет необычный случай, на который придётся потратить время.

    Я утвердился в своих опасениях, когда не смог воспроизвести ложное срабатывание ни на текущей alpha-версии анализатора, ни на именно той версии, которая установлена у клиента. И так, и сяк, но анализатор молчит.

    Я попросил клиента прислать мне препроцессированный i-файл, сгенерированный для программы с синтетическим примером. Он сгенерировал, прислал файл, и я приступил к его детальному рассмотрению.

    На присланном файле анализатор сразу выдал ложное срабатывание. С одной стороны, это хорошо, так как баг воспроизведён. С другой стороны, я испытал чувства, которые лучше всего описывает вот эта картинка.

    шта


    Почему именно эти? Я отлично знаю, как работает анализатор и диагностика V547. Ну не может быть такого срабатывания!

    Ok, заварим чай и продолжим.

    Вызов функции GetNamedSecurityInfo разворачивается в:

    ::GetNamedSecurityInfoW(L"ObjectName", SE_FILE_OBJECT,
      (0x00000004L), 0, 0, &pDACL, 0, &pSD);

    Этот код одинаково выглядит как в моём собственном препроцессированном i-файле, так и в файле, присланном клиентом.

    Хм… Хорошо, теперь изучим, как объявлена эта функция. В своём файле я вижу:

    __declspec(dllimport)
    DWORD
    __stdcall
    GetNamedSecurityInfoW(
           LPCWSTR               pObjectName,
           SE_OBJECT_TYPE         ObjectType,
           SECURITY_INFORMATION   SecurityInfo,
                PSID         * ppsidOwner,
                PSID         * ppsidGroup,
                PACL         * ppDacl,
                PACL         * ppSacl,
          PSECURITY_DESCRIPTOR   * ppSecurityDescriptor
        );

    Всё логично, всё понятно. Ничего неожиданного.

    Далее я заглядываю в файл клиента и…

    шта???


    Там я вижу что-то из параллельной реальности:

    __declspec(dllimport)
    DWORD
    __stdcall 
    GetNamedSecurityInfoW(
          LPCWSTR               pObjectName,
          SE_OBJECT_TYPE         ObjectType,
          SECURITY_INFORMATION   SecurityInfo,
         const PSID         * ppsidOwner,
         const PSID         * ppsidGroup,
         const PACL         * ppDacl,
         const PACL         * ppSacl,
         PSECURITY_DESCRIPTOR   * ppSecurityDescriptor
        );

    Обратите внимание, что формальный аргумент ppDacl помечен как const.

    WAT? WTF? WAT? WTF?

    Что за const!? Откуда он здесь!?

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

    Аргумент является указателем на константный объект. Получается, что с точки зрения анализатора функция GetNamedSecurityInfoW не может менять объект, на который ссылается указатель. Следовательно, здесь:

    PACL pDACL = NULL;
    PSECURITY_DESCRIPTOR pSD = NULL;
    ::GetNamedSecurityInfo(_T("ObjectName"), SE_FILE_OBJECT,
       DACL_SECURITY_INFORMATION, NULL, NULL, &pDACL, NULL, &pSD);
    auto test = pDACL == NULL; // V547 Expression 'pDACL == 0' is always true.

    переменная pDACL не может измениться и анализатор выдаёт обоснованное предупреждение (Expression 'pDACL == 0' is always true.).

    Почему выдаётся предупреждение — понятно. Зато непонятно, откуда взялся этот const. Его просто не может там быть!

    Впрочем, есть догадка, и она подтверждается поисками в интернете. Оказывается, существует старый неправильный файл aclapi.h с некорректным описанием функции. Также я нашёл в интернете две интересные ссылки:


    Итак, жило-было в файле aclapi.h (6.0.6002.18005-Windows 6.0) вот такое описание функции:

    WINADVAPI
    DWORD
    WINAPI
    GetNamedSecurityInfoW(
        __in  LPWSTR                pObjectName,
        __in  SE_OBJECT_TYPE         ObjectType,
        __in  SECURITY_INFORMATION   SecurityInfo,
        __out_opt PSID                 * ppsidOwner,
        __out_opt PSID                 * ppsidGroup,
        __out_opt PACL                 * ppDacl,
        __out_opt PACL                 * ppSacl,
        __out_opt PSECURITY_DESCRIPTOR * ppSecurityDescriptor
        );

    Затем кто-то захотел исправить тип формального аргумента pObjectName, но попутно испортил типы указателей, вписав const. И aclapi.h (6.1.7601.23418-Windows 7.0) стал таким:

    WINADVAPI
    DWORD
    WINAPI
    GetNamedSecurityInfoW(
        __in LPCWSTR pObjectName,
        __in SE_OBJECT_TYPE ObjectType,
        __in SECURITY_INFORMATION SecurityInfo,
        __out_opt const PSID * ppsidOwner,
        __out_opt const PSID * ppsidGroup,
        __out_opt const PACL * ppDacl,
        __out_opt const PACL * ppSacl,
        __out PSECURITY_DESCRIPTOR * ppSecurityDescriptor
        );

    diff 1


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

    Вот как выглядит уже исправленное описание функции в aclapi.h (6.3.9600.17415-Windows_8.1).

    WINADVAPI
    DWORD
    WINAPI
    GetNamedSecurityInfoW(
        _In_ LPCWSTR pObjectName,
        _In_ SE_OBJECT_TYPE ObjectType,
        _In_ SECURITY_INFORMATION SecurityInfo,
        _Out_opt_ PSID * ppsidOwner,
        _Out_opt_ PSID * ppsidGroup,
        _Out_opt_ PACL * ppDacl,
        _Out_opt_ PACL * ppSacl,
        _Out_ PSECURITY_DESCRIPTOR * ppSecurityDescriptor
        );

    diff 2


    Тип аргумента pObjectName остался прежним, а вот лишние const убрали. Всё вернулось на свои места, но в мире пока продолжают жить заголовочные файлы с неправильным объявлением функции.

    Всё это я рассказываю клиенту. Он рад и доволен, что ситуация прояснилась. Более того, он находит причину, почему он то видит ложное срабатывание, то нет:

    Я забыл, что я когда-то на этом тестовом проекте экспериментировал с тулсетами. В тестовом проекте Debug конфигурация настроена на Platform Toolset по умолчанию для Visual Studio 2017 — «Visual Studio 2017 (v141)», а вот Release конфигурация настроена на «Visual Studio 2015 — Windows XP (v140_xp)». Вчера я просто в какой-то момент менял конфигурации, и предупреждение то появлялось, то исчезало.

    Всё. В расследовании можно ставить точку. С клиентом решаем, что не будем делать какую-то специальную подпорку в анализаторе, чтобы он учитывал этот баг в заголовочном файле. Главное, что теперь ситуация понятна. Как говорится, «дело закрыто».

    Заключение

    Анализатор PVS-Studio — сложный программный продукт, собирающий из кода программы множество информации и использующий её для различных технологий анализа. Конкретно в данном случае излишняя интеллектуальность анализатора привела к тому, что из-за неправильного описания функции он начал выдавать ложное срабатывание.

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



    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. False Positives in PVS-Studio: How Deep the Rabbit Hole Goes.
    PVS-Studio
    859,00
    Static Code Analysis for C, C++, C# and Java
    Поделиться публикацией

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

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

      +8
      Класс. Найти ошибку в файлах майкрософта предлагаю сделать программой минимум для испытания анализаторов) Например, в VS2017 в Community\Common7\IDE был отличный файл srcsrv.ini, примерно такого содержания:
      ;Path to tf.exe. We should have a better solution, and Dev15 Bug 194740 is tracking this…
        +1
        Круто! Спасибо за статью!
          +3
          Непонятно только, почему Вы везде пишете «ложное срабатывание». Мне кажется, что анализатор прав, он ведь работает не по документации, а по реальному заголовочному файлу, так? И не важно, что все эти стилистические изыски в коде не учитываются реальным ABI.
            +4
            Это пользователь говорит «ложное срабатывание».
              +1
              Возможно, мне показалось, что в конце статьи это написали Вы. Неважно, но приношу извинения.
              Мне кажется, что истинный «пафос» статьи в том, что либо реальные системные библиотеки Windows собираются не с теми заголовками, которые поставляются в составе публичного SDK, либо собираются со значимыми предупреждениями, если не с ошибками компиляции. Казалось бы, при чем здесь Лужков?! (С).
                0
                Сам того не планируя, я начал писать статьи, посвященные развенчанию мифа, что статические анализаторы — это 90% шума и 10% пользы. Да, действительно, статические анализаторы в силу своей природы дают ложные срабатывания. Однако, подавляющее количество ложных срабатываний можно исключить, проведя минимальную настройку анализатора. Большинство бессмысленных срабатываний связано с неудачными макросами и опасным стилем кодирования (например, нет проверки указателя после malloc).

                Примечание. Кстати, в срабатываниях на макросах анализатор часто формально прав. Но с практической точки зрения это никого не волнует и получается, что это всё равно ложные предупреждения :).

                К сожалению, настроить анализатор недостаточно. Всё равно возникает ощущение, что многие срабатывания неверны. Иногда, когда начинаешь разбираться, это не так. Поэтому мои статьи наделены предостеречь программистов от поспешных выводов.

                Плюс ещё бывают такие интересные ситуации, как описаны здесь. С практической точки зрения это ложное срабатывание анализатора, хотя инструмент и не виноват. Тут собственно никакой и морали-то нет. Просто захотелось рассказать про эту ситуацию. Хотя нет, вру. Есть мораль: мы оказываем глубокую качественную поддержку. Становитесь нашим клиентом, и мы поможем побороть любое зло.
                  0
                  Плюс ещё бывают такие интересные ситуации, как описаны здесь. С практической точки зрения это ложное срабатывание анализатор

                  Андрей, подскажите, а не может ли возникнуть ситуации когда компилятор что-то «наоптимизирует» на основе некорректных const qualifiers, что в итоге приведёт к некорректному «с практической точки зрения» исполнению кода? Я имею в виду «сложные» оптимизации, в т.ч. основанные на undefined behavior и т.п. если что...

                    0
                    Я не могу дать точный ответ, но вижу ситуацию следующим образом. Совсем чисто теоретически такое можно представить. Например, компилятор, как и анализатору, может предположить, что переменная не изменится и удалит проверку, заменив её на true. На практике такое не случится, так как компилятор не видит внутренности тела функции и не рискнёт сделать такую смелую оптимизацию. А вдруг внутри функции кто-то снимет константность, используя const_cast.
                      0

                      Компилятор имеет право и может заменить это выражение на true.


                      const_cast снимает модификатор const со ссылки, но не позволяет писать в неё. Попытка сделать это приводит к undefined behavior.


                      Так что хоть я не смог заставить ни один компилятор на godbolt превратить эту проверку в true, полагаться на такое поведение компиляторов не стоит.

                        0
                        Я всегда за то, чтобы писать правильные программы. Но кажется здесь не тот случай, чтобы ожидать подвоха. Интересно кстати будет послушать мнение других участников.

                        Если не менять объекты после const_cast, то тогда зачем вообще этот оператор нужен? :)

                        Даже разработчики компиляторов так делают. Вот первый попавшийся фрагмент кода из Clang:

                        static void getCLEnvVarOptions(
                          std::string &EnvValue, llvm::StringSaver &Saver,
                          SmallVectorImpl<const char *> &Opts)
                        {
                          llvm::cl::TokenizeWindowsCommandLine(EnvValue, Saver, Opts);
                          for (const char *Opt : Opts)
                            if (char *NumberSignPtr = const_cast<char *>(::strchr(Opt, '#')))
                              *NumberSignPtr = '=';
                        }
                        
                          +4

                          Ммм. Вы правы. Я неправильно интерпретиловал следующую цитату:


                          Even though const_cast may remove constness or volatility from any pointer or reference, using the resulting pointer or reference to write to an object that was declared const or to access an object that was declared volatile invokes undefined behavior.

                          А конкретно я упустил


                          using the resulting pointer or reference to write to an object that was declared const

                          Т.е. если у нас есть объект, который не объявлен как const, мы его скастовали в const, а потом кастом сняли const, в него можно писать и это не ошибка.


                          Так что кажется что ни в моём, ни в вашем примере UB нет.
                          Если функция получает объект по const ref, компилятор не знает как этот объект был объявлен. А значит компилятор не может предположить, что неконстантный объект, который мы передали по константной ссылке не может быть изменён.

                0
                Хотя нет, я вот на что среагировал:
                Конкретно в данном случае излишняя интеллектуальность анализатора привела к тому, что из-за неправильного описания функции он начал выдавать ложное срабатывание.
              0

              А можно подробнее раскрыть утверждение: "Аргумент является указателем на константный объект. Получается, что с точки зрения анализатора функция GetNamedSecurityInfoW не может менять объект, на который ссылается указатель. "?


              void foo(const int** pp){
              const int* pi;
              *pp = pi;
              }
              const int* p;
              foo(&p);

              Сигнатура функции гласит утрировано const T** p; Это же про константый объект, но не указатель на который указывает переменная. Откуда взялся вывод, что функция не может менять указатель, если указатель указывает на указатель, который не константый?

                0
                Легко могу ошибиться, но я понимаю проблему так:
                #define PINT int*
                void f(const PINT * pi);
                // препроцессируется в псевдокод
                void f(const (int *) * pi);
                // east const rules!!!
                void f( int * const * pi);
                
                  0

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


                  #define PINT int*
                  void foo(const PINT* pp){
                      *pp = nullptr;
                  }
                  
                  typedef int* pint;
                  void foo(const pint* pp){
                      // *pp = nullptr;  
                  }
                  
                  int main(int argc, char** argv)
                  {
                      const int* p;
                      foo(&p);
                  
                      int * const pc = NULL;
                      foo(&pc);
                  
                      int* pi = NULL;
                      foo(&pi); // Case 2.
                      return 0;
                  }
                  
                    0
                    Тут вообще разница в обработки объявлений сигнатуры функции и определения функции.
                    В C квалификатор в передающемся по значению аргументе не учитывается и часто является причиной предупреждения компилятора.
                    В C++ (если мне не изменяет склероз) такой квалификатор не учитывается, но допускается.
                    В определении функций квалификаторы имеют смысл и в C, и в C++, поскольку в определении функции аргумент является определением локальной переменной. В этом случае квалификатор const «играет», хотя в C его смысл и отличается от C++ (тут константа просто константа, а не readonly переменная).
                    Если бы advapi32.dll компилировалась с теми заголовками, в которых аргументы квалифицированы, как const, то было бы как минимум предупреждение компилятора о несоответствии прототипа определению функции.
                    Как-то так,
                    Искренне Ваш КО)
                  0
                  typedef int near *PINT;
                  foo(const PINT *x);
                  
                  В функцию передаётся указатель на константный объект. Значит, функция не может изменить объект, который пришёл в функцию из вне. В данном случае из вне приходит объект типа PACL (который на самом деле указатель). И вот этот объект (указатель) менять нельзя.
                    0
                    А поменялось ли само предупреждение после исследования?
                    Возможно дополнительные сведения помогли бы пользователю разобраться самостоятельно.
                      0
                      Сколь часто Вы рассматриваете препроцессированный код единицы компиляции? Увы, я даже не помню, когда делал это сам, хотя с 1992 года пару раз делал, помню)))
                      Статья на самом деле классная в частности именно тем, что напоминает о важности этого «секретного приемчика» в разработке.
                        0
                        Первый и последний раз смотрел для файлов, сгенерированных flex и bison лет 5 назад, больше и не помню.
                        Эта простыня из заголовочных файлов помогает ощутить весь объём неиспользуемых зависимостей, можно открыть для себя что-то новое и неожиданное :)
                        +1
                        Нет. К сожалению, сделать объясняющее предупреждение намного сложнее, чем может показаться на первый взгляд.

                        Уже предвижу комментарий, что полезно показать путь вывода правила (как работал анализ потока данных). Т.е. из каких условий и операций следует, что получится ложь/истина/индекс за границами массива и т.д. Я работал с такими инструментами и, к сожалению, пришел к выводу, что часто путь не помогает, а только ещё больше усложняет картину.

                        Кстати, здесь бы и путь не помог. Вот переменная равна 0. Вот вызывается функция, которая не меняет эту переменную. Вот всегда истинное условие. То, что анализатор посчитал, что функция не меняет значение, это и так было понятно. Если же анализатор ещё начнёт отправлять ко всем объявлениям функций в системных заголовочных файлах… Человек тогда вообще потеряется в обилии данных и удалит анализатор :).
                        0
                        И, что наиболее печально, нельзя его менять только с точки зрения статического анализатора (including but not limited to extended compiler wornings))). С точки зрения языка константность/волатильность аргумента, передаваемого по значению и/или возвращаемого значения не являются частью прототипа функции, не участвуют в overloading'ге и т.д.
                        Что меня всегда удивляло, так это то, что в C T const * и T * — разные типы в списке аргументов прототипа функции, но не в возвращаемом значении.
                        Если я не прав, то пусть лучше знающие стандарты C и C++ меня поправят.
                          +1
                          Но ведь аргумент был передан не по значению, а по указателю…

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

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