Доброго времени суток! Эта статья посвящена применению бесплатной версии (для свободных и открытых проектов) статического анализатора PVS-Studio. Проверять мы будем исходный код файловой системы Reiser4 и ее утилит.
Данная статья впервые была опубликована на сайте Хабрахабр. Статья публикуется в нашем блоге с разрешения автора.
Я надеюсь все, кто собрался прочитать эту статью, хотя бы краем уха, слышали о статическом анализаторе кода PVS-Studio. Если вы не из их числа, то пройдите по этой ссылке, где можете кратко прочитать про данный статический анализатор.
Также у компании разработчика есть официальный блог на Хабрахабре где часто появляются отчеты по проверки различных открытых проектов.
Немного почитать про Reiser4 можно на Вики ядра.
Начнем, пожалуй, с утилит, а конкретно с библиотеки libaal. Затем проверим утилиты reiser4progs, а проверку кода в ядре оставим напоследок.
Для начала нам необходимо поставить PVS-Studio. На официальном сайте можно найти deb и rpm пакеты, а также просто архив с программой. Устанавливаем самым удобным для нас способом.
Далее, нужно как-то воспользоваться бесплатной лицензией. Для открытых проектов необходимо в начале каждого файла с исходным кодом добавить следующие строки (в заголовочные файлы не обязательно):
// This is an open source non-commercial project. Dear PVS-Studio, please check it.
// PVS-Studio Static Code Analyzer for C, C++ and C#: http://www.viva64.com
Дабы вручную не добавлять данные строки в каждый файл, напишем небольшой скрипт на bash'е. Для этих целей используем потоковый текстовый редактор sed (команда одной строкой):
#!/usr/bin/bash
for str in $(find $1 -name '*.c'); do
sed -i -e '1 s/^/\/\/ This is an open source non-commercial project.
Dear PVS-Studio, please check it.\n\/\/ PVS-Studio Static Code
Analyzer for C, C++ and C\#: http:\/\/www.viva64.com\n\n/;' $str
doneДля удобства напишем еще один скрипт, для сборки и запуска PVS-Studio:
#!/usr/bin/bash
pvs-studio-analyzer trace -- make -j9 || exit 1
pvs-studio-analyzer analyze -o log.log -j9 || exit 1
plog-converter -a GA:1,2 -t tasklist log.log || exit 1Теперь мы готовы к анализу исходного кода. Начнем с библиотеки libaal.
libaal это библиотека абстракции структур Reiser4, используемая reiser4progs.
Лог анализатора: log1.txt
Если не считать предупреждения, связанные с повторным объявлением стандартных типов данных, то возможные проблемы у нас только в строках 68, 129 и 139 в файле src/bitops.c:
V629 Consider inspecting the 'byte_nr << 3' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type.
В 129 и 139 строке код следующего вида:
bit_t aal_find_next_set_bit(void *map, bit_t size, bit_t offset)
{
....
unsigned int byte_nr = offset >> 3;
....
unsigned int nzb = aal_find_nzb(b, bit_nr);
....
if (nzb < 8)
return (byte_nr << 3) + nzb;
....
}В данном случае ошибку легко исправить заменив объявления переменных типа unsigned int на тип bit_t.
Что касается строки 68:
bit_t aal_find_first_zero_bit(void *map, bit_t size)
{
....
unsigned char *p = map;
unsigned char *addr = map;
....
return (p - addr) << 3;
....
}то тут я теряюсь в догадках с чего это вдруг PVS считает (p-addr) 32-битным. Даже sizeof() выдает четкие 8 байт (я использую amd64).
Лог анализатора: log2.txt
А вот в reiser4progs все гораздо интереснее и в некоторых местах печальнее. Вообще, Эдуард Шишкин упомянул, что: "после того, как были написаны эти прогсы, автор сразу уволился, и с тех пор в этот код никто не заглядывал (я только пару раз фиксил fsck по просьбам). Так что весь этот урожай ошибок не удивителен". И правда, не удивительно что такие ошибки за столько лет небыли убраны.
Первая серьезная ошибка появляется в файле plugin/key/key_short/key_short_repair.c:
V616 The 'KEY_SHORT_BAND_MASK' named constant with the value of 0 is used in the bitwise operation.
errno_t key_short_check_struct(reiser4_key_t *key)
{
....
if (oid & KEY_SHORT_BAND_MASK)
key_short_set_locality(key, oid & !KEY_SHORT_BAND_MASK);
....
}KEY_SHORT_BAND_MASK это константа 0xf000000000000000ull, т.е. булева операция отрицания, в данном случае, дает ложь (в C все что не 0 это истина), т.е. по факту 0. Очевидно, что автор имел в виду побитовую операцию НЕ — ~, а не булеву.
Данная ошибка повторяется несколько раз, в нескольких разных файлах.
Далее следует plugin/hash/tea_hash/tea_hash.c с ошибками вида:
V547 Expression 'len >= 16' is always false.
Но... это не совсем ошибка, это какая-то магия, или грязный хак, если вы не верите в магию. Почему? А вот сами посудите может ли подобный код считаться понятным и очевидным без глубокого познания работы процессора, ОС и оригинальной идеи автора кода?
uint64_t tea_hash_build(unsigned char *name, uint32_t len)
{
....
while(len >= 16)
{
....
len -= 16;
....
}
....
if (len >= 12)
{
if (len >= 16)
*(int *)0 = 0;
....
}
....
}Как вам? Это не ошибка, но не понимая, что тут происходит лучше это не трогать. Попробуем разобраться в нем.
Код *(int *)0 = 0; в обычной программе приведет к SIGSEGV. Если поискать информацию относительно ядра, то в нем такой код используется для того, чтоб ядро сделало упс (oops). Вопросы на эту тему всплывали в рассылке разработчиков ядра здесь, да и сам Торвальдс упоминал об этом. Т.е. получается если каким-нибудь неведанным образом подобное присвоение исполнится в коде ядра, то будет упс. Причины проверки "невозможного" условия остаются на совести разработчика, но, как я уже упомянул выше, не понимаешь, не трогай.
Единственный момент, который нам можно спокойно разобрать, так это причину срабатывания V547. Выражение len >= 16 всегда ложно. Цикл while выполняется пока значение len больше или равно 16, а в конце цикла вычитается 16 на каждой итерации. Т.е. переменную можно представить в виде len = 16*n+m, где n,m это целые числа, а m<16. Становится очевидным, что после завершения цикла все 16*n будут вычтены, а m останется.
Остальные подобные предупреждения идут по той же схеме.
В файле plugin/sdext/sdext_plug/sdext_plug.c мы находим следующую ошибку:
V595 The 'stat' pointer was utilized before it was verified against nullptr. Check lines: 18, 21.
static void sdext_plug_info(stat_entity_t *stat)
{
....
stat->info.digest = NULL;
if (stat->plug->p.id.id != SDEXT_PSET_ID || !stat)
return;
....
}Здесь имеет место либо банальная опечатка, либо автор имел нечто другое. Проверка !stat выглядит как проверка на nullptr, но она не имеет смысла по двум причинам. Во-первых, выше уже разыменовывался указатель stat. Во-вторых, по стандарту данное выражение вычисляется слева, направо и если это действительно проверка на nullptr то ее нужно переместить в начало условия, ибо раньше в этом же условии указатель разыменовывается.
В файле plugin/item/cde40/cde40_repair.c встречается несколько срабатываний вида:
V547 Expression 'pol == 3' is always true.
static errno_t cde40_pair_offsets_check(reiser4_place_t *place,
uint32_t start_pos,
uint32_t end_pos)
{
....
if (end_offset == cde_get_offset(place, start_pos, pol) +
ENTRY_LEN_MIN(S_NAME, pol) * count)
{
return 0;
}
....
}Автор, скорее всего, имел в виду конструкцию вида A == (B + C), но по невнимательности получил (A == B) + C.
upd1. Моя ошибка, перепутал приоритет + и ==
В файле plugin/object/sym40/sym40.c встречаем ошибку — опечатку:
V593 Consider reviewing the expression of the 'A = B < C' kind. The expression is calculated as following: 'A = (B < C)'.
errno_t sym40_follow(reiser4_object_t *sym,
reiser4_key_t *from,
reiser4_key_t *key)
{
....
if ((res = sym40_read(sym, path, size) < 0))
goto error;
....
}Проблема похожа на ту, что была выше. Видим, что переменной res присвоится результат булева выражения. Очевидно, что здесь был применена "фича" C и выражение нужно переписать в виде (A = B) < C.
Очередной представитель опечаток или невнимательности. Файл libreiser4/flow.c:
V555 The expression 'end - off > 0' will work as 'end != off'.
int64_t reiser4_flow_write(reiser4_tree_t *tree, trans_hint_t *hint)
{
....
uint64_t off;
uint64_t end;
....
if (end - off > 0)
{
....
}
....
}Имеем две целочисленные переменные. Их разность ВСЕГДА больше или равна нулю, т.к., с точки зрения представления целых чисел в памяти ЭВМ, для процессора вычитание и сложение есть суть одна и та же операция (Дополнительный Код). Скорее всего условие нужно заменить на end > off.
Очередная возможная ошибка — опечатка:
V547 Expression 'insert > 0' is always true.
errno_t reiser4_flow_convert(reiser4_tree_t *tree,
conv_hint_t *hint)
{
....
for (hint->bytes = 0; insert > 0; insert -= conv)
{
....
if (insert > 0)
{
....
}
....
}
}Код в цикле, а тело цикла выполняется только при insert > 0, т.е. проверка в условии всегда истинна в этом участке кода. Имеем либо ошибку и, как следствие, имелось в виду нечто другое, либо имеет место бесполезная проверка.
V547 Expression 'ret' is always false.
static errno_t repair_node_items_check(reiser4_node_t *node,
place_func_t func,
uint8_t mode,
void *data)
{
....
if ((ret = objcall(&key, check_struct) < 0))
return ret;
if (ret)
{
....
}
....
}Видим что в предыдущем условии конструкция вида A = ( B < 0 ), а имелось в виду скорее всего (A = B) < C.
В файле librepair/semantic.c возможно присутствует очередной представитель "черной магии":
V612 An unconditional 'break' within a loop.
static reiser4_object_t *cb_object_traverse(reiser4_object_t *parent,
entry_hint_t *entry,
void *data)
{
....
while (sem->repair->mode == RM_BUILD && !attached)
{
....
break;
}
....
}В данном случае цикл while используется как оператор if, т.к. если условие истина, то тело выполнится один раз (ибо в конце стоит break), либо не выполнится в случае, когда условие ложь.
Попробуйте угадать проблема какого плана предстанет пред нами дальше?
Правильно, это опечатка — ошибка! Код продолжает выглядеть "написанным и брошенным". На этот раз ошибка в файле libmisc/profile.c:
V528 It is odd that pointer to 'char' type is compared with the '\\0' value. Probably meant: *c + 1 == '\\0'.
errno_t misc_profile_override(char *override)
{
....
char *entry, *c;
....
if (c + 1 == '\0')
{
....
}
....
}Сравнивать указатель с терминальным символом это, без сомнений, сильное решение, однако скорее всего имелось в виду *(c + 1) == '\0', т.к. вариант *c + 1 == '\0' не имеет особого смысла.
Рассмотрим пару предупреждений насчет использования fprintf(). Сами предупреждения простые, но дабы увидеть, что же в них происходит нужно перескочить по нескольким файлам.
Для начала полезем в файл libmisc/ui.c.
V618 It's dangerous to call the 'fprintf' function in such a manner, as the line being passed could contain format specification. The example of the safe code: printf("%s", str);
Видим в нем следующий код:
void misc_print_wrap(void *stream, char *text)
{
char *string, *word;
....
for (line_width = 0; (string = aal_strsep(&text, "\n")); )
{
for (; (word = aal_strsep(&string, " ")); )
{
if (line_width + aal_strlen(word) > screen_width)
{
fprintf(stream, "\n");
line_width = 0;
}
fprintf(stream, word);
....
}
....
}
}Ищем использование этой функции. Находим в том же файле:
void misc_print_banner(char *name)
{
char *banner;
....
if (!(banner = aal_calloc(255, 0)))
return;
aal_snprintf(banner, 255, BANNER);
misc_print_wrap(stderr, banner);
....
}Ищем BANNER и находим его в файле include/misc/version.h:
#define BANNER \
"Copyright (C) 2001-2005 by Hans Reiser, " \
"licensing governed by reiser4progs/COPYING."Т.е. никаких "инъекций" произойти не может.
Рассмотрим вторую подобную ошибку в файлах progs/debugfs/browse.c и progs/debugfs/print.c они используют один и тот-же код, поэтому рассмотрим только browse.c
static errno_t debugfs_reg_cat(reiser4_object_t *object)
{
....
char buff[4096];
....
read = reiser4_object_read(object, buff, sizeof(buff));
if (read <= 0)
break;
printf(buff);
....
}Ищем функцию reiser4_object_read():
int64_t reiser4_object_read(
reiser4_object_t *object, /* object entry will be read from */
void *buff, /* buffer result will be stored in */
uint64_t n) /* buffer size */
{
....
return plugcall(reiser4_psobj(object), read, object, buff, n);
}Ищем что-же делает plugcall(), а это макрос:
/* Checks if @method is implemented in @plug and calls it. */
#define plugcall(plug, method, ...) ({ \
aal_assert("Method \""#method"\" isn't implemented " \
"in "#plug"", (plug)->method != NULL); \
(plug)->method(__VA_ARGS__); \
})И в очередной раз нужно найти чем-же занимается method(). А он зависит от plug, а plug это reiser4_psobj(object):
#define reiser4_psobj(obj) \
((reiser4_object_plug_t *)(obj)->info.pset.plug[PSET_OBJ])Если еще порыться в коде, то окажется что это все тоже строки константы:
char *pset_name[PSET_STORE_LAST] = {
[PSET_OBJ] = "object",
[PSET_DIR] = "directory",
[PSET_PERM] = "permission",
[PSET_POLICY] = "formatting",
[PSET_HASH] = "hash",
[PSET_FIBRE] = "fibration",
[PSET_STAT] = "statdata",
[PSET_DIRITEM] = "diritem",
[PSET_CRYPTO] = "crypto",
[PSET_DIGEST] = "digest",
[PSET_COMPRESS] = "compress",
[PSET_CMODE] = "compressMode",
[PSET_CLUSTER] = "cluster",
[PSET_CREATE] = "create",
};И никаких инъекций не получится.
Остальные ошибки либо того же плана, что рассмотренные выше, либо те, которые в данном случае я не вижу смысла рассматривать.
Переходим непосредственно к проверке кода Reiser4 в ядре. Дабы не собирать все ядро, то модифицируем скрипт для запуска PVS, дабы происходила только сборка кода Reiser4:
#!/usr/bin/bash
pvs-studio-analyzer trace -- make SUBDIRS=fs/reiser4 -j9 || exit 1
pvs-studio-analyzer analyze -o log.log -j9 || exit 1
plog-converter -a GA:1,2 -t tasklist log.log || exit 1Таким образом у нас соберется только исходный код, находящийся в папке fs/reiser4.
Лог анализатора: log3.txt
Пропускаем ошибки и предупреждения связанные с переопределением стандартных типов в заголовках самого ядра, ибо при сборке не используется стандартные заголовки, да и код ядра нам не интересен.
Первый файл, попавшийся на нашем пути, это fs/reiser4/carry.c.
V522 Dereferencing of the null pointer 'reference' might take place. The null pointer is passed into 'add_op' function. Inspect the third argument. Check lines: 564, 703.
static carry_op *add_op(carry_level * level, /* &carry_level to add
* node to */
pool_ordering order, /* where to insert:
* at the beginning of @level;
* before @reference;
* after @reference;
* at the end of @level */
carry_op * reference /* reference node for insertion */)
{
....
result =
(carry_op *) reiser4_add_obj(&level->pool->op_pool, &level->ops,
order, &reference->header);
....
}В данном случае необходима проверка reference на NULL, т.к. дальше в коде можно встретить подобный вызов этой функции:
carry_op *node_post_carry(carry_plugin_info * info /* carry
* parameters
* passed down to node
* plugin */ ,
carry_opcode op /* opcode of operation */ ,
znode * node /* node on which this
* operation will operate */ ,
int apply_to_parent_p /* whether operation will
* operate directly on @node
* or on it parent. */ )
{
....
result = add_op(info->todo, POOLO_LAST, NULL);
....
}где add_op() явно вызывается со значением reference равным NULL и ядро сделает oops.
Следующая ошибка:
V591 Non-void function should return a value.
static cmp_t
carry_node_cmp(carry_level * level, carry_node * n1, carry_node * n2)
{
assert("nikita-2199", n1 != NULL);
assert("nikita-2200", n2 != NULL);
if (n1 == n2)
return EQUAL_TO;
while (1) {
n1 = carry_node_next(n1);
if (carry_node_end(level, n1))
return GREATER_THAN;
if (n1 == n2)
return LESS_THAN;
}
impossible("nikita-2201", "End of level reached");
}Ошибка говорит о том, что функция не void, следовательно, должна возвращать какое-то значение. Из последней строчки кода становится очевидным, что данный случай не является ошибкой, т.к. случай когда while перестанет выполнятся является ошибкой.
V560 A part of conditional expression is always true: (result == 0).
int lock_carry_node(carry_level * level /* level @node is in */ ,
carry_node * node /* node to lock */)
{
....
result = 0;
....
if (node->parent && (result == 0))
{
....
}
}Тут все просто, значение result не изменяется и проверку можно опустить. Ничего страшного.
V1004 The 'ref' pointer was used unsafely after it was verified against nullptr. Check lines: 1191, 1210.
carry_node *add_new_znode(znode * brother /* existing left neighbor
* of new node */ ,
carry_node * ref /* carry node after which new
* carry node is to be inserted
* into queue. This affects
* locking. */ ,
carry_level * doing /* carry queue where new node is
* to be added */ ,
carry_level * todo /* carry queue where COP_INSERT
* operation to add pointer to
* new node will ne added */ )
{
....
/* There is a lot of possible variations here: to what parent
new node will be attached and where. For simplicity, always
do the following:
(1) new node and @brother will have the same parent.
(2) new node is added on the right of @brother
*/
fresh = reiser4_add_carry_skip(doing,
ref ? POOLO_AFTER : POOLO_LAST, ref);
....
while (ZF_ISSET(reiser4_carry_real(ref), JNODE_ORPHAN))
{
....
}
....
}Суть этой проверки в том, что в тернарном операторе происходит проверка ref на nullptr, а дальше она передается в функцию reiser4_carry_real() где происходит потенциальное разыменование указателя без проверки на nullptr. Однако это не так. Рассмотрим функцию reiser4_carry_real():
znode *reiser4_carry_real(const carry_node * node)
{
assert("nikita-3061", node != NULL);
return node->lock_handle.node;
}Видим, что указатель node проверяется на nullptr в теле функции, так-что все в порядке.
Дальше следует возможно неправильное срабатывания проверки в файле fs/reiser4/tree.c:
V547 Expression 'child->in_parent.item_pos + 1 != 0' is always true.
int find_child_ptr(znode * parent /* parent znode, passed locked */ ,
znode * child /* child znode, passed locked */ ,
coord_t * result /* where result is stored in */ )
{
....
if (child->in_parent.item_pos + 1 != 0) {
....
}Нужно найти объявление item_pos и понять чем оно является. Порыскав в нескольких файлах получаем следующее:
struct znode
{
....
parent_coord_t in_parent;
....
} __attribute__ ((aligned(16)));
....
typedef struct parent_coord
{
....
pos_in_node_t item_pos;
} parent_coord_t;
....
typedef unsigned short pos_in_node_t;В комментариях Andrey2008 указал в чем здесь ошибка. А она состоит в том, что в if выражение преобразуется к типу int, поэтому даже в случае максимального значения item_pos переполнения не будет, т.к. выражение преобразуется к типу int и вместо нуля, получится следующее 0xFFFF + 1 = 0x010000.
Все остальные ошибки являются либо похожими на рассмотренные выше, либо являются ложными срабатываниями, которые тоже были рассмотрены выше.
Выводы простые.
Во-первых, PVS-Studio крут. Любой хороший инструмент в правильных руках позволяет работать лучше и продуктивнее. PVS-Studio в качестве статического анализатора кода уже не раз показал себя с самых лучших сторон. Он позволяет находить и решать неожиданные проблемы, опечатки или недосмотра со стороны разработчика.
Во-вторых, пишите код внимательней. Не нужно использовать "хаки" языка C, по крайней мере не в тех местах где это действительно оправдано и без этого никак нельзя. В условиях всегда используйте дополнительно круглые скобки для расставления приоритетов, ибо даже если вы супер-пупер хакер и спец по языку C, то вы банально можете забыть правильные приоритеты операторов и получить множество ошибок, особенно если за один заход напишете много кода.
Отдельно хочу поблагодарить разработчиков за такой прекрасный инструмент! Они очень постарались, реализуя PVS-Studio для GNU/Linux систем и продумывая реализацию анализатора (подробнее можете прочитать здесь). Он элегантно встраивается в системы сборки и генерирует логи. Если вам не хочется встраивать в систему сборки, то можно просто "отловить" запуски компилятора запуская make.
И самое главное, огромное спасибо за возможность использовать бесплатно для студентов, свободных проектов и индивидуальных разработчиков! Это прекрасно!