Анализ утилит статического анализа C++ кода

    Анализ следующих утилит:Все необходимое можно найти пройдя по ссылкам, а мы сразу перейдем к делу.

    Тест 1:

    int main()
    {
    	vector<int> v;
    	v.reserve(2);
    	assert(v.capacity() == 2);
    	v[0];
    	v[0] = 1;
    	v[1] = 2;
    	cout << v[0] << endl;
    	v.reserve(100);
    	cout << v[0] << endl;
    	return 0;
    }
    

    Внимательно изучите данный код, сколько проблем можете выявить вы?
    Разумеется все нужные заголовки инклудированы(iostream, vector, cassert).
    До того как мы обсудим версии профессионалов на счет этого кода, произведем анализ известных утилит для статического анализа исходников. Далее дан результат выше указанных утилит.

    RATS: ничего
    Cppcheck: ничего
    Graudit: ничего
    g++(с флагом -Wall): ничего

    Может и на первый взгляд все не так уж плохо или вообще нет ничего плохого? Не говорите это при Саттере (между прочим пример взят из его книги «Новые сложные задачи на С++. 40 новых головоломных примеров с решениями»).
    На первой строке в теле функции main() мы объявляем вектор v состоящий из целых чисел. Потом резервируем место для 2 элементов. Прежде чем перейдем к следующей строке, взглянем на стандарт С++. Что она говорит нам о методе vector::reserve? (warning: вольный перевод)
    "
    void reserve(size_type n);
    Сообщает о планированном изменении в размерах, возможно соответственное управление распределением памяти. После reserve(), capacity()(емкость) больше или равна аргументу переданному reserve() в случае перераспределения памяти. В противном случае равна прежнему значению capacity(). Перераспределение памяти проиходит тогда и только тогда, когда текущее значение capacity() меньше чем значение аргумента reserve(). Она не меняет размер контейнера и требует в худшем случае линейного времени выполнения. При значении аргумента больше чем max_size(), генерирует исключение long_error. В случае перераспределения делает недействительными все ссылки, указатели и итераторы, ссылающиеся на элементы контейнера."

    Пока этого достаточно, пойдем дальше. Строка assert(v.capacity() == 2); в лучшем случае плохая, во первых reserve() гарантирует увелечение емкости и в этом случае assert просто лишняя строка кода (которые так любят вставлять «программисты», которые слепо верят ассерту после каждой операции и считают это профессиональным тоном). Во-вторых, как мы узнали из самого стандарта, reserve() может увеличить емкость контейнера больше, чем заданный аргумент, так что Саттер рекомендует assert(v.capacity() >= 2). Рекомендую вообще убрать ее.
    Дальше — хуже. «Оператор vector<T>::operator[] может, но не обязан выполнять проверку диапазона. В стандарте об этом ничего не сказано, так что разработчик стандартной библиотеки имеет полное право как добавить такую проверку, так и обойтись без нее.» Саттер. Всему причина эффективность, проверка занимает некоторое время, многочисленные такие проверки потребуют значительную часть времени работы программы. Но это не значит, что твоя эффективная программа лучше надежной программы Васи. Побробуй замени строчку v[0]; на v.at(0); прога вылетит с исключением. Что и в большинстве случаев нужно надежному коду. vector<T>::at выполняет проверку диапазона значения индекса. Строгая рекомендация использовать этот метод в таких, и не только, ситуациях.

    Строки v[0] = 1; v[1] = 2; вполне работают, убедится можно откомпилировав пример. Но надо помнить разницу между size/resize и reserve/capacity. resize изменяет количество элементов в контейнере на указанное значение аргумента путем добавления или удаления их из конца контейнера. О методе reserve мы узнали из стандарта, добавим что он увеличивает размер внутреннего буфера, чтобы он был способен вместить указанное количество элементов (как минимум). "Мы можем безопасно использовать operator[](или метод at) только для изменения элементов, которые реально содержатся в контейнере, т.е. реально учтены в size." Понятно? Так и утилитам статического анализа тоже не понятно. Но ведь проблема значительная. Саттер и многие другие профи пишат в своих книгах о тонких моментах в кодинге на любимом С++. На первый взгляд может показаться, что умение решать проблемы и знание синтаксиса языка более чем достаточно для разработки. Но мы то с тобой знаем, что сами себя обманываем, сколько из вас вставляли cout-ы на каждой строке кода, чтобы выявить проблему? Сколько из вас не мог терпеть отладку, которую выполнял уже третий день? Так вот, есть люди, которым не присущи такие вещи, в их число входят люди, владеющие профессиональными навыками программирования, которые не ленятся идти дальше и подряд изучают книги таких гуру как Саттер и Майерс (и многие другие).

    И еще, в примере 1 нас ждет последняя логическая ошибка, как вы думаете что выводит cout << v[0]; после reserve(100)? Правильно, в лучшем случае ноль! Заметьте, что до резервирования мы дали v[0] ненулевое значение. «Проблема» опять в функции резервирования.
    Так в чем дело в общем случае? В том, что мне нужна утилита, которая подскажет, что в случае v.reserve(2); v[0] = 1; лучше использовать v.resize(2); v[0] = 1; или v.reserve(2); v.push_back(1)! А рассмотренные нами утилиты мило промолчали. Еще мне нужна утилита, проверяющая неправеренные границы, и в случае v[0]; посоветовать использовать v.at(0). А наши рассмотренные утилиты опять мило промолчали. Отсюда мораль, в тонких ситуациях не полагайтесь на такие утилиты, как RATS, Cppcheck, и тем более на Graudit. Даже на компилятор с включенным высшим уровнем предупреждений иногда следует смотреть глазами кодера-параноика.
    Ладно, утилиты ведь не зря занимают место на ваших винтах, и не зря на нем трудились разработчики.

    Тест 2:

    Пойдем дальше, увеличим дозу ошибок в коде. Рассмотрим тот же код, с новой функцией prettyFormat (пример опять из Саттера). Она получает число и буфер, потом вставляет это число в полученный буфер при помощи sprintf.
    void prettyFormat(int i, char* buf)
    {
        sprintf(buf, "%4d", i);
    }
    
    int main()
    {
        vector<int> v;
        v.reserve(2);
        //....
        char buf[5];
        prettyFormat(10, buf);
        return 0;
    }
    

    Как видно, добавили prettyFormat, и строку char buf[10]. Хотя код из предыдущего примера нам больше не понадобиться, но пусть остаеться, может одна из утилит что-то увидит? Вот результат:

    RATS: на строке char buf[5]; "High: fixed size local buffer. Extra care should be taken to ensure that character arrays that are allocated on the stack are used safely. They are prime targets for buffer overflow attacks." Советует быть внимательным при использовании символьных массивов, типо они беспомощны при атаках на переполнение буфера.
    Cppcheck: ничего
    Graudit: ничего
    g++(с опцией -Wall): ничего

    О переполнении буфера сказано очень много, так что будем кратки и только покритикуем. У меня такое впечатление, что утилиты для статического анализа предназначены только для нахождения возможных случаев переполнения буфера. Впрочем, несколько из них молчат даже в таких случаях (как Cppcheck или Graudit). Компилятора не виню, он не обешал мне, что найдет проблемы, а вот утилиты специально предназначенные для таких вещей в большинстве случаев мило молчат. В таких ситуациях нам нужно получить от утилит предупреждения и рекомендации использовать альтернативы sprintf (ну и для других возможных функции). Об альтернативных ситуациях следует прочитать в «Саттере» (речь идет о std::stringstream, std::strstream, boost::lexical_cast, впрочем последняя основана на std::stringstream, и наконец snprintf, позволячщая задать размер буфера, обеспечив тем самым защиту от переполнения буфера).

    Тест 3:

    Добавим в функцию prettyFormat неинициализированный указатель.
    int* prettyFormat(int i, char* buf)
    {
        sprintf(buf, "%4d", i);
        int* a;
        return a;
    }
    
    int main()
    { 
        ...
    

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

    RATS: тоже, что и в предыдущем тесте(речь идет о строке char buf[5];)
    Cppcheck: на строке int* a; выдает (error) Uninitialized variable: a.
    Прекрасно, но…
    g++(с опцией -Wall): In function ‘int* prettyFormat(int, char*)’:
    warning: ‘a’ is used uninitialized in this function.

    Что-то знакомое не правда ли? Тоже самое предупреждение, которое наконец дал Cppcheck.
    Graudit вообще молчит.

    Тест 4:

    Добавим юмора, изменим функцию prettyFormat так:
    int* prettyFormat(int i, char* buf)
    {
        sprintf(buf, "%4d", i);
        int* a;
        fopen("filename", "r");
        char buf2[5];
        strcpy(buf2, buf);
        return a;
    }
    

    Добавились fopen, которая открывает не знаю что, добавился локальный буфер buf2, в которую с помощью strcpy копируется buf, получаемый как аргумент. Посмотрим, кто сколько даст.

    RATS: Для строк char buf2[5]; char buf[10] то же предупреждение про внимательность использования символьных буферов фиксированного размера.
    И наконец: «High: strcpy
    Check to be sure that argument 2 passed to this function call will not copy
    more data than can be handled, resulting in a buffer overflow.»

    Проверьте, не скопирует ли второй аргумент больше данных, чем может содержать buf2.
    Неплохо, во многих ситуациях бы помог. Пойдем дальше.
    Cppcheck: (error) Unitialized variable: a.
    Что-то знакомое, то же что и в предыдущем примере, а жаль.
    g++(с опцией -Wall): То же, что и в предыдущем тесте.

    С задачей более-менее справился RATS, остальных — расстрелять! Почему все так плохо? Во-первых, «писавшие» эти утилиты ребята сами толком не гуру С++, многие больше всего любят просто публиковать программу и все. Во-вторых, многие тонкие ошибки сложно обнаружить, и с ними справляются в основном коммерческие утилиты, хотя сам еще не пробовал. В-третьих, читай книги, журналы, в частности Хабр и не будешь нуждатся в утилитах подобных этим.

    Спасибо за внимание!

    P. S. Хотел опубликовать в блог «Ревизия кода», кармы не хватает. Нет, не то, чтобы хочу карму таким способом увеличить, так на всякий случай, чтоб толпа не заорала — «а почему не там-сям-...».

    P. P. S. Огромное спасибо Хабрасообществу — так держать!
    Share post

    Similar posts

    Comments 34

      +21
      Вы не могли бы связаться с разработчикам хорошо пропиариной на хабре PVS-studio и попросить у них прогнать эти же тесты?) Было бы интересно посмотреть действительно ли коммерческие аналоги лучше)
        +3
        Отличная идея! Если не ошибаюсь у них есть триальная версия, тогда мне самому нужно будет тесты гонять)
          +21
          Полтора месяца назад PVS не находил ни разыменования нулевых указателей, ни утечек памяти, ни непарных new[]-delete[], тогда как CppCheck с этим успешно справлялся (тема на rsdn).

          Впрочем, не удивлюсь, если сейчас разработчики PVS-Studio быстро добавят фильтр на вышеприведённые куски кода и через пару дней выступят с разгромной статьёй =)
          –4
          С++…
            –1
            … но альтернативы нет :)
              +6
              Да я ценю С++, но в таких сжатых кусках кода столько потенциальных багов, что волосы на одном месте шевелиться начинают, честное слово!
            +4
            «Сферический анализатор результатов анализа утилит статического анализа кода позитронного анализатора»…
              +1
              А как так получилось, что у вас GCC не ругается на неиспользованный результат fopen? Может это вообще не от компилятора зависит?

              warning: ignoring return value of ‘FILE* fopen(const char*, const char*)’, declared with attribute warn_unused_result

              www.ideone.com/eBflq
                0
                Интересно, и правда не ругается, «копну поглубже».
                  0
                  Это зависит от заголовочных файлов: объявлена ли функция с атрибутом warn_unused_result или нет. Можете этот атрибут даже к своим функциям применять и GCC будет ругаться.
                    0
                    Это-то понятно. Вопрос в том, почему эти атрибуты отсутствуют в заголовочных файлах стандартной библиотеки той же Ubuntu. Это же больше сотни функций (вот примерный список). Под подозрением eglibc, облегчённая версия glibc, которая используется в Ubuntu. В настоящей glibc __wur присутствует с 2006 года и никто его не трогал [1].
                      0
                      В Debian Unstable eglibc 2.13 и __wur для fopen() есть.
                        0
                        Ха-ха, Ubuntu прекрасна в своём роде. Уже собрался идти в багзиллу жаловаться на потерянные __wur, но тут наткнулся на wiki.ubuntu.com/CompilerFlags#A-D_FORTIFY_SOURCE.3D2

                        Таким образом, за проверку использования возвращаемых значений отвечает флаг оптимизации -O2, с чем я не могу поспорить.
                  +1
                  Добавте С++ в заголовок, пожалуйста, а не в теги.
                    +2
                    Apple clang version 3.0 (tags/Apple/clang-211.9)
                    clang++ --analyze

                    1. ничего
                    2. ничего
                    3. test.cpp:11:5: warning: Undefined or garbage value returned to caller
                    return a;

                    4. test.cpp:14:5: warning: Undefined or garbage value returned to caller
                    return a;


                      +1
                      Ну это не удивительно, они пока еще obj-c только пилят, а «support for analyzing C++ and Objective-C++ files is currently extremely limited»
                      0
                      sprintf(buf, "%4d", i);
                      а здесь где баг?
                      4 символа + завершающий ноль влазят в 5символьный буфер, не?
                        0
                        "%4d" — это минимум 4 символа. Число больше 9999 займёт больше места.
                          0
                          да, действительно. смутило что так чётко укладывалось в размер буфера.
                        +7
                        Если уж включать в этот список GCC, то стоит к его ключам добавить -Wextra и -pedantic.
                          +1
                          >> В том, что мне нужна утилита, которая подскажет, что в случае v.reserve(2); v[0] = 1; лучше использовать v.resize(2); v[0] = 1; или v.reserve(2); v.push_back(1)! А рассмотренные нами утилиты мило промолчали. Еще мне нужна утилита, проверяющая неправеренные границы, и в случае v[0]; посоветовать использовать v.at(0).

                          Полагаю, что по этим диагностикам количество false positive будет зашкаливающим. Поэтому и не делают. То есть это скорее всего диагностика, которая будет делаться под заказ для конкретного заказчика, которому она нужна.
                            0
                            Согласен, но было бы хорошо, если бы были соответствующие опции, то есть «уровни» выдачи предупреждений.
                              0
                              В сами знаете каком анализаторе ;) есть уровни.
                                0
                                Тот, чье имя нельзя произнести) Спасибо, этот анализатор — «следующая жертва».
                            +1
                            Также надо понимать, что скриптовый graudit не является полноценным инструментом статического анализа кода по причинам, изложенным в статье Статический анализ и регулярные выражения.
                              +1
                              а какая версия g++ использовалась?
                              –2
                              Я считаю, что все статические анализаторы для C++ просто бесполезная трата времени и иллюзия качества кода. Было есть и будет, что качество C++ кода определяется образованием, опытом, талантом разработчика. Эти примеры — ни о чем не говорят, они надуманы. Более того реальные C++ проблемы вообще даже человеку отследить трудно статически. И прежде всего, виновато в этом ручное управление временем жизни объектов — кто-то использует указатель на объект, его грохают в другом месте, в первом обращаются и привет. Overflow неплохо отлавливают сейчас — buffer security check, проверка при free (это про Visual С++), а еще инструменты как Microsoft Application Verifier. Проблема в том, что для статического определения корректности надо знать СЕМАНИТКУ метода, а определение этого в общем случае в C++ невозможно.
                                +1
                                Немного модифицирую текст:

                                Я считаю, что все инструменты проверки орфографии для русского языка просто бесполезная трата времени и иллюзия качества текста. Было есть и будет, что качество текста определяется образованием, опытом, талантом автора. Эти примеры с ошибками — ни о чем не говорят, они надуманы.

                                И вроде всё правильно написано. Но только оторвано от практики. Проверка орфографии в Word не призвана заменить преподавание русского языка в школе. Статически анализ не призван отменить вдумчивое программирование. Но это инструменты, которые могут помочь. Вы думаете, когда Пелевин или Лукьяненко работает над книгой, они ошибки и опечатки в тексте не делают? Взяв первого попавшегося дворника, сложно ожидать от него получить роман. И тут проверка орфографии вообще мимо. Но если проверка орфографии помогает мастеру создать роман с меньшим количеством вычитываний текста – это замечательно.
                                  –1
                                  Неверная аналогия. В корне. ОРФОГРАФИЮ проверяет компилятор. А вот статический анализатор пытается проверить СМЫСЛ (семантику текста). То есть как бы все по правилам написано, но смысла нет или предложение не верно. Ведь предложение «Я вижу летучего зайца» безупречно орфографически и построено по правилам, но лишено смысла.

                                  Проверить орфографию — не проблема. А проверить осмысленность кода (верифицировать) что вы пытаетесь делать в своей PVS — невыполнимая задача. Нужно сначала иметь язык, верификация которого будет проще на порядки чем в C++.

                                  Я вам могу привести осмысленный код, который PVS считает неверным.
                                    +1
                                    Не то, чтоб задача невыполнима, а то, что сложна, и от этого не следует, что такой утилиты не следует выпускать. Лично я хочу и считаю нужным использовать статический анализатор. Во-первых, насколько бы код не был хорош, всегда появляются ошибки из-за невнимательности. Во-вторых, статический анализатор — это как бы коллега-ревьюер, на всякий случай проверяет ваш код.
                                    +2
                                    Вы делаете хорошую работу. Любой инструмент, увеличивающий вероятность отловить дурные ошибки, нужен и полезен.
                                  0
                                  Я вижу смысл делать тул, который по функции, анализируя ее, будет пытаться выдавать набор Unit Test-ов и тестовых данных для него, пытаясь скажем, сделать максимальным покрытие ветвлений и проверять то что ей кажется странным набором тестов.
                                  Более сложный вариант — как препроцессор, который бы как-то модифицировал код, объектник или что-то еще, вставляя проверки в разные подозрительные места. Это как автоматические ассерты.

                                Only users with full accounts can post comments. Log in, please.