CodeNet / Языки программирования / C / C++ / Microsoft Visual C++ / Среда разработки
CodeNet / Языки программирования / C / C++ / Язык C/C++
Стажировка: PVS-Studio и Git
Автор: Игорь Штукарев
Дата: 07.08.15
Я прохожу стажировку в компании, где разрабатывается статический анализатор кода 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
{
struct object *obj = &tree->object;
if (!tree)
return;
....
}
В этой функции переданный указатель может быть равен нулю, об этом нам говорит проверка 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
{
char *str;
int str_len;
....
str = malloc(str_len + 1);
....
str = realloc(str, ++str_len + 1);
if (!str)
{
warning("realloc failed: '%s'", strerror(errno));
return;
}
....
}
Это предупреждение указывает на возможную утечку памяти. Заметим, что указатель "str" уже содержит адрес выделенной памяти. Это значит, что если функция "realloc" не сможет выделить достаточное количество памяти, то она вернет NULL, перезаписав предыдущее значение "str". Во избежание таких ситуаций следует сохранять адрес во временный указатель перед использованием "realloc", чтобы в случае неудачи не потерять память по старому указателю. В остальном коде "realloc" используется правильно.
Предупреждение PVS-Studio: V560 A part of conditional expression is always true: !keep_name. index-pack.c 1713
const char *prefix)
{
const char *keep_name = NULL;
....
if (!keep_name && ....)
....
}
Аналогично предыдущему предупреждению, здесь условие 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
#define GIT_DIGIT 0x02
#define isdigit(x) sane_istest(x,GIT_DIGIT)
#define sane_istest(x,mask) \
((sane_ctype[(unsigned char)(x)] & (mask)) != 0)
int versioncmp(....)
{
int state;
unsigned char c1;
....
state = S_N + ((c1 == '0') + (isdigit(c1) != 0));
....
}
Еще одна проверка из разряда "мелочь, а неприятно". В макросе результат побитовой операции "И" уже проверяется на отличие от нуля. Вероятно, программист не заглянул в макрос, и не был уверен, что истина кодируется именно числом 1. Поэтому появилась лишняя проверка. К тому же написание собственного варианта стандартной функции "isdigit" кажется довольно странным решением. Больше подобных предупреждений можно найти здесь:
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: V575 The null pointer is passed into 'memset' function. Inspect the first argument. merge.c 742
Сразу стоит отметить, что между инициализацией указателя и вызовом функции "memset" всего одна строчка, и никаких манипуляций с указателем она не содержит. NULL является макросом, который раскрывается в (void *) 0 и при приравнивании заполняет весь адрес нулями. Зачем программисту потребовалось второй раз обнулять указатель, да еще и через вызов "memset", остается загадкой. Предупреждение 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: V804 Decreased performance. The 'strlen' function is called twice in the specified expression to calculate length of the same string. transport-helper.c 77 Участок кода, в котором не только происходит два вызова функции strlen с одним и тем же параметром, но и производится сравнение их возвращаемых значений. Очевидно, что третьим параметром функции write_in_full всегда будет 0, так как возвращаемые значения одинаковы. Непонятно, что могло вызвать такую ошибку и что на самом деле программист хотел написать здесь. Анализатором было найдено 10 мест, в которых "strlen" использовалась дважды с одним и тем же параметром. Предупреждение 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/
{
struct strategy *list = NULL;
....
memset(&list, 0, sizeof(list));
....
}
{
size_t cur = pack_mapped;
while (need >= (cur - pack_mapped)
&& unuse_one_window(NULL))
; /* nothing */
}Оптимизационные предупреждения
typedef uint64_t eword_t;
struct ewah_iterator
{
const eword_t *buffer;
size_t buffer_size;
size_t pointer;
eword_t compressed, literals;
eword_t rl, lw;
int b;
};
{
....
if (write_in_full(fd, str, strlen(str)) != strlen(str))
....
}
{
int baselen;
....
} store;
int git_config_rename_section_in_file(const char *new_name,....)
{
while (....)
{
char *output = buf;
....
store.baselen = strlen(new_name);
....
if (strlen(output) > 0)
....
}
}
{
size_t new_name_length = strlen(new_name);
while (....)
{
char *output = buf;
....
store.baselen = new_name_length;
....
if (str[0] != '\0')
....
}
}
{
char *repo_config = NULL;
....
if (repo_config)
free(repo_config);
....
}Заключение
Оставить комментарий
Комментарии
Что-то тут на форуме старые статьи, более новые есть?
А то я ищу место, где бы пообщаться с ИТ-специалистами.
