Я прохожу стажировку в компании, где разрабатывается статический анализатор кода PVS-Studio. Первым заданием новичку здесь является проверка проекта и написание про это заметки. Эта статья расскажет о проверке Git. Когда я узнал, что Git еще не проверен PVS-Studio, я очень обрадовался предоставленной возможности. Все дело в популярности проекта: чем больше людей использует проект, тем меньше ошибок прячется в нем. Git невероятно популярен, поэтому мне было интересно, сможет ли PVS-Studio найти что-то. Хочу предупредить заранее, что для меня это является неким экспериментом. Так как у меня практически отсутствует опыт работы с анализатором, равно как и опыт написания статей, прошу отнестись к ней снисходительно.


 
Введение
Git - пожалуй, одна из самых популярных распределённых систем управления версиями. Проект был создан Линусом Торвальдсом для управления разработкой ядра Linux, первая версия выпущена 7 апреля 2005 года. Ядром Git является набор утилит командной строки с параметрами. Система поддерживает быстрое разделение, слияния версий, инструменты для визуализации и навигации по истории разработки. Имеет преимущество перед SVN в виде локального репозитория и более полной реализацией ветвей.

Сборка и проверка
Сборка проводилась с помощью утилиты make под MinGW. Git - маленький проект, который имеет крайне небольшое количество зависимостей, поэтому процесс сборки довольно легок и не доставляет проблем. Еще более легким оказался процесс проверки с помощью Standalone. Я указал папку с исходными файлами, запустил анализатор и начал сборку проекта. По окончанию сборки нажал кнопку Stop monitoring и через 5 минут получил набор предупреждений.

Общие предупреждения
Предупреждение PVS-Studio: V595 The 'tree' pointer was utilized before it was verified against nullptr. Check lines: 134, 136. revision.c 134

В этой функции переданный указатель может быть равен нулю, об этом нам говорит проверка if (!tree). Но перед проверкой "tree" используется в выражении &tree->object. Компилятор имеет полное право удалить проверку, так как он видит, что мы уже использовали указатель, а значит он не может быть нулевым. Такой код приводит к неопределенному поведению. Последующее использование указателя obj в таком случае может принести много неприятных сюрпризов. На тему этой ошибки уже было много споров и все точки над i были давно расставлены в статье "Разыменование указателя приводит к неопределенному поведению". Всего анализатор нашел более 20 использований указателей до их проверки, что довольно много для такого маленького проекта как Git.

Предупреждение PVS-Studio: V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'str' is lost. Consider assigning realloc() to a temporary pointer. syslog.c 46

Это предупреждение указывает на возможную утечку памяти. Заметим, что указатель "str" уже содержит адрес выделенной памяти. Это значит, что если функция "realloc" не сможет выделить достаточное количество памяти, то она вернет NULL, перезаписав предыдущее значение "str". Во избежание таких ситуаций следует сохранять адрес во временный указатель перед использованием "realloc", чтобы в случае неудачи не потерять память по старому указателю. В остальном коде "realloc" используется правильно.

Предупреждение PVS-Studio: V560 A part of conditional expression is always true: !keep_name. index-pack.c 1713

Аналогично предыдущему предупреждению, здесь условие if содержит лишнюю проверку. Ее причиной стал указатель, который инициализирован через NULL и не изменяется до блока if. Возможно, причиной подобной проверки послужил рефакторинг, но как бы там ни было теперь указатель "keep_name" ни коим образом не влияет на условие if и слегка усложняет чтение кода.

Предупреждение PVS-Studio: V562 It's odd to compare 0 or 1 with a value of 0. versioncmp.c 108

Еще одна проверка из разряда "мелочь, а неприятно". В макросе результат побитовой операции "И" уже проверяется на отличие от нуля. Вероятно, программист не заглянул в макрос, и не был уверен, что истина кодируется именно числом 1. Поэтому появилась лишняя проверка. К тому же написание собственного варианта стандартной функции "isdigit" кажется довольно странным решением. Больше подобных предупреждений можно найти здесь:[LIST]
[*]V562 It's odd to compare 0 or 1 with a value of 0. versioncmp.c 117
[*]V562 It's odd to compare 0 or 1 with a value of 0. versioncmp.c 127

Предупреждение PVS-Studio: V654 The condition of loop is always false. sha1_file.c 693

А это просто странная функция с пустым циклом. Сперва в условии while вычитают переменную "pack_mapped" из "cur", давая в результате ноль из-за их равенства. Затем переменную типа size_t, которая, напомню, является беззнаковой, сравнивают с результатом разности. Результатом, разумеется, всегда будет true. Необходимость использования всех трех переменных здесь явно под сомнением.
В статьях, рассказывающих про проверку различных проектов так редко используются оптимизационные предупреждения, что, возможно, читатель не знал или забыл про такую категорию в анализаторе PVS-Studio. Напомню, что в анализаторе есть 3 категории предупреждений (не путать с 3 уровнями): General analysis, optimization analysis и 64-bit analysis. 64-битная диагностика в данном проекте нас не интересует, а основные предупреждения общего анализа мы рассмотрели выше, поэтому предлагаю взглянуть на оптимизационные предупреждения.

Оптимизационные предупреждения
Предупреждение PVS-Studio: V802 On 32-bit platform, structure size can be reduced from 56 to 48 bytes by rearranging the fields according to their sizes in decreasing order. ewok.h 144

Переменные в структуре располагаются в памяти в порядке их объявления. Они могут начинаться только с адресов, кратных их размеру. В данной структуре на 32-битной платформе переменная pointer начинается с адреса, кратного 4. Она располагается с 8 по 11 байт. Следующая переменная compressed размером в 8 байт не может расположиться сразу за ней, поэтому 4 байта после переменной pointer пропускается, и "compressed" начинается с 16 байта. Здесь и теряются 4 байта. По такому принципу размер "ewah_iterator" становится равным 52. Еще 4 байта теряются из-за того, что структура выравнивается в памяти по размеру своего максимального элемента. Потеря восьми байт кажется не такой большой, пока вы не начнете использовать массив структур. Чтобы не допустить этого, всегда следует располагать переменные в порядке уменьшения их размера, если это не сильно повлияет на читаемость кода. Данный прием называется упаковкой структур, подробней про него можно почитать здесь, а про выравнивание данных в целом - тут. Всего анализатор нашел 5 структур, для которых можно провести подобную оптимизацию.

Предупреждение PVS-Studio: V814 Decreased performance. The 'strlen' function was called multiple times inside the body of a loop. config.c 2273
Предупреждение PVS-Studio: V805 Decreased performance. It is inefficient to identify an empty string by using 'strlen(str) > 0' construct. A more efficient way is to check: str[0] != '\0'. config.c 2284

В этой функции одновременно две причины падения производительности. Во-первых, функция strlen вызывается в цикле с одним и тем же постоянным параметром. Значение этой функции следует вычислить до цикла и затем использовать результат. Во-вторых, здесь для проверки строки на пустоту используется вызов все той же функции strlen вместо ручной проверки первого элемента. Оптимизированный код должен выглядеть так:


При написании кода программист в первую очередь стремится сделать его рабочим и не всегда задумывается или забывает об оптимизации. И несмотря на то, что значительную часть оптимизаций производит компилятор, ему не всегда понятно, чего хотел добиться программист. Анализатор смотрит на код с более высокого уровня и способен помочь в оптимизации таких мест, в которых компилятор бессилен. 
Что касается предыдущих предупреждений, анализатор нашел 20 вызовов функции strlen с неизменным параметром в циклах и 3 неэффективные проверки строки.

Предупреждение PVS-Studio: V809 Verifying that a pointer value is not NULL is not required. The 'if (repo_config)' check can be removed. config.c 1258

И напоследок снова лишняя проверка. Функция free умеет обрабатывать нулевые указатели на всех современных платформах, поэтому нет необходимости делать проверку перед ее использованием.

Заключение
Подводя итог, нужно сказать, что анализатор справился со своей задачей. Отсутствие сколь ни будь серьезных ошибок объясняется огромной популярностью проекта. Сегодня каждый разработчик если и не использует Git, то точно наслышан о нем. Хочется похвалить и разработчиков Git за высокое качество кода. Возможно, они уже пользовались каким-то статическим анализатором. И хотя я не нашел упоминаний об этом, это еще не доказывает обратное. 
Так или иначе в любом проекте можно найти ошибки. Возможно, они прячутся в вашем прямо сейчас.
Предлагаю скачать и попробовать PVS-Studio на своём проекте:
http://www.viva64.com/ru/pvs-studio-download/

Это сообщение отредактировал(а) cppclimber - 22.8.2015, 15:58

Спасибо за статью, удачи в стажировке)))


Вот не очевидно...