Нам в поддержку написал пользователь о странном ложном срабатывании анализатора PVS-Studio. Сейчас станет понятно, почему этот случай заслуживает отдельной маленькой статьи и насколько у программистов может быть замылен взгляд.
Пользователи время от времени присылают нам различные фрагменты C++ кода, на которые анализатор PVS-Studio, по их мнению, выдал странные/ложные предупреждения. После чего мы дорабатываем диагностики или выписываем идеи по доработкам на потом. В общем, идёт обыкновенная работа по поддержке пользователей.
Однако в этот раз программист, который получил письмо от пользователя, прибежал возбуждённый ко мне с идеей, что про этот случай стоит что-то написать в блог. Мы не можем назвать пользователя или показать его код. Поэтому вот переписанный сокращённый вариант:
char *SomeGlobalPointer {};
void foo()
{
SomeGlobalPointer = new char[1024];
}
int main()
{
foo();
if (!SomeGlobalPointer)
{
delete[] SomeGlobalPointer;
}
return 0;
}Программист со стороны пользователя жалуется на "ложное" срабатывание PVS-Studio:
V575 The null pointer is passed into 'operator delete[]'. Inspect the argument.
Какой же это нулевой указатель? Вон же он явно инициализируется в функции.
Это интересный пример, когда код выглядит настолько простым, что человек не видит ошибку. Код для освобождения памяти скучен, и в него сложно вчитаться на обзоре кода. Я уже описывал подобный эффект, например, в статье "Зло живёт в функциях сравнения".
На самом деле перед нами неприметная опечатка в условии:
if (!SomeGlobalPointer)
{
delete[] SomeGlobalPointer;
}Оператор delete[] вызывается только в том случае, если указатель нулевой. В результате возникает утечка памяти. Естественно, должно быть наоборот:
if (SomeGlobalPointer)
{
delete[] SomeGlobalPointer;
}На самом деле проверка вообще не нужна. Оператор delete[] корректно обрабатывает нулевые указатели. Код можно упростить:
char *SomeGlobalPointer {};
void foo()
{
SomeGlobalPointer = new char[1024];
}
int main()
{
foo();
delete[] SomeGlobalPointer;
return 0;
}Поскольку мы заговорили о рефакторинге, то сюда ещё просится умный указатель (std::unique_ptr или std::shared_ptr в зависимости от ситуации). Конечно, код вообще выглядит странным, но не забывайте, что в статье приведён синтетический вариант. Впрочем, мы отвлеклись.
Согласитесь, это красивая опечатка. Она настолько проста, что её не заметили. Более того, даже когда анализатор выдал предупреждение на этот код, программист не увидел проблему. Вместо этого написал нам письмо о том, что анализатор, видимо запутался из-за использования глобально переменной и выдал ложное срабатывание.
Если бы не PVS-Studio, в коде так и осталась бы эта ошибка, приводящая к утечке памяти.
Любите статический анализ кода! Прокачайте процесс обзора кода, используя дополнительно анализатор кода!
Некоторые другие статьи о том, как статический анализ компенсирует нашу человеческую невнимательность:
- В очередной раз анализатор PVS-Studio оказался внимательнее человека.
- Как PVS-Studio оказался внимательнее, чем три с половиной программиста.
- Один день из жизни разработчика PVS-Studio, или отладка диагностики, которая оказалась внимательнее трёх программистов.
- Конкурс внимательности: PVS-Studio vs Хакер.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Simple, yet easy-to-miss errors in code.
Комментарии (65)
viordash
12.09.2023 12:57+10немного про стили в условиях. Для себя давно уже определил что ! или просто выражение применять только к булевым, те это условие я бы писал как
if (SomeGlobalPointer == nullptr).В таком варианте при прочтении кода лучше видны не стыковки.
vasan
12.09.2023 12:57+4Кстати хорошая практика использовать явное приведения аргумента условия к булеву типу. Те, кто начинал программировать на Object Pascal (Delphi, Lazarus итп), стараются хотя бы поначалу делать именно так, поскольку в Паскале передавать в if в качестве аргумента не булевы типы считается ошибкой.
Malizia
12.09.2023 12:57+4Ну тогда сразу if (nullptr == SomeGlobalPointer) чтобы исключить ошибочное присваивание if (SomeGlobalPointer = nullptr).
viordash
12.09.2023 12:57+5о нее, это не мое:) я читатель кода, и когда читаю что
если 0 равен..., и тут меня клинит, ноль может быть равен только нулю.Мне кажется уже давно эта проблема присваиваний в условии ловится анализаторами.
ReadOnlySadUser
12.09.2023 12:57+2А я наоборот люблю Yoda style. Именно за то, что обычно то, какая переменная будет проверяться, чаще всего понятно из контекста, и на деле я хочу глазами сразу увидеть значение, которое ожидается в условии. Так просто удобнее читать)
Helltraitor
12.09.2023 12:57Думаю, стоит считать конструкции a = b ошибкой компиляции в любом месте, если каким либо образом не сказано иначе (хотя, что должно возвращать операция присваивания - для меня загадка)
Gromilo
12.09.2023 12:57public static void Main() { int a, b, c, d; a = b = c = d = 1; Console.WriteLine($"{a} {b} {c} {d}"); } //1 1 1 1Возвращает само значение. Код на шарпах, но помнится на плюсах можно так же делать.
Можно ещё в условии присваивание и сравнение сразу делать, типа `if((b = a) > 10)`. Но я такое на рефакторинге не пропустил бы.
Tujh
12.09.2023 12:57+5я бы писал как
if (SomeGlobalPointer == nullptr)Между прочим, этим вы нарушаете С++ Guidelines
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-if
Andrey2008 Автор
12.09.2023 12:57+5Если смотреть не из мира розовых единорогов, где весь код короткий и при этом правильный, то это плохой совет. На мой взгляд, длинный код лучше читается и более защищает от ошибок. По крайней мере в контексте сравнения с
nullptr.Вот, например, как узнать что хотели проверить? Указатель или значение по указателю?
int *p = foo(); if (p) .... // невозможно сказать, верно написано или нет if (*p) .... // невозможно сказать, верно написано или нет if (p != nullptr) .... // скорее всего всё хорошо, хотели проверить именно указатель if (*p != nullptr) .... // ошибка компиляцииИли вот такой реальный пример из проекта Mozilla Firefox:
const char *token = str.get(); if (token == '\0') { return NS_ERROR_DOM_SYNTAX_ERR; // nothing between commas }Если-бы человек был фанатом сокращения текста, он бы написал:
if (!token)И тогда анализатор PVS-Studio не смог-бы найти ошибку. А за счёт дополнительной информации, что сравнивают не с абстрактным нулём, а с нулевым литералом '\0', анализатор способен предположить, что здесь опечатка:
V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: *token == '\0'. svgnumberlist.cpp 96
В общем, забыли разыменовать указатель. Правильно:
if (*token == '\0')
slamon
12.09.2023 12:57-2if (p) .... // невозможно сказать, верно написано или нет
if (*p) .... // невозможно сказать, верно написано или нетСерьезно? Быть может вам стоит попрактиковаться в программировании?
Если-бы человек был фанатом сокращения текста, он бы написал:
Никто бы так не написал. Человек четко понимает, что работает с текстом, и отчетливо осознает что именно хочет проверить
datacompboy
12.09.2023 12:57+1К сожалению, if(p) vs if (*p) встречаются многократно при долгосрочной жизни кода. Требования меняются, набивки объектов меняются, передачи копией, ссылкой, указателем, тушкой, чучелком -- меняются друг на друга. И да, вот такие implicit проверки становятся минами замедленного действия, всплывающими когда их не ждёшь.
technic93
12.09.2023 12:57По хорошему не должно быть чтобы и p и *p имели такую семантику. Это означает что у вас в коде есть что то вроде
optional<optional<T>> pdatacompboy
12.09.2023 12:57Почему стразу optional?
if (p) / if (*p)
одинаково успешно компилируются и для для int / int*, int[], int**.
Tujh
12.09.2023 12:57+1По крайней мере в контексте сравнения с
nullptr.Как раз сравнени с nullptr и не повышает читаемость. Ну или у вас сказывается профессиональная деформация, когда "дуют на молоко, обжегшись водой" :)
if (p) .... // невозможно сказать, верно написано или нет
if (*p) .... // невозможно сказать, верно написано или нетНе очень корректный пример, потому что не видно, что потом делают с указателем.
По второй строке частично соглашусь, потому что правильным (относительно абстрактного примера) было бы написать:
if(p && *p) ...а теперь перепишите эту же проверку с использованием избыточности
if((p != nullptr) && (*p != '\0')) ...для меня, в этом примере избыточность ухудшает читаемость, потому что в глаза, в первую очередь, бросается с чем сравнивается указатель, а не операции с ним самим, а значит на review можно пропустить ошибку с отсутствием "*" во втором случае, в короткой форме эта проверка явственно видна. Собственно для чего и написана рекомендация.
Или вот такой реальный пример из проекта Mozilla Firefox:
А тут стоит дочитать рекомендацию до конца (я же привёл только часть)
тут не integer, но и char может принимать далеко не два значения, так что рекомендация прямо говорит, что в вашем примере с Mozilla Firefox она не применима.
И, конечно, всегда нужно руководствоваться здравым смыслом.
technic93
12.09.2023 12:57Любой компилятор даст варнинг на код мозиллы, так что этот пример как аргумент не зачитываю. Во всех ваших примерах понятно что хотели написать, это в коде написано.
killAllHumans() // не понятно возможно хотели loveAllHumans() loveAllHumansWithMyHartIRellyMeanIt() // вот так лучше
enree
12.09.2023 12:57+4clang-tidy, наоборот, против: https://clang.llvm.org/extra/clang-tidy/checks/readability/implicit-bool-conversion.html
oficsu
12.09.2023 12:57У него достаточно взаимопротиворечивых проверок, так что апелляция к нему сомнительна
ReadOnlySadUser
12.09.2023 12:57Мне нравится Reason. "eliminates some opportunities for mistakes". В то же самое время порождает opprotunities для других ошибок :)
В общем, из всего правила, я согласен только с тем, что не стоит сравнивать указатели с нулём. Сранивать с
nullptrболее чем имеет смысл, если рассуждать со стороны моего личного опыта. Просто на практике ни разу не встретил ошибок, связанных с явным сравнением, но с десяток раз уже натыкался на баги, связанные именно с желанием сократить код.
oficsu
12.09.2023 12:57+2В таком варианте при прочтении кода лучше видны не стыковки.
Это иллюзия, проявившаяся из-за того, что вы прямо сейчас погружены в контекст проблемы, понимаете проблему и думаете, что вне этого контекста так же легко заметите проблему. Стоит вам только забыть контекст и спустя время вернуться к коду, увидев такие строчки...
if (SomeGlobalPointer == nullptr) { delete[] SomeGlobalPointer; }...никаких несостыковок вы тут не заметите. И явность вам никак вообще не поможет. Потому что проблема не в отсутствии явности, а в том, что мозг крайне плохо обрабатывает в условиях инварианты, завязанные на инверсию
Лично мне же такой код ещё и усложняет чтение. Ведь для кода в голове обычно строится некоторая ментальная модель. И она строится не на конструкциях языка. В моём, примере, мозг поступает иначе — определяет зависимости между телом ветки и содержимым условия. Это необходимая для понимания сути кода низкоуровневая часть ментального процесса. И независимо от того, способны ли вы её пронаблюдать в своём мозгу сознательно, она на некотором уровне работы мозга отрабатывает
И если в исходном условии зависимость лишь одна —SomeGlobalPointer, возможных исходов в ментальной модели тоже два — либо у насSomeGlobalPointer, либо у нас неSomeGlobalPointer. Добавив же== nullptr, мы привнесли в ментальную модель ещё как минимум одну сущность. И не важно, что это неизменная часть условия
В нашей ментальной модели теперь сущностей, от которых зависит исполнение тела, стало в два раза больше — две. Также и возможное число исходов (где-то в фоне считаемое мозгом) увеличилось до четырёх —SomeGlobalPointer, неSomeGlobalPointer,nullptr, неnullptr
Это лишь увеличит когнитивную нагрузку при том же семантическом наполнении кода. Вы будете уставать чуть быстрее. Уставая же, будете совершать новые ошибки. Решали одну конкретную частную проблему — добавили новых неочевидным для себя образом
Xeldos
12.09.2023 12:57+5Мне нравилась в Perl конструкция unless. Казалось бы, сахарный сахар, но сразу видно, где "если да", а где "если нет". Очень семантично, я считаю.
alan008
12.09.2023 12:57+1Немного не по теме статьи, но каково ваше ощущение (по прошествии пары лет) от востребованности анализаторов для C# и Java , а также анализатора MISRA и CVE?
Или всё-таки C++ был и остаётся основным направлением работы PVS Studio?
Можно в л.с., если это sensitive информация.
Просто интересуюсь темой статического анализа, но мы пишем на Delphi и у нас свой самописный анализатор под наши задачи.
Andrey2008 Автор
12.09.2023 12:57+2Интерес к C# и Java растёт, но медленно. Думаю, мы сами в этом виноваты, так как команде не хватает сил более активно заниматься их продвижением. Про Java, например, вообще давно статей не писали :(. На тему C++ у нас куда больше разнообразного материала и активностей. В том числе больших и сильных, таких как мини-книга про вредные C++ советы.
MISRA - интерес есть, но у малого количества пользователей. Хорошо, что сделали, но существенно на жизнь это не повлияло. По запросам пользователей скоро будем дорабатывать это направление и делать ещё некоторые диагностики.
CVE - как я понимаю, имеется в виду SCA (software composition analysis). Пока не понятно.
C++ остаётся основным направлением? - Да. Оно самое старое, поэтому там больше всего клиентов и поддержки. И с продвижением у нас в этом ловчее получается.
alan008
12.09.2023 12:57+1Спасибо, Андрей!
Большую и хорошую работу делаете! Насчет CVE — вчера вечером был немного не в себе и сам не до конца разобрался, что я имел в виду. У меня в памяти застряла вот эта статья:
https://habr.com/ru/companies/pvs-studio/articles/418143/
И я считал (возможно, ошибочно), что PVS-Studio как-то особенно сигнализирует именно об уязвимостях из списка CVE.
Насчет SCA — тоже интересная тема, не знал о ней ранее.Насчет сравнения языков — C++ всё-таки лидер по стрелянию себе по ногам, возможно поэтому C# и Java менее востребованы, в них изначально меньше косячат + многое выявляется компилятором. Возникнет ли надежная замена языку С++ — это вопрос. Все пророчили Rust, но там местами запутанность конструкций не меньшего уровня, чем в С++, хоть и safe.
Теоретически, мог бы взлететь анализатор для Python или PHP, но это опять же новые направления, требующие еще и еще ресурсов.
SquareRootOfZero
12.09.2023 12:57+6Казалось бы, поставь брекпойнт да проверь, выполняется ли деле delete[] и чему при этом равен указатель. Прежде чем уличать в ложном срабатывании ажно само PVS-Studio! Признайтесь, вы этот пример сами выдумали? :)
Опять же, какое-то сбивающее с толку предупреждение:
V575 The null pointer is passed into 'operator delete[]'. Inspect the argument.
При том что, как сами пишете, оператор delete[] корректно обрабатывает нулевые указатели. Что оно сказать-то хочет? Что указатель возможно нулевой или что указатель обязательно нулевой?
mentin
12.09.2023 12:57+8+1 про сбивающее предупреждение. Было бы там The pointer passed into ... is always null, его бы думаю прекрасно понимали.
Andrey2008 Автор
12.09.2023 12:57+1Признайтесь, вы этот пример сами выдумали? :)
Нет. Такой простой случай сложно придумать :) Правильнее будет сказать, я его "услышал". Хотя можно без кавычек. Это в прямом смысле.
Слышу вдруг коллега в соседней секции: "Ааааа!" и прочие эмоции. Заинтригован, подхожу. Что такое? Он рассказывает и показывает. Я понимаю, что это стоит маленькой заметки :)
Про V575.
Сообщение говорит, что в оператор
deleteвсегда передаётся нулевой указатель. Если бы он не всегда был нулевым, то и предупреждения не будет. Это ведь нормально. А вот передача явногоnullptrговорит и какой-то ошибке или избыточном коде. Возможно, опечатались и не ту переменную используют. Не очень понятно, что именно сбивает с толку. на мой взгляд вполне понятное предупреждение. Хотя, конечно, у меня может быть взгляд замылен :).
boldape
12.09.2023 12:57Если бы не PVS-Studio, в коде так и осталась бы эта ошибка, приводящая к утечке памяти.
Все зависит от вашего определения утечки памяти. Если ваш синтетический тест повторяет структуру реального кода, то я бы не согласился с тем что это утечка, т.к. это уже конец мэйна - программа завершена. Опять же можно спорить об определениях и эта "утечка" может мешать анализу настоящих утечек с помощью дебажного алакатора, но по факту конкретно в этом случае это не утечка. Если уж совсем дотошно, то с точки зрения самого с++ это вообще не утечка при любом раскладе, т.к. утечкой язык называет аллоцированную память на которую нет ни единого валидного указателя, а тут он есть.
SquareRootOfZero
12.09.2023 12:57+2Так они своим тестовым кодом воспроизводят не (возможную) утечку памяти, а якобы ложное срабатывание статического анализатора кода. Будет ли там реально утечка памяти, статический анализатор вам не определит.
Andrey2008 Автор
12.09.2023 12:57+2Что вы хотите от синтетического примера? :) Его смысл показать, что предупреждение показалось ложным, а оно не ложное.
Естественно, в реальном приложении, всё это где-то глубоко в потрохах. И утечка памяти самая настоящая. Естественно, реальная программа не заканчивает сразу после этого работу, как в моём примере :).
Gromilo
12.09.2023 12:57+5Пишу на C# и у нас на проекте принято соглашение не использовать восклицательный знак, т.к. его не видно. Пишем такую вот, на первый взгляд, странную конструкцию:
if(project.IsEnabled == false) { //Что-то делаем }Сначала непривычно, но быстро привыкаешь. Ищешь отрицание в конце, а не в начале.
Кроме малозаметности, ещё вижу проблему с отрицаниями через восклицательный знак. Когда последовательно читаешь код, нужно держать в голове это отрицание по доберёшься до того, что отрицаешь.
Не страна.город.мэрия.зачем.так.много.вложенности.удалена //удалена что? а! не удалена
vs
страна.город.мэрия.зачем.так.много.вложенности.удалена отрицаем удаление // теперь не читается как предложение, зато понятно о каком свойстве идёт речь.
SergeyMax
12.09.2023 12:57+2Признайтесь, у вас KPI по количеству кода?)
Gromilo
12.09.2023 12:57+1К счастью, нет (:
Я вот что ещё понял: мы обычно сравниваем одни значения с другими и только булы особенные: с ними либо ничего не делают, либо отрицают (C#). Т.е. если увидел булевую переменную в выражении, будь любезен сделай глазами влево, чтоб узнать есть там восклицательный знак или нет (и ты его точно не пропустил).
Если использовать сравнение через `== false` вместо отрицания, то ответ на вопрос, было отрицание или нет, всегда будет находится справа от переменной. Лично мне так оказалось удобнее.
a-tk
12.09.2023 12:57Как вам повезло, что Вы не застали в естественном ареале обитания такую дичь, как операторы true и false в дополнение к приведению к bool для пользовательских типов.
Hidden text
The
trueoperator returns the bool valuetrueto indicate that its operand is definitely true. Thefalseoperator returns theboolvaluetrueto indicate that its operand is definitely false. Thetrueandfalseoperators aren't guaranteed to complement each other. That is, both thetrueandfalseoperator might return theboolvaluefalsefor the same operand. If a type defines one of the two operators, it must also define another operator.
a-tk
12.09.2023 12:57Попробуйте провести эксперимент с коллегами с pattern matching-ом вида
if (project is {IsEnabled: false})
Gromilo
12.09.2023 12:57И появляется дополнительная проверка на то, что проект не null. Возникает вопрос: это специально или побочный эффект?
В данном случае я бы не стал пользоваться паттерн матчингом, т.к. проще не становится.
a-tk
12.09.2023 12:57Ну, если у вас там и так нету NRE и JIT в этом уверен, то вы за это не заплатите.
В данном случае семантика не меняется
Gromilo
12.09.2023 12:57Согласен, JIT убирает проверку, если точно уверен, например, если экземпляр создали через new. Но чаще значение мы получаем из функции или из параметра и для таки переменных проверка всё равно будет, хоть переменная и не nullable.
Для параметра видно 2 джампа на L0028.
Но это всё экономия на спичках
igormich88
12.09.2023 12:57Примерно из таких соображений в Котлине к коллекциям прикрутили метод расширение isNotEmpty в дополнение к стандартному isEmpty, чтобы вероятность ошибиться была меньше.
dopusteam
12.09.2023 12:57Не страна.город.мэрия.зачем.так.много.вложенности.удалена //удалена что? а! не удалена
Если у вас такое большое обращение к вложенном свойству, то оно так или иначе читается тяжело.
khe404
12.09.2023 12:57+2Да интересная ошибка, с одной стороны все очень просто, а с другой стороны вот из таких простых ошибок все проблемы и складываются.
Тут люди делились грамотными методами написания кода, которые позволяют таких ошибок избежать, мне всегда был близок следующий подход:
bool PointerIsDefined = (bool) SomeGlobalPointer; if(PointerIsDefined){ delete [] SomeGlobalPointer; }Т.е. в имени переменной заключается текстом что конкретно проверяется в условном выражении. В случае из статьи это не так принципиально, но когда условий несколько и они сложно сочетаются очень удобно прямо текстом написать что каждое из условий проверяет.
И конечно здорово, что среда разработки уже умеет находить подобные проблемы.
voldemar_d
12.09.2023 12:57Наверное, лучше const bool, или даже так:
const bool PointerIsDefined = (nullptr != SomeGlobalPointer);А то остается вероятность, что значение переменной PointerIsDefined где-то по коду могут изменить. Но еще остается возможность, что bool не изменится, а сам указатель изменится. ИМХО, нужно как следует подумать, разнося одну сущность в две разных переменных. Особенно, если они куда-то еще передаются.
khe404
12.09.2023 12:57-1Согласен с вами const в данном случае уместен и скорее всего поможет оптимизатору принять правильное решение. Может быть даже constexpr.
Что касается сравнения с nullptr это обсуждали выше в комментариях.
Бесспорно, раздвоение сущностей — путь к ошибкам. Однако, разделение условий на понятные составляющие может исключить ошибки и поспособствовать при проверке.
voldemar_d
12.09.2023 12:57Раздвоение сущностей опасно и в другом смысле. Иногда, в результате каких-нибудь ошибок, мусорится память. И в переменную bool PointerIsDefined может попасть значение, не соответствующее текущему состоянию указателя. Понимаю, что ситуация маловероятная, но и незаряженное ружье иногда стреляет.
Travisw
12.09.2023 12:57-1Эти ваши безопасные джавы мигрировали с одной бд на другую и вылетали и набили дублетов в новую базу, потому что разрабы не рассчитали память ОЗУ потребляемую и был java out of memory
Apoheliy
Конечно не нужна! Ведь new не может выдать nullptr.
А поставить обработчик исключений желательно.
CaptainFlint
Andrey2008 Автор
В данном случае согласен. В более общем, указатель может инициализироваться при одних условиях и оставаться нулевым в других. Программисты знают про это и пишут лишние проверки перед
delete(и передfreeв C). Я имел в иду, что такие проверки не нужны, так какdelete(иfree) корректно обработают нулевой указатель.Да, еще есть
new(std::nothrow), но он тут ни при чём.agmt
А ещё и исключение скорее всего не кинет (во многих системах /proc/sys/vm/overcommit_memory = 0), а при обращении OOM пришлёт SIGKILL.
Лечение: поставить в "2" - тогда будешь получать исключение, но gcc перестанет работать.
a-tk
Код может компилироваться под железяку без сигналов и с отключенной поддержкой исключений.
Andrey2008 Автор
На это полагаться нельзя и даже вредно про это думать. Неизвестно как и где будет выполняться программа. Более того, код может быть заимствован для других проектов, работающих в других условиях. Всегда следует исходить из того, что
newможет сгенерировать исключение.P.S. Раньше подобное приходилось про malloc писать, теперь new.... эх...
agmt
Посыл моего сообщения был не в том, что можно доверять тому, что исключения не будет.
А что даже если исключения не будет, доверять тому, что процесс не упадёт - нельзя. Надо обращаться к платформо-зависимым штукам.
voldemar_d
del