Любите статический анализ кода

    PVS-Studio - супергерой

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

    Работа с инструментами статического анализа кода требует внимательности. Очень часто код, на который указал анализатор, кажется корректным. Сразу хочется посчитать предупреждение ложным срабатыванием и пойти дальше. Даже я, один из разработчиков PVS-Studio, попадаю в эту ловушку и не вижу ошибку. На днях я открыл 2 тикета в нашем багтрекере, касающиеся диагностики V614, которая ищет использование неинициализированных переменных и массивов.

    В обоих случаях я подумал, что анализатор неправ и в нём надо что-то исправить. Первый случай:

    Код, опечатка


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

    Буфер caption остаётся неинициализированным. Посмотрите выше, там обе строки загружаются в буфер text. Опечатка. Я не смог её увидеть.

    А вот еще более эпичный случай:

    на первый взгляд всё хорошо


    Анализатор PVS-Studio говорит, что используется неинициализированный буфер buf. Бред какой-то. И я отписываю этот случай в багтрекере как баг, который надо обязательно поправить. Ведь очевидно, что функция sprintf инициализирует буфер и всё в этом коде хорошо.

    Нифига! Вновь прав анализатор PVS-Studio, а не я. Тот случай, когда творение превзошло создателя. :)

    Очень нехороший программист в одном из заголовочных файлов написал вот это:

    эпичный #define


    Видите, sprinf раскрывается в std::printf. Да, да, в этой программе sprintf это тоже самое что printf.

    Ужас то какой. Получается, что функция printf использует неинициализированный буфер buf как управляющую строку.

    Любите и используйте статические анализаторы кода! Они сэкономят вам нервы и время.


    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Appreciate Static Code Analysis!
    PVS-Studio 244,45
    Ищем ошибки в C, C++ и C# на Windows и Linux
    Поделиться публикацией
    Комментарии 64
    • +1
      А мы и любим. Подскажите, кстати, какой-нибудь для чистого Си в GNU/Linux бесплатный, лучше open source.
        • 0
          А чтобы работал standalone и не требовал проекта в Visual Studio?
          • +2
            … сам исправлял ошибки, коммитил код, общался с пользователями, учавствовал в конференциях от имени разработчиков…
          • 0
            Такая возможность давно существует в PVS-Studio. Есть несколько способов проверки проектов. Проще всего начать с системы мониторинга компиляции: windows, linux (см. «Любой проект»).
          • +5
            Cppcheck
            • +2
              спасибо, то, что надо: простое, рабочее, живое!
              • 0
                лучше gcc -wall -wexta. Намного выше процент полезных предупреждений. у cppcheck сплошняком бесполезные. Кстати, то, что находит gcc, cppcheck не ищет
                • +2
                  clang еще лучше — но pvs еще лучшее :D
                  • +2
                    вот не уверен: всегда собираю с -Wall -Wextra -Wpedantic, проверил кое-что через cppheck и уже нашёл кое-какую «пищу для размышлений»
                    • –1
                      А теперь попробуйте PVS-Studio. :)
                      • –2
                        Попробовали на проекте в полторы тысячи строк.

                        1. gcc — 33 предупреждения, ценных — больше половины, включая особо ценные вроде operation on ‘d’ may be undefined [-Wsequence-point]
                          ELM_Answer[d-12] = AnswerBuffer[0][d++];;
                        2. cppcheck — 36 предупреждений, два The scope of the variable 'b' can be reduced, остальные — Prefer prefix ++/-- operators for non-primitive types, причем непримитивными он обозвал то ли указатели, то ли enum, то ли вообще int.
                        3. pvs выдал 6 предупреждений двух типов 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 — золотая серединка. Кстати, лучше всего именно в инкрементальном режиме, то есть устранять все выданные предупреждения.
                        • +3
                          То есть в проекте из 1500 строк (6 файлов?) вы сам не знаете, то ли у вас указатели, то ли enum, то ли вообще int, но виноват статический анализатор?
                          • –4
                            Восхищен вашей память. Помнить год все ложные срабатывания статанализатора в проектах всех сотрудников — это очень сильно. Завидую вам сильно.

                            P.S. Git подсказывает, что там были постфиксные инкременты у enum. В любом случае — диагностика ложная, пока мы не имеем дело с operator ++.
                          • +1
                            Попробовали на проекте в полторы тысячи строк.
                            Для проекта в полторы тысячи строк плотность ошибок составляет 0-25 ошибок на 1000 строк. Так что ставить эксперименты на столь малых проектах имеет мало практического смысла. В них вообще может не быть ошибок.

                            Так что pvs полезен, но безумно дорог,
                            Это заблуждение, которое продолжает тиражироваться. Компании приходят и приобретают инструмент и только некоторые обсуждают скидки. Есть конечно те, кто говорит, что дорого. Но им просто не нужен инструмент и им был бы дорог и CppCat за 250$. Поэтому про это нет смысла говорить.

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

                            • –4
                              Наш тест показал, что вычистка кода с помощью gcc и cppcheck недостаточна и pvs-studio все равно находит ошибки, причем без ложных срабатываний.

                              Ну что же, будем считать, что на самом деле PVS-studio работает намного хуже того великолепного результата, что он показал в нашем тесте.

                              Что касается цены, то разумней на те же деньги (пара миллионов в год уже или больше?) нанять тестера, а то и двух.

                              PVS-Studio в рассчёте на 1 программиста дешевле, чем оплата аренды за квадратные метры в офисе, которые занимает программист

                              Цена PVS-Studio примерно равна аренде офиса на всю фирму. Не все живут в Москве и располагаются с видом на Кремль.
                              • –3
                                PVS-Studio в рассчёте на 1 программиста дешевле, чем оплата аренды за квадратные метры в офисе, которые занимает программист

                                Кстати, это хороший критерий, кому есть смысл покупать PVS-Studio. Есть офис с видом на Кремль (или Гудзон) — есть смысл покупать. Офис дешевле — нет смысла.

                                Иными словами, PVS-Studio — это как малиновый пиджак или розовый лаборджини — нечто, показывающее престижность владельца. Ну Apple заработала миллиарды на такой стратегии, так что почему бы и нет.

                                Если бы ещё дотянули до продукта… Потому как качество документации — ниже плинтуса, а время ответа на письма… Ну мы уже полгода ждем.
                        • 0
                          с другой стороны, gcc не ищет того, что находит cppcheck. к примеру незакрытые файлы и адресацию за пределами буфера. я в своё с помощью cppcheck хорошо почистил от багов один старый проект, который компилировался без ошибок и предупреждений. статический анализ ищет логические ошибки, а не формальные.
                          • +2
                            у cppcheck сплошняком бесполезные

                            За 4 года использования не увидел у Cppcheck ни одного бесполезного предупреждения.

                            Для GCC -Wall и -Wextra недостаточно, есть еще много флагов, которые нужно включать вручную.
                            • 0
                              Можно списочек?
                              • +1
                                Например -Wfloat-equal, а вообще список с каждой версией пополняется/изменяется, поэтому нужно брать документацию от используемой версии и просматривать весь список.
                                • 0
                                  Получается, что на каждую версию gcc — свои опции компиляции? Неудобно это.
                            • +1

                              Использую gcc -Wreturn-type -Wpointer-sign -Wsign-compare -Wshadow -Wpointer-arith -Wimplicit -Werror (список не сразу таким стал)


                              Недавно пришлось столкнуться с clang — там добавил -Wno-logical-op-parentheses -Wno-parentheses


                              Вообще думаю что набор актуальных (и возможно-индикаторов-бага) предупреждений у каждого свой, в зависимости от привычек и стиля написания кода.

                              • 0
                                СПАСИБО. -Wshadow, -Wpointer-arith действительно не включаются по -Wall.

                                -Werror — спорный вопрос, не всегда это удобно, особенно при отладке, когда сознательно закомментарена часть кода и вылезают предупреждения о неиспользуемых параметрах и переменных.
                                • 0

                                  Ну если предупреждения о неиспользуемых переменных включены, то да. У меня то -Wall не используется и -Wunused-variable тоже.

                                  • 0
                                    Неиспользуемая переменная часто бывает багом. То есть ошиблись и использовали, но не ту.
                                • 0
                                  -Werror не включает дополнительных предупреждений, его использование это дело вкуса.
                                  • 0
                                    Он не дает слинковаться при наличии предупреждений. С одной стороны, мы 95% времени именно так и делаем, то есть считаем, что предупреждение — это ошибка. С другой — при отладке иногда бывают временно закомментаренные куски кода.
                        • +15
                          #define suchar
                          звучит как угроза
                          • +2

                            Жесть какая (я про второй случай).
                            Интересно, реально ли разработать специальную диагностику для всяких "#define true false"?

                            • +2
                              Скорее всего, программа с "#define true false" просто не скомпилируется. Или программа станет столь неработоспособна, что невозможно будет не обратить внимание на это.

                              В любом случае, "#define true false" это какое-то вредительство и не стоит о нём специально думать. Если уж кто-то захочет навредить, он придумает как это сделать, чтобы это было и не заметно и скрылось от анализаторов.

                              На практике больший интерес представляют случаи неумышленно неправильного написания #define. Например, могут быть забыты скобки. Мы уже выявляем некоторые такие ситуации (см., например, V1003) и будем развивать анализатор в этом направлении и дальше.
                              • +1

                                Ну просто пример, который вы привели, был именно такого уровня, просто чуть подлее. Типа #define true (rand()%100000!=0).

                                • –4

                                  Можно попробовать сделать наоборот


                                       #define false true
                                  • 0
                                    Как тут не вспомнить саму статью + комментарии…
                                    habrahabr.ru/post/197266
                                  • 0
                                    Кстати, Visual C++ борется с некоторыми подобными случаями (в том числе и с приведённым define). Я писал про это заметку 5 лет назад — Прощай #define private public.
                                  • +1
                                    А анализатор выдал бы ошибку, типа «неправильный размер буфера», если бы в первом случае размер text был меньше размера caption? Например TCHAR text[128], caption[512];…
                                    • +6
                                      Посмотрел, как у нас размечена функция LoadString. К сожалению, предупреждения не будет. Доработаем.
                                      • +2

                                        Строки в стиле си это минное поле, каждый раз убеждаюсь.

                                      • 0

                                        В легаси коде


                                        //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
                                        • 0
                                          Не понятно, это похвала анализатора или критика за ложное срабатывание? :) Прошу уточнить.

                                          Проверьте, есть ли объявление функции ReadPictureName в texman.c. Скорее всего нет, а раз так, считается, что функция возвращает int. Как следствие, имеем красивую 64-битную ошибку.
                                          • 0

                                            Нет, функции из ж.cpp там вызываются из ж.c без их объявления.

                                            • 0
                                              Значит по делу ругается.
                                              • +1

                                                И как это правильно исправить?
                                                Неужели, поменять char на int?! в офигении
                                                Или описать вызываю функцию явно?

                                                • 0
                                                  Объявите функцию в cpp файле где она используется. Так себе решение, но хоть ошибка исправится.
                                                  • 0

                                                    Добавил в textman.c строку:


                                                    char* ReadPictureName(const LPSTR package, const UINT8 ID, char* Extention);//it is external! in MD3_Man.cpp

                                                    Сообщение об ошибке исчезло :)
                                                    (не уверен, что сделал всё правильно правильно, так как после миграции с VC++ 6 на VC++ 2013, проект сейчас вылетает в совсем другом месте)

                                                    • 0

                                                      Правильно — сделать MD3_Man.h с этим прототипом и заинключить его в обоих исходниках.

                                                      • 0

                                                        А то, что MD3_Man написан на C++, а texman на обычном простом C, какой на это эффект окажет?


                                                        PS в комментариях к MD3_Man.h почему-то написано


                                                        //USE extern!!!!!!! just for myself
                                                        • +1

                                                          Тогда так


                                                          #ifdef __cplusplus
                                                          extern "C"
                                                          #endif
                                                          char* ReadPictureName(const LPSTR package, const UINT8 ID, char* Extention);

                                                          Что означает комментарий не знаю, это в конце концов ваш код и вам лучше надо его знать.

                                                          • 0

                                                            Вспомнил, почему добавлено


                                                            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", вряд ли бы мне пришло в голову (подозреваю, что это легаси, но, точно не уверен).

                                                  • +3
                                                    простите, а вы правильно прочитали предупреждение PVS и комментарии?
                                                    у вас нет объявления функции в файле textman.c, и С компилятор считает, что функция возвращает int параметр.
                                                    т.е. это все равно, что вы напишите
                                                    int a = 2;
                                                    char *c = a;

                                                    Вас же не смутит тут такое же предупреждение?
                                                    З.Ы. чисто теоретически интересно, за что минусуют сейчас комменты топик стартера?
                                                • +2

                                                  Эм, я не понял, а -Wimplicit компилятора вы УЖЕ проигнорировали/отключили? Если да, то в этом и есть корень вашей проблемы, а если нет, то это и правда проблема анализатора, но я в этом сомневаюсь.

                                            • +8

                                              Как же приятно, когда за такими вещами следит сам компилятор!


                                              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

                                              Хм, переменные используются одинаково а варнинг только про одну из них, посмотрю ка повнимательней.

                                            • 0
                                              Прозвучало (не здесь, а в другом месте), что второй пример выдуманный. Нет, описанные случаи в статье взяты из реальных проектов. Я не стал писать названия, так как не посчитал, что это важно. Источник: definesTypes.h
                                              • 0
                                                Вопросы по примеру
                                                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 может уронить программу.
                                                • 0
                                                  1) Я уже писал выше, что функция LoadString размечена «не на полную». Сложно аннотируя тысячи функций сразу учесть все способы их неправильного использования. Доработаем. Собственно, во многом благодаря подомным рекомендациям пользователей анализатор улучшает свои возможности.

                                                  2) Кажется это было сделано специально. Уж очень никто не проверяет результат LoadString. :) Я изучу этот вопрос.
                                              • 0
                                                А как дела с кросс-платформенным кодом? Могу я в VS проверить makefile проект для avr мк или Arduino проект?
                                                • +1
                                                  Я не понимаю вопрос. Если что-то компилируется в Windows и Linux, Вы можете это проверить, используя мониторинг компиляции (см. документацию). Если у Вас какой-то особый сложный случай, то прошу прийти в поддержку и мы пообщаемся.
                                                  • –2
                                                    Нет, не сможете. Уже пройдено
                                                    • +1
                                                      Не скажу за Windows. Но если на линуксе собирается, то проблем нет вообще.
                                                      Собрали под мониторингом, проанализировали, впихнули в сонар и наслаждайтесь.
                                                      У нас продукты наверное 4-5 компайлерами собираются — и все в один проход срабатывает.

                                                      А то что IronHead рассказывает — скорее всего силно криворукие люди были. В свое время мы даже splint под Code Composer Studio гоняли.

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

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