Наша команда оказывает быструю и эффективную поддержку клиентов. В поддержке принимают участие только программисты, так как вопросы нам тоже задают программисты и над многими из них приходится подумать. Хочется описать одно из недавних обращений в поддержку на тему ложного срабатывания, которое привело к целому небольшому исследованию описанной в письме проблемы.
Мы много работаем над тем, чтобы сократить количество ложных срабатываний, выдаваемых анализатором 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 с некорректным описанием функции. Также я нашёл в интернете две интересные ссылки:
- Headers diff for advapi32.dll between 6.0.6002.18005-Windows 6.0 and 6.1.7601.23418-Windows 7.0 versions
- Headers diff for advapi32.dll between 6.1.7601.23418-Windows_7.0 and 6.3.9600.17415-Windows_8.1 versions
Итак, жило-было в файле 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
);
Становится понятным, что именно подобный неправильный файл 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
);
Тип аргумента 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.
Комментарии (23)
Ryppka
21.02.2019 11:58+1Непонятно только, почему Вы везде пишете «ложное срабатывание». Мне кажется, что анализатор прав, он ведь работает не по документации, а по реальному заголовочному файлу, так? И не важно, что все эти стилистические изыски в коде не учитываются реальным ABI.
EvgeniyRyzhkov
21.02.2019 12:16+1Это пользователь говорит «ложное срабатывание».
Ryppka
21.02.2019 12:41Возможно, мне показалось, что в конце статьи это написали Вы. Неважно, но приношу извинения.
Мне кажется, что истинный «пафос» статьи в том, что либо реальные системные библиотеки Windows собираются не с теми заголовками, которые поставляются в составе публичного SDK, либо собираются со значимыми предупреждениями, если не с ошибками компиляции. Казалось бы, при чем здесь Лужков?! (С).
Andrey2008 Автор
21.02.2019 14:37Сам того не планируя, я начал писать статьи, посвященные развенчанию мифа, что статические анализаторы — это 90% шума и 10% пользы. Да, действительно, статические анализаторы в силу своей природы дают ложные срабатывания. Однако, подавляющее количество ложных срабатываний можно исключить, проведя минимальную настройку анализатора. Большинство бессмысленных срабатываний связано с неудачными макросами и опасным стилем кодирования (например, нет проверки указателя после malloc).
Примечание. Кстати, в срабатываниях на макросах анализатор часто формально прав. Но с практической точки зрения это никого не волнует и получается, что это всё равно ложные предупреждения :).
К сожалению, настроить анализатор недостаточно. Всё равно возникает ощущение, что многие срабатывания неверны. Иногда, когда начинаешь разбираться, это не так. Поэтому мои статьи наделены предостеречь программистов от поспешных выводов.
Плюс ещё бывают такие интересные ситуации, как описаны здесь. С практической точки зрения это ложное срабатывание анализатора, хотя инструмент и не виноват. Тут собственно никакой и морали-то нет. Просто захотелось рассказать про эту ситуацию. Хотя нет, вру. Есть мораль: мы оказываем глубокую качественную поддержку. Становитесь нашим клиентом, и мы поможем побороть любое зло.
Wyrd
21.02.2019 21:56Плюс ещё бывают такие интересные ситуации, как описаны здесь. С практической точки зрения это ложное срабатывание анализатор
Андрей, подскажите, а не может ли возникнуть ситуации когда компилятор что-то «наоптимизирует» на основе некорректных const qualifiers, что в итоге приведёт к некорректному «с практической точки зрения» исполнению кода? Я имею в виду «сложные» оптимизации, в т.ч. основанные на undefined behavior и т.п. если что...
Andrey2008 Автор
21.02.2019 22:48Я не могу дать точный ответ, но вижу ситуацию следующим образом. Совсем чисто теоретически такое можно представить. Например, компилятор, как и анализатору, может предположить, что переменная не изменится и удалит проверку, заменив её на true. На практике такое не случится, так как компилятор не видит внутренности тела функции и не рискнёт сделать такую смелую оптимизацию. А вдруг внутри функции кто-то снимет константность, используя const_cast.
marsermd
22.02.2019 02:55Компилятор имеет право и может заменить это выражение на true.
const_cast снимает модификатор const со ссылки, но не позволяет писать в неё. Попытка сделать это приводит к undefined behavior.
Так что хоть я не смог заставить ни один компилятор на godbolt превратить эту проверку в true, полагаться на такое поведение компиляторов не стоит.
Andrey2008 Автор
22.02.2019 10:07Я всегда за то, чтобы писать правильные программы. Но кажется здесь не тот случай, чтобы ожидать подвоха. Интересно кстати будет послушать мнение других участников.
Если не менять объекты после 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 = '='; }marsermd
22.02.2019 23:34Ммм. Вы правы. Я неправильно интерпретиловал следующую цитату:
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, компилятор не знает как этот объект был объявлен. А значит компилятор не может предположить, что неконстантный объект, который мы передали по константной ссылке не может быть изменён.
Ryppka
21.02.2019 14:49Хотя нет, я вот на что среагировал:
Конкретно в данном случае излишняя интеллектуальность анализатора привела к тому, что из-за неправильного описания функции он начал выдавать ложное срабатывание.
NightShad0w
21.02.2019 12:41А можно подробнее раскрыть утверждение: "Аргумент является указателем на константный объект. Получается, что с точки зрения анализатора функция GetNamedSecurityInfoW не может менять объект, на который ссылается указатель. "?
void foo(const int** pp){ const int* pi; *pp = pi; } const int* p; foo(&p);
Сигнатура функции гласит утрировано const T** p; Это же про константый объект, но не указатель на который указывает переменная. Откуда взялся вывод, что функция не может менять указатель, если указатель указывает на указатель, который не константый?
Ryppka
21.02.2019 12:46Легко могу ошибиться, но я понимаю проблему так:
#define PINT int* void f(const PINT * pi); // препроцессируется в псевдокод void f(const (int *) * pi); // east const rules!!! void f( int * const * pi);NightShad0w
21.02.2019 12:54Проверил мысль с учетом макроса и псевдонима. Поведение разное. Спасибо за наводку.
#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; }Ryppka
21.02.2019 13:06Тут вообще разница в обработки объявлений сигнатуры функции и определения функции.
В C квалификатор в передающемся по значению аргументе не учитывается и часто является причиной предупреждения компилятора.
В C++ (если мне не изменяет склероз) такой квалификатор не учитывается, но допускается.
В определении функций квалификаторы имеют смысл и в C, и в C++, поскольку в определении функции аргумент является определением локальной переменной. В этом случае квалификатор const «играет», хотя в C его смысл и отличается от C++ (тут константа просто константа, а не readonly переменная).
Если бы advapi32.dll компилировалась с теми заголовками, в которых аргументы квалифицированы, как const, то было бы как минимум предупреждение компилятора о несоответствии прототипа определению функции.
Как-то так,
Искренне Ваш КО)
Andrey2008 Автор
21.02.2019 14:12
В функцию передаётся указатель на константный объект. Значит, функция не может изменить объект, который пришёл в функцию из вне. В данном случае из вне приходит объект типа PACL (который на самом деле указатель). И вот этот объект (указатель) менять нельзя.typedef int near *PINT; foo(const PINT *x);
GCU
21.02.2019 14:39А поменялось ли само предупреждение после исследования?
Возможно дополнительные сведения помогли бы пользователю разобраться самостоятельно.Ryppka
21.02.2019 14:47Сколь часто Вы рассматриваете препроцессированный код единицы компиляции? Увы, я даже не помню, когда делал это сам, хотя с 1992 года пару раз делал, помню)))
Статья на самом деле классная в частности именно тем, что напоминает о важности этого «секретного приемчика» в разработке.GCU
21.02.2019 15:58Первый и последний раз смотрел для файлов, сгенерированных flex и bison лет 5 назад, больше и не помню.
Эта простыня из заголовочных файлов помогает ощутить весь объём неиспользуемых зависимостей, можно открыть для себя что-то новое и неожиданное :)
Andrey2008 Автор
21.02.2019 14:53Нет. К сожалению, сделать объясняющее предупреждение намного сложнее, чем может показаться на первый взгляд.
Уже предвижу комментарий, что полезно показать путь вывода правила (как работал анализ потока данных). Т.е. из каких условий и операций следует, что получится ложь/истина/индекс за границами массива и т.д. Я работал с такими инструментами и, к сожалению, пришел к выводу, что часто путь не помогает, а только ещё больше усложняет картину.
Кстати, здесь бы и путь не помог. Вот переменная равна 0. Вот вызывается функция, которая не меняет эту переменную. Вот всегда истинное условие. То, что анализатор посчитал, что функция не меняет значение, это и так было понятно. Если же анализатор ещё начнёт отправлять ко всем объявлениям функций в системных заголовочных файлах… Человек тогда вообще потеряется в обилии данных и удалит анализатор :).
Ryppka
21.02.2019 14:44И, что наиболее печально, нельзя его менять только с точки зрения статического анализатора (including but not limited to extended compiler wornings))). С точки зрения языка константность/волатильность аргумента, передаваемого по значению и/или возвращаемого значения не являются частью прототипа функции, не участвуют в overloading'ге и т.д.
Что меня всегда удивляло, так это то, что в C T const * и T * — разные типы в списке аргументов прототипа функции, но не в возвращаемом значении.
Если я не прав, то пусть лучше знающие стандарты C и C++ меня поправят.
HobbitSam
Класс. Найти ошибку в файлах майкрософта предлагаю сделать программой минимум для испытания анализаторов) Например, в VS2017 в Community\Common7\IDE был отличный файл srcsrv.ini, примерно такого содержания:
;Path to tf.exe. We should have a better solution, and Dev15 Bug 194740 is tracking this…