Андрей Карпов

21 Фев 2019

Теги:

#Cpp #StaticAnalysis

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

21 Фев 2019

Автор: Андрей Карпов

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

0612_I_show_you_how_deep_the_rabbit_hole_goes_ru/image1.png

Мы много работаем над тем, чтобы сократить количество ложных срабатываний, выдаваемых анализатором 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-файл, сгенерированный для программы с синтетическим примером. Он сгенерировал, прислал файл, и я приступил к его детальному рассмотрению.

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

0612_I_show_you_how_deep_the_rabbit_hole_goes_ru/image2.png

Почему именно эти? Я отлично знаю, как работает анализатор и диагностика 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
    );

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

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

0612_I_show_you_how_deep_the_rabbit_hole_goes_ru/image3.png

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

__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
    );

0612_I_show_you_how_deep_the_rabbit_hole_goes_ru/image4.png

Становится понятным, что именно подобный неправильный файл 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
    );

0612_I_show_you_how_deep_the_rabbit_hole_goes_ru/image6.png

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

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

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

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

Заключение

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

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

#Cpp #StaticAnalysis