Работа с инструментами статического анализа кода требует внимательности. Очень часто код, на который указал анализатор, кажется корректным. Сразу хочется посчитать предупреждение ложным срабатыванием и пойти дальше. Даже я, один из разработчиков PVS-Studio, попадаю в эту ловушку и не вижу ошибку. На днях я открыл 2 тикета в нашем багтрекере, касающиеся диагностики V614, которая ищет использование неинициализированных переменных и массивов.
В обоих случаях я подумал, что анализатор неправ и в нём надо что-то исправить. Первый случай:
Я четыре раза прочитал этот код, но так и не увидел ошибку. И решил, что это ложное срабатывание, которое надо править. Но прав анализатор, а я просто невнимательный человек.
Буфер caption остаётся неинициализированным. Посмотрите выше, там обе строки загружаются в буфер text. Опечатка. Я не смог её увидеть.
А вот еще более эпичный случай:
Анализатор PVS-Studio говорит, что используется неинициализированный буфер buf. Бред какой-то. И я отписываю этот случай в багтрекере как баг, который надо обязательно поправить. Ведь очевидно, что функция sprintf инициализирует буфер и всё в этом коде хорошо.
Нифига! Вновь прав анализатор PVS-Studio, а не я. Тот случай, когда творение превзошло создателя. :)
Очень нехороший программист в одном из заголовочных файлов написал вот это:
Видите, sprinf раскрывается в std::printf. Да, да, в этой программе sprintf это тоже самое что printf.
Ужас то какой. Получается, что функция printf использует неинициализированный буфер buf как управляющую строку.
Любите и используйте статические анализаторы кода! Они сэкономят вам нервы и время.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Appreciate Static Code Analysis!
Комментарии (64)
aamonster
16.10.2017 16:51Жесть какая (я про второй случай).
Интересно, реально ли разработать специальную диагностику для всяких "#define true false"?
Andrey2008 Автор
16.10.2017 16:58Скорее всего, программа с "#define true false" просто не скомпилируется. Или программа станет столь неработоспособна, что невозможно будет не обратить внимание на это.
В любом случае, "#define true false" это какое-то вредительство и не стоит о нём специально думать. Если уж кто-то захочет навредить, он придумает как это сделать, чтобы это было и не заметно и скрылось от анализаторов.
На практике больший интерес представляют случаи неумышленно неправильного написания #define. Например, могут быть забыты скобки. Мы уже выявляем некоторые такие ситуации (см., например, V1003) и будем развивать анализатор в этом направлении и дальше.aamonster
16.10.2017 18:06Ну просто пример, который вы привели, был именно такого уровня, просто чуть подлее. Типа #define true (rand()%100000!=0).
Andrey2008 Автор
16.10.2017 17:02Кстати, Visual C++ борется с некоторыми подобными случаями (в том числе и с приведённым define). Я писал про это заметку 5 лет назад — Прощай #define private public.
oleg-m1973
16.10.2017 19:44+1А анализатор выдал бы ошибку, типа «неправильный размер буфера», если бы в первом случае размер text был меньше размера caption? Например TCHAR text[128], caption[512];…
Andrey2008 Автор
16.10.2017 19:45+2Посмотрел, как у нас размечена функция LoadString. К сожалению, предупреждения не будет. Доработаем.
Idot
16.10.2017 21:16В легаси коде
//md3_man.cpp typedef char* as_if_string; ... extern "C" as_if_string ReadPictureName(const LPSTR package, const UINT8 ID, as_if_string Extention)
далее, PVS ругается на
//texman.c char* path; ... path=ReadPictureName("Posters",i,"");
и пишет
V647 The value of 'int' type is assigned to the pointer of 'char' type. Consider inspecting the assignment: 'path = ReadPictureName("Posters", i, "")'. texman.c 165
Andrey2008 Автор
16.10.2017 22:03Не понятно, это похвала анализатора или критика за ложное срабатывание? :) Прошу уточнить.
Проверьте, есть ли объявление функции ReadPictureName в texman.c. Скорее всего нет, а раз так, считается, что функция возвращает int. Как следствие, имеем красивую 64-битную ошибку.Idot
16.10.2017 22:19Нет, функции из ж.cpp там вызываются из ж.c без их объявления.
Andrey2008 Автор
16.10.2017 23:09Значит по делу ругается.
Idot
17.10.2017 04:32И как это правильно исправить?
Неужели, поменять char на int?! в офигении
Или описать вызываю функцию явно?Andrey2008 Автор
17.10.2017 10:46Объявите функцию в cpp файле где она используется. Так себе решение, но хоть ошибка исправится.
Idot
17.10.2017 19:20Добавил в textman.c строку:
char* ReadPictureName(const LPSTR package, const UINT8 ID, char* Extention);//it is external! in MD3_Man.cpp
Сообщение об ошибке исчезло :)
(не уверен, что сделал всё правильно правильно, так как после миграции с VC++ 6 на VC++ 2013, проект сейчас вылетает в совсем другом месте)firk
17.10.2017 22:19Правильно — сделать MD3_Man.h с этим прототипом и заинключить его в обоих исходниках.
Idot
18.10.2017 04:31А то, что MD3_Man написан на C++, а texman на обычном простом C, какой на это эффект окажет?
PS в комментариях к MD3_Man.h почему-то написано
//USE extern!!!!!!! just for myselffirk
18.10.2017 13:41+1Тогда так
#ifdef __cplusplus extern "C" #endif char* ReadPictureName(const LPSTR package, const UINT8 ID, char* Extention);
Что означает комментарий не знаю, это в конце концов ваш код и вам лучше надо его знать.
Idot
18.10.2017 14:13Вспомнил, почему добавлено
typedef char* as_if_string;
потому что без этого
extern "C" char* ReadPictureName(const LPSTR package, const UINT8 ID, char* Extention);
при вызове
path=ReadPictureName("Posters",i,"");
не работал корректно и выдавал ошибку.
PS комментарий, вообще не помню откуда, но писать в комментариях "just for myself", вряд ли бы мне пришло в голову (подозреваю, что это легаси, но, точно не уверен).
BinaryShaman
17.10.2017 14:18+2простите, а вы правильно прочитали предупреждение PVS и комментарии?
у вас нет объявления функции в файле textman.c, и С компилятор считает, что функция возвращает int параметр.
т.е. это все равно, что вы напишите
int a = 2;
char *c = a;
Вас же не смутит тут такое же предупреждение?
З.Ы. чисто теоретически интересно, за что минусуют сейчас комменты топик стартера?
firk
17.10.2017 14:11+2Эм, я не понял, а -Wimplicit компилятора вы УЖЕ проигнорировали/отключили? Если да, то в этом и есть корень вашей проблемы, а если нет, то это и правда проблема анализатора, но я в этом сомневаюсь.
FL3
16.10.2017 21:36+1Как же приятно, когда за такими вещами следит сам компилятор!
let mut text = vec![0; 256]; let mut caption = vec![0; 256]; load_string(&mut text); load_string(&mut text); message_box(&text, &caption);
warning: variable does not need to be mutable --> src/main.rs:9:9 | 9 | let mut caption = vec![0; 256]; | ^^^^^^^^^^^ | = note: #[warn(unused_mut)] on by default
Хм, переменные используются одинаково а варнинг только про одну из них, посмотрю ка повнимательней.
Andrey2008 Автор
16.10.2017 21:58Прозвучало (не здесь, а в другом месте), что второй пример выдуманный. Нет, описанные случаи в статье взяты из реальных проектов. Я не стал писать названия, так как не посчитал, что это важно. Источник: definesTypes.h
pavel_pimenov
17.10.2017 09:27Вопросы по примеру
TCHAR text[512], caption[128];
LoadString(GetModuleHandle(NULL),…, text, 512);
LoadString(GetModuleHandle(NULL),…, text, 128);
MessageBox(NULL, text, caption, MB_ICONERROR | MB_OK);
1. Второй вызов LoadString получает размер массива 128, но он не соответствует _countof(text)
почему тут не ловится V512. A call of the 'Foo' function will lead to a buffer overflow or underflow.?
Вообще передавать константу — плохо. в одном месте поправил а в другом забыл.
будет удобно, если анализатор в подобные места будет тыкать.
2. Не обрабатывается код возврата LoadString — в случае ошибки в переменных будет мусор
и MessageBox может уронить программу.Andrey2008 Автор
17.10.2017 11:031) Я уже писал выше, что функция LoadString размечена «не на полную». Сложно аннотируя тысячи функций сразу учесть все способы их неправильного использования. Доработаем. Собственно, во многом благодаря подомным рекомендациям пользователей анализатор улучшает свои возможности.
2) Кажется это было сделано специально. Уж очень никто не проверяет результат LoadString. :) Я изучу этот вопрос.
ViacheslavMezentsev
17.10.2017 14:21А как дела с кросс-платформенным кодом? Могу я в VS проверить makefile проект для avr мк или Arduino проект?
Andrey2008 Автор
17.10.2017 14:27-1Я не понимаю вопрос. Если что-то компилируется в Windows и Linux, Вы можете это проверить, используя мониторинг компиляции (см. документацию). Если у Вас какой-то особый сложный случай, то прошу прийти в поддержку и мы пообщаемся.
rustler2000
18.10.2017 21:05Не скажу за Windows. Но если на линуксе собирается, то проблем нет вообще.
Собрали под мониторингом, проанализировали, впихнули в сонар и наслаждайтесь.
У нас продукты наверное 4-5 компайлерами собираются — и все в один проход срабатывает.
А то что IronHead рассказывает — скорее всего силно криворукие люди были. В свое время мы даже splint под Code Composer Studio гоняли.
mrobespierre
А мы и любим. Подскажите, кстати, какой-нибудь для чистого Си в GNU/Linux бесплатный, лучше open source.
Andrey2008 Автор
PVS-Studio (как использовать PVS-Studio бесплатно).
IronHead
А чтобы работал standalone и не требовал проекта в Visual Studio?
jamepock
… сам исправлял ошибки, коммитил код, общался с пользователями, учавствовал в конференциях от имени разработчиков…
IronHead
Нет, просто не все пишут код в VS. Есть куча других IDE, которые к сожалению не поддерживаются данным ПО.
Andrey2008 Автор
На данный момент возможна интеграция PVS-Studio с CLion, QtCreator, SonarQube и т.д. Подобнее.
IronHead
Уже писал про свои попытки запустить ваш софт в комментариях к предыдущей вашей статье https://habrahabr.ru/company/pvs-studio/blog/337182/#comment_10401688
Andrey2008 Автор
Такая возможность давно существует в PVS-Studio. Есть несколько способов проверки проектов. Проще всего начать с системы мониторинга компиляции: windows, linux (см. «Любой проект»).
lain8dono
en.wikipedia.org/wiki/List_of_tools_for_static_code_analysis
tantrus332
Cppcheck
mrobespierre
спасибо, то, что надо: простое, рабочее, живое!
Jef239
лучше gcc -wall -wexta. Намного выше процент полезных предупреждений. у cppcheck сплошняком бесполезные. Кстати, то, что находит gcc, cppcheck не ищет
rustler2000
clang еще лучше — но pvs еще лучшее :D
mrobespierre
вот не уверен: всегда собираю с -Wall -Wextra -Wpedantic, проверил кое-что через cppheck и уже нашёл кое-какую «пищу для размышлений»
Andrey2008 Автор
А теперь попробуйте PVS-Studio. :)
Jef239
Попробовали на проекте в полторы тысячи строк.
operation on ‘d’ may be undefined [-Wsequence-point]ELM_Answer[d-12] = AnswerBuffer[0][d++];;
The scope of the variable 'b' can be reduced, остальные —Prefer prefix ++/-- operators for non-primitive types, причем непримитивными он обозвал то ли указатели, то ли enum, то ли вообще int.V640 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing.иV560 A part of conditional expression is always true:.На честный запуск ознакомительной версии pvs потратили неделю. Пытались и под windows и под linux… Ответов на написанные тогда письма — ждем до сих пор. В конце концов плюнули и запустили нечестно — правила позволяют один раз проверить в бесплатном варианте, а потом удалить нужные строки.
Так что pvs полезен, но безумно дорог, cppcheck — обычно не оправдывает время на разбор его предупреждений, а -wall -wextra — золотая серединка. Кстати, лучше всего именно в инкрементальном режиме, то есть устранять все выданные предупреждения.
Ritorno
То есть в проекте из 1500 строк (6 файлов?) вы сам не знаете, то ли у вас указатели, то ли enum, то ли вообще int, но виноват статический анализатор?
Jef239
Восхищен вашей память. Помнить год все ложные срабатывания статанализатора в проектах всех сотрудников — это очень сильно. Завидую вам сильно.
P.S. Git подсказывает, что там были постфиксные инкременты у enum. В любом случае — диагностика ложная, пока мы не имеем дело с operator ++.
Andrey2008 Автор
Это заблуждение, которое продолжает тиражироваться. Компании приходят и приобретают инструмент и только некоторые обсуждают скидки. Есть конечно те, кто говорит, что дорого. Но им просто не нужен инструмент и им был бы дорог и CppCat за 250$. Поэтому про это нет смысла говорить.
PVS-Studio в рассчёте на 1 программиста дешевле, чем оплата аренды за квадратные метры в офисе, которые занимает программист. Если взять общие расходы на содержание программиста, то стоимость PVS-Studio вообще не заметна на их фоне.
Jef239
Наш тест показал, что вычистка кода с помощью gcc и cppcheck недостаточна и pvs-studio все равно находит ошибки, причем без ложных срабатываний.
Ну что же, будем считать, что на самом деле PVS-studio работает намного хуже того великолепного результата, что он показал в нашем тесте.
Что касается цены, то разумней на те же деньги (пара миллионов в год уже или больше?) нанять тестера, а то и двух.
Цена PVS-Studio примерно равна аренде офиса на всю фирму. Не все живут в Москве и располагаются с видом на Кремль.
Jef239
Кстати, это хороший критерий, кому есть смысл покупать PVS-Studio. Есть офис с видом на Кремль (или Гудзон) — есть смысл покупать. Офис дешевле — нет смысла.
Иными словами, PVS-Studio — это как малиновый пиджак или розовый лаборджини — нечто, показывающее престижность владельца. Ну Apple заработала миллиарды на такой стратегии, так что почему бы и нет.
Если бы ещё дотянули до продукта… Потому как качество документации — ниже плинтуса, а время ответа на письма… Ну мы уже полгода ждем.
dunkelfalke
с другой стороны, gcc не ищет того, что находит cppcheck. к примеру незакрытые файлы и адресацию за пределами буфера. я в своё с помощью cppcheck хорошо почистил от багов один старый проект, который компилировался без ошибок и предупреждений. статический анализ ищет логические ошибки, а не формальные.
Ritorno
За 4 года использования не увидел у Cppcheck ни одного бесполезного предупреждения.
Для GCC -Wall и -Wextra недостаточно, есть еще много флагов, которые нужно включать вручную.
Jef239
Можно списочек?
Ritorno
Например -Wfloat-equal, а вообще список с каждой версией пополняется/изменяется, поэтому нужно брать документацию от используемой версии и просматривать весь список.
Jef239
Получается, что на каждую версию gcc — свои опции компиляции? Неудобно это.
firk
Использую gcc -Wreturn-type -Wpointer-sign -Wsign-compare -Wshadow -Wpointer-arith -Wimplicit -Werror (список не сразу таким стал)
Недавно пришлось столкнуться с clang — там добавил -Wno-logical-op-parentheses -Wno-parentheses
Вообще думаю что набор актуальных (и возможно-индикаторов-бага) предупреждений у каждого свой, в зависимости от привычек и стиля написания кода.
Jef239
СПАСИБО. -Wshadow, -Wpointer-arith действительно не включаются по -Wall.
-Werror — спорный вопрос, не всегда это удобно, особенно при отладке, когда сознательно закомментарена часть кода и вылезают предупреждения о неиспользуемых параметрах и переменных.
firk
Ну если предупреждения о неиспользуемых переменных включены, то да. У меня то -Wall не используется и -Wunused-variable тоже.
Jef239
Неиспользуемая переменная часто бывает багом. То есть ошиблись и использовали, но не ту.
Ritorno
-Werror не включает дополнительных предупреждений, его использование это дело вкуса.
Jef239
Он не дает слинковаться при наличии предупреждений. С одной стороны, мы 95% времени именно так и делаем, то есть считаем, что предупреждение — это ошибка. С другой — при отладке иногда бывают временно закомментаренные куски кода.