Проверка Vim при помощи PVS-Studio в GNU-Linux

Читатель мог бы подумать, что это очередная статья о проверке еще одного проекта из мира свободного ПО, но на самом деле, статья не столько о проверке, сколько о практике использования анализатора PVS-Studio в полностью GNU/Linux окружении. Не случайно выбором проекта для проверки стал Vim, ибо и он в этом деле сослужил свою службу.

Для начала немного о Vim

Vim (http://vim.org) — кроссплатформенный свободный текстовый редактор c 30-летней историей, являющийся наследником редактора vi и пришедший из мира Unix систем.

Vim весьма широко применяется в администрировании и разработке, во многих дистрибутивах GNU/Linux он является редактором по умолчанию. От других текстовых редакторов Vim отличается ориентацией на использование исключительно клавиатуры, текстовый интерфейс, богатыми возможностями расширения через систему написанных на Vim Script плагинов.

Теперь о самой проверке

Одним из вариантов проверки проектов под Linux является интеграция в систему сборки, например, GNU Make. Для проверки vim был выбран именно этот способ. Для каждого вызова компилятора в make-файле был добавлен вызов анализатора. Для удобства этот вызов был обернут в переменную Make подобным образом:

#PVS Studio vars
PVS_CFLAGS = $(ALL_CFLAGS)
PVS_INCFLAGS = -I$(srcdir)
PVS_STUDIO = ~/PVS-Studio/PVS-Studio –cfg 
    ~/PVS-Studio/PVS-Studio_vim.cfg --source-file 
    $< --cl-params $(PVS_CFLAGS) -c $(PVS_INCFLAGS) $<

Далее проект собирается в обычном режиме командой make (при желании можно добавить отдельный target для анализа, например ".analysis"). Результатом, помимо собранного проекта является сырой лог анализатора.

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

С сырым логом работать невозможно. В этом логе множество дублирующихся сообщений (когда один .h-файл включен в несколько .cpp-файлов). Для изменения параметров анализа приходится, после правки конфигурационного файла, запускать проверку заново, что серьезно сказывается на времени для больших проектов. Даже если мы всего лишь хотели, например, отключить предупреждения, относящиеся к файлам из определенной директории. Для решения этой проблемы на C++ была написана утилита log-parser, которая обрабатывает сырой лог PVS-Studio, убирает дублирующиеся сообщения, применяет к сообщениям фильтры из своего файла опций и выводит предупреждения анализатора в одном из поддерживаемых форматов. Утилита работает очень быстро (даже на логах проверки очень больших проектов ее время работы не превышает 2-3 секунд), что дает возможность легко и быстро изменить какие-то параметры анализа и получить новый список сообщений.

При необходимости можно легко добавить новые форматы вывода. На данный момент их два: xml и так называемый errorfile. Насколько я понимаю, официального названия у него и нет, это именно тот формат, в котором выдают сообщения многие программы под Linux, например, grep, ошибки компиляции gcc и т.д. Именно он нам и пригодился.

В отличие от Windows, где подавляющее большинство разработчиков пользуются Visual Studio, в мире GNU/Linux существует множество IDE, текстовых редакторов и прочего со своей долей пользователей. Единого мнения и перевеса сторон нет и каждый выбирает себе инструмент по своему вкусу. Тем не менее, при проверке проектов важно не только получить результаты анализа, но и иметь возможность удобной работы с ними, как эту возможность предоставляет интеграция PVS-Studio с Visual Studio. Формат сообщений ошибок, описанный выше, является своего рода стандартом для Linux программ и большинство редакторов и сред разработки имеют ту или иную его поддержку, но, к сожалению, в большинстве случаев эта поддержка существует лишь для чтения сообщений компилятора из stderr при сборке, в данном же случае гораздо удобнее брать сообщения анализатора из заранее подготовленного файла.

Здесь нам и пригодился редактор Vim. Конечно, к любому из остальных инструментов можно разработать соответствующий плагин, но обнаружилось, что Vim имеет эту возможность изначально.

Рисунок 1 – Запущенный vim с результатами анализа.

Достаточно после работы анализатора и утилиты обработки лога выполнить vim -q <errorfile>, А затем в открывшемся редакторе выполнить команду создания буфера с ошибками. Например, :cw 20. И вот мы уже получаем комфортную среду для работы с сообщениями анализатора и навигации по коду. Конечно, пришлось потратить несколько часов на изучение самого Vim, так как я в нем ранее не работал, а его принципы использования очень сильно отличаются от более привычных текстовых редакторов. Но в итоге я могу сказать, что весьма проникся удобством его использования и из категории непонятных штуковин для инопланетян он перешел в категорию мощных инструментов для работы. Соответственно, долго с выбором проекта для проверки мучиться не пришлось — конечно же сам Vim. Код vim оказался очень качественным, явных багов в нем не нашлось (хотя стиль написания кода местами довольно спорный, но, думаю, можно сделать скидку на возраст проекта. Тем не менее обнаружилось некоторое количество мест, на которые стоит обратить внимание. Поговорим о них подробнее.

Лишняя проверка

    if (ptr == NULL)
    {
        if (compl_leader != NULL)
            ptr = compl_leader;
        else
            return;  /* nothing to do */
    }
    if (compl_orig_text != NULL)
    {
        p = compl_orig_text;
        for (len = 0; p[len] != NUL && p[len] == ptr[len]; ++len)
        ;
#ifdef FEAT_MBYTE
        if (len > 0)
            len -= (*mb_head_off)(p, p + len);
#endif
        for (p += len; *p != NUL; mb_ptr_adv(p))
            AppendCharToRedobuff(K_BS);
    }
    else
        len = 0;
    if (ptr != NULL)
        AppendToRedobuffLit(ptr + len, -1);

Предупреждение анализатора V595 (1) The 'ptr' pointer was utilized before it was verified against nullptr. Check lines: 3922, 3933.

Указатель ptr уже был проверен на NULL, в случае истинности этого условия ему был присвоен указатель comp_leader, который точно не нулевой. Вторая проверка не требуется.

Странный memset

/*
* If requested, store and reset the global values controlling
* the exception handling (used when debugging). Otherwise avoid
* clear it to a bogus compiler warning when the optimizer
* uses inline functions...
*/
if (flags & DOCMD_EXCRESET)
  save_dbg_stuff(&debug_saved);
else
  vim_memset(&debug_saved, 0, 1);

где debug_saved — объект структуры

struct dbg_stuff
{
    int        trylevel;
    int        force_abort;
    except_T    *caught_stack;
    char_u    *vv_exception;
    char_u    *vv_throwpoint;
    int        did_emsg;
    int        got_int;
    int        did_throw;
    int        need_rethrow;
    int        check_cstack;
    except_T    *current_exception;
};

Сообщение анализатора: V512 (1) A call of the 'memset' function will lead to underflow of the buffer '& debug_saved'.

Сложно сказать, зачем нужно было обнулять только первый байт структуры. Если это используется как какой-то флаг, лучше определить его отдельным полем структуры (можно union).

Подозрительный цикл

/* check for out-of-memory */
for (i = 0; i < num_names; ++i)
{
  if (names[i] == NULL)
  {
    for (i = 0; i < num_names; ++i)
      vim_free(names[i]);
    num_names = 0;
  }
}

Сообщение анализатора: V535 (1) The variable 'i' is being used for this loop and for the outer loop. Check lines: 1893, 1897.

Во внешнем и внутреннем цикле по одному и тому же массиву проходят одним и тем же счетчиком i. Понятно, что при первом же срабатывании условия if (names[i] == NULL) следующий шаг внешнего цикла уже выполняться не будет, но стороннему человеку придется напрячься, чтобы понять этот код верно, а своеобразность его написания наводит на мысли, то ли поведение имел в виду автор. Другими словами, хотя ошибки нет, но код немного пахнет. Пожалуй, оператор 'break' подошел бы для остановки цикла лучше.

Области видимости

char_u *p, *old;
//...
{
    char_u        buffer[BUFLEN + 1];
    //...
    for (p = buffer; p < buffer + len; p += l)
    //...

Предупреждение анализатора: V507 (2) Pointer to local array 'buffer' is stored outside the scope of this array. Such a pointer will become invalid.

Подобных мест в коде в Vim довольно много (к вопросу о стиле). В указателе p, объявленном в самом начале функции (кое-где вообще с глобальной областью видимости) сохраняется указатель на массив, существующий только в меньшей области видимости и который будет удален при выходе из его блока кода. При беглом осмотре мне показалось, что после выхода из области видимости buffer указатель p используется только после присвоения ему нового значения, но ведь где-то можно это и пропустить. С какой целью так делать вместо объявления еще одной переменной внутри области видимости buffer (неужели для экономии места на стеке?), мне не понятно. Такой код плохо поддерживаем и неудобно читаем.

Ошибка с signed и unsigned типами в одном выражении

for (cu = 1; cu <= 255; cu++)
    if (VIM_ISDIGIT(cu))
        regc(cu);

где

#define VIM_ISDIGIT(c) ((unsigned)(c) - '0' < 10)

Предупреждение анализатора: V658 (2) A value is being subtracted from the unsigned variable. This can result in an overflow. In such a case, the '<' comparison operation can potentially behave unexpectedly. Consider inspecting the '(unsigned)(cu) — '0' < 10' expression.

Этот код больше похож на грязный хак. При вычислении выражения ((unsigned)© — '0' < 10) результатом вычитания будет значение типа unsigned и при сравнении обе части выражения тоже будут приведены к unsigned. Соответственно, когда переменная cu меньше, чем числовое значение 0, происходит переполнение. В данным случае код ведет себя верно, цель свою (проверка, является ли символ цифрой) выполняет, но не думаю, что есть причины использовать в своем коде подобные приемы без нужды. Цикл можно было сделать от '0' и обойтись без приведения к unsigned.

Указатель инициализирован NULL и нигде не изменяется, притом он используется

char_u    *retval = NULL;
//...
if (round == 2)
  vim_strncpy(retval, s, len); //первое использование retval
//...
if (retval == NULL)
{

Предупреждение анализатора: V595 (1) The 'retval' pointer was utilized before it was verified against nullptr. Check lines: 7903, 7907.

Это уже похоже на ошибку. Анализатор говорит о лишней проверке, но на самом деле проблема прежде всего в другом. Указатель retval инициализирован 0, и я не нашел ни одного места в этой функции, где бы его значение менялось. При этом в него делается много раз делается strncpy. После чего его внезапно решили проверить на NULL.

Небезопасное использование realloc

/* TODO: check for vim_realloc() returning NULL. */
l->t = vim_realloc(l->t, newlen * sizeof(nfa_thread_T));

Предупреждение анализатора V701 (2) realloc() possible leak: when realloc() fails in allocating memory, original pointer 'l->t' is lost. Consider assigning realloc() to a temporary pointer.

Очень частая ошибка во многих проектах, суть этой ошибки подробно описана в сообщении анализатора. К счастью, судя по комментарию, это скоро исправят. В других местах в коде Vim realloc используется правильно.

Несколько примеров ложных срабатываний

if (ireg_icombine && len == 0)
{
  /* If Z was present, then ignore composing characters.
   * When ignoring the base character this always matches. */
   if (len == 0 && sta->c != curc)
     result = FAIL;

V560 (2) A part of conditional expression is always true: len == 0.

V571 (2) Recurring check. The 'len == 0' condition was already verified in line 6032.

if (VIsual_active)
{
  if (VIsual_active
      && (VIsual_mode != wp->w_old_visual_mode
      || type == INVERTED_ALL))

V571 (2) Recurring check. The 'VIsual_active' condition was already verified in line 1515.

Есть еще несколько мест с подобными проверками. Особого интереса для обсуждения они не вызывают, в большинстве случаев вреда не несут, но в некоторых может скрываться логическая ошибка, поэтому таким местам стоит уделять внимание.

Просто плохой код, в структуре заполняют только первый байт

#ifdef FEAT_TAG_BINS
  /* This is only to avoid a compiler warning for using search_info
  * uninitialised. */
  vim_memset(&search_info, 0, (size_t)1);
#endif

V512 (1) A call of the 'memset' function will lead to underflow of the buffer '& search_info'.

В комментарии объяснено, зачем это сделано, но способ довольно странный. Есть множество более красивых путей избегания предупреждения компилятора.

Плохая практика использования коротких имен

extern char *UP, *BC, PC;

Предупреждение анализатора: V707 (2) Giving short names to global variables is considered to be bad practice. It is suggested to rename 'UP', 'BC', 'PC' variables.

Подобная практика не редкость в коде Vim. У многих переменных 1- и 2-буквенные имена, зачастую с большой областью видимости, а в данном случае вообще с глобальной. В сочетании с функциями длиной в 500+ строк это приводит к весьма трудночитаемому коду.

Странное присваивание i в условии

int i = 2; /* index in s[] just after <Esc>[ or CSI */
//...
if (n >= 8 && t_colors >= 16
    && ((s[0] == ESC && s[1] == '[')
        || (s[0] == CSI && (i = 1) == 1))
    && s[i] != NUL
    && (STRCMP(s + i + 1, "%p1%dm") == 0
    || STRCMP(s + i + 1, "%dm") == 0)
    && (s[i] == '3' || s[i] == '4'))

Предупреждение анализатора: V560 (2) A part of conditional expression is always true: (i = 1) == 1.

Сложно сказать, ошибка ли это или просто странный способ присвоить i единицу. Писать так явно не стоит.

Заключение

В заключение хотелось бы сказать, что сейчас проверка проектов с помощью PVS-Studio под GNU Linux без использования машины с Windows является вполне реальной и довольно удобной. Помог в этом числе и Vim, за что был подвергнут подобному сценарию проверки первым.

Эта статья на английском

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Anton Tokarev. Analyzing Vim by PVS-Studio in GNU/Linux.

Автор: FoxCanFly

Источник