Александр Синотов

15 Янв 2018

Теги:

#Cpp

Проверка кода Reiser4 статическим анализатором PVS-Studio

15 Янв 2018

Автор: Александр Синотов

Предварительная подготовка
Проверка libaal-1.0.7
Проверка reiser4progs-1.2.1
Проверка Reiser4
Выводы
Благодарность разработчикам PVS-Studio

Доброго времени суток! Эта статья посвящена применению бесплатной версии (для свободных и открытых проектов) статического анализатора 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-1.0.7

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).

Проверка reiser4progs-1.2.1

Лог анализатора: 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

Переходим непосредственно к проверке кода 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

Отдельно хочу поблагодарить разработчиков за такой прекрасный инструмент! Они очень постарались, реализуя PVS-Studio для GNU/Linux систем и продумывая реализацию анализатора (подробнее можете прочитать здесь). Он элегантно встраивается в системы сборки и генерирует логи. Если вам не хочется встраивать в систему сборки, то можно просто "отловить" запуски компилятора запуская make.

И самое главное, огромное спасибо за возможность использовать бесплатно для студентов, свободных проектов и индивидуальных разработчиков! Это прекрасно!

#Cpp