Проверка проекта Samba с помощью PVS-Studio под Linux

Автор оригинала: Aurélien Aptel
  • Перевод
Проверка Samba с помощью PVS-Studio под LinuxЕсли вы следили за новостями о последних разработках в области инструментов анализа C/C++ кода, то, должно быть, слышали про инструмент PVS-Studio. Я узнал о нем благодаря статьям, которые разработчики публикуют на своем сайте и в которых они рассказывают о проверках проектов с открытым кодом. К настоящему времени уже проверено внушительное число проектов, включая ядро Linux, Qt, Unreal и т.д., и каждый раз им удается находить интересные ошибки, подолгу живущие в коде, никем не обнаруженные. Опечатки, неаккуратное копирование, неопределенное поведение, бессмысленный код, синтаксические ошибки, которые чудесным образом пропускаются компилятором…

Как сказал Джон Кармак, "Все, что является допустимым с точки зрения синтаксиса и пропускается компилятором, в конце концов окажется в вашей кодовой базе".

К сожалению, в качестве поддерживаемой операционной системы заявлена только Windows. Инструмент доступен в виде плагина для Visual Studio или в качестве отдельного приложения, если у вас не установлена Visual Studio. Мой первый опыт работы с анализатором пришелся на 2014 год, когда я проверял с его помощью относительно объемную базу C++ кода, которая использовалась для внутренних нужд лабораторией компьютерной графики в моем университете в Лионе (LIRIS). Разработку мы тогда вели в Visual Studio (обычно я им не пользуюсь), и я подумал, почему бы не попробовать анализатор в деле. Я остался доволен результатами и продолжил следить за появлением новых статей на сайте PVS-Studio.

Два года спустя (за это время вышло несколько статей PVS-Studio) я начал работу над проектом Samba. Его суммарный размер — около 2 миллионов строк кода на языке C, и я подумал, что было бы интересно проверить его с помощью PVS-Studio. В коде статического анализатора в принципе не должно быть много платформенно-зависимых участков, так что я стал искать способ, как реализовать такую проверку. Анализатор работает с препроцессированным кодом, поэтому ему необходимо прогнать исходный код через препроцессор, а для этого ему нужно знать обо всех флагах препроцессора, макросах и путях включаемых файлов. Автоматический сбор таких данных может оказаться трудоемкой задачей. Для ее решения я написал небольшой скрипт на основе strace, чтобы он отслеживал вызовы компилятора — в этом случае можно осуществлять проверку независимо от используемого инструмента сборки. Последнюю версию моего решения можно найти на github.

Я отправил этот скрипт разработчикам PVS-Studio, и после небольшой переписки они прислали мне экспериментальную сборку PVS-Studio под Linux (за что им еще раз спасибо!). Теперь скрипт осуществляет все этапы анализа: от сбора информации о ключах компиляции до непосредственно анализа, отображения и фильтрации результатов.

А теперь давайте разберемся, как работать с этим скриптом.

Чтобы не пришлось при каждом запуске указывать расположение файла лицензии и исполняемого файла, можно настроить переменные окружения.
$ export PVS_LICENSE=~/prog/pvs/PVS-Studio.lic
$ export PVS_BIN=~/prog/pvs/PVS-Studio

Перейдите в директорию проекта и сгенерируйте файл конфигурации для своего C++11-проекта.
$ pvs-tool genconf  -l C++11 pvs.cfg

При необходимости настройте параметры сборки до начала компиляции, после чего включите отслеживание текущей сборки (команду сборки следует указать после символов --).
$ pvs-tool trace    -- make -j8

После этого будет сгенерирован файл «strace_out», в котором содержится вся необходимая информация. На этапе анализа из этого файла будут извлечены все единицы компиляции и флаги препроцессора, после чего они будут проверены в PVS-Studio.
$ pvs-tool analyze  pvs.cfg
pvs-tool: deleting existing log pvs.log...
001/061 [ 0%] analyzing /hom../rtags/src/ClangIndexer.cpp...
002/061 [ 1%] analyzing /hom../rtags/src/CompilerManager.cpp...
003/061 [ 3%] analyzing /hom../rtags/src/CompletionThread.cpp...
004/061 [ 4%] analyzing /hom../rtags/src/DependenciesJob.cpp...
<...>
061/061 [98%] analyzing /hom../rtags/src/rp.cpp...
pvs-tool: analysis finished
pvs-tool: cleaning output...
pvs-tool: done (2M -> 0M)

На этапе очистки скрипт удалит дублированные строки, что значительно сократит размер файла с результатами анализа.

Теперь можно просматривать результаты, сгруппированные по файлам, к которым они относятся:
$ pvs-tool view     pvs.log

Формат выходного файла аналогичен формату gcc/make, то есть с ним можно работать «как есть». Например, его можно открыть в редакторе Emacs и применять к нему стандартные встроенные функции перехода к ошибкам (goto-error). Также можно выключать отдельные диагностики, например:
$ pvs-tool view -d V2006,V2008 pvs.log

По умолчанию отображаются предупреждения только 1 уровня, но этот параметр можно изменить с помощью команды -l.

Подробная справка вызывается командой -h.

PVS-Studio нашел много проблемных участков в Samba. Большинство из них оказались ложными срабатываниями, но это неизбежно при проверке объемной кодовой базы, каким бы анализатором вы ни пользовались. Важно то, что были найдены и настоящие ошибки. Ниже я приведу наиболее интересные из них, а также способы их исправления в формате diff-патчей.
- if (memcmp(u0, _u0, sizeof(u0) != 0)) {
+ if (memcmp(u0, _u0, sizeof(*u0)) != 0) {
   printf("USER_MODALS_INFO_0 struct has changed!!!!\n");
   return -1;
  }

В этом примере неправильно поставлена закрывающая скобка. Результат сравнения sizeof с нулём был использован в качестве размера памяти для функции memcmp (всегда 1 байт). Также нас интересует размер типа, на который указывает указатель u0, а не размер самого указателя.
   handle_main_input(regedit, key);
   update_panels();
   doupdate();
- } while (key != 'q' || key == 'Q');
+ } while (key != 'q' && key != 'Q');

В данном коде необходимо выйти из цикла, если встретится буква 'q' в любом регистре.
  uid = request->data.auth.uid;
 
- if (uid < 0) {
+ if (uid == (uid_t)-1) {
   DEBUG(1,("invalid uid: '%u'\n", (unsigned int)uid));
   return -1;
  }

Здесь переменная типа uid_t проверяется на отрицательное значение.

Знак переменной типа uid_t не определен в POSIX. Он определён как беззнаковый тип размером 32 бита на Linux, следовательно, проверка < 0 всегда ложна.

В случае с беззнаковой версией типа uid_t компилятор в сравнении uid == -1 всегда будет неявно приводить -1 к беззнаковому типу, в результате чего сравнение как со знаковым, так и с беззнаковым типом uid_t всегда будет давать верный результат. Я сделал преобразование явным: в нашем случае чем меньше магии, тем лучше.
  DEBUG(4,("smb_pam_auth: PAM: Authenticate User: %s\n", user));
 
- pam_error = pam_authenticate(pamh, PAM_SILENT |
-   allow_null_passwords ? 0 : PAM_DISALLOW_NULL_AUTHTOK);
+ pam_error = pam_authenticate(pamh, PAM_SILENT |
+  (allow_null_passwords ? 0 : PAM_DISALLOW_NULL_AUTHTOK));
  switch( pam_error ){
   case PAM_AUTH_ERR:
    DEBUG(2, ("smb_pam_auth: PAM: ....", user));

Обычная путаница с приоритетом операторов.
  gensec_init();
  dump_args();
 
- if (check_arg_numeric("ibs") == 0 ||
-     check_arg_numeric("ibs") == 0) {
+ if (check_arg_numeric("ibs") == 0 ||
+     check_arg_numeric("obs") == 0) {
   fprintf(stderr, "%s: block sizes must be greater that zero\n",
     PROGNAME);
   exit(SYNTAX_EXIT_CODE);

В этом примере дважды проверялось одно и то же.
   if (!gss_oid_equal(&name1->gn_type, &name2->gn_type)) {
    *name_equal = 0;
   } else if (name1->gn_value.length != name2->gn_value.length ||
-      memcmp(name1->gn_value.value, name1->gn_value.value,
+      memcmp(name1->gn_value.value, name2->gn_value.value,
    name1->gn_value.length)) {
    *name_equal = 0;
   }

В этом коде функция memcmp вызывается с одним и тем же указателем в качестве ее параметров, в результате чего один и тот же блок памяти сравнивается сам с собой.
  ioctl_arg.fd = src_fd;
  ioctl_arg.transid = 0;
  ioctl_arg.flags = (rw == false) ? BTRFS_SUBVOL_RDONLY : 0;
- memset(ioctl_arg.unused, 0, ARRAY_SIZE(ioctl_arg.unused));
+ memset(ioctl_arg.unused, 0, sizeof(ioctl_arg.unused));
  len = strlcpy(ioctl_arg.name, dest_subvolume,
         ARRAY_SIZE(ioctl_arg.name));
  if (len >= ARRAY_SIZE(ioctl_arg.name)) {

Здесь в качестве параметра memset было передано количество элементов массива, а не размер в байтах.
  if (n + IDR_BITS < 31 &&
-     ((id & ~(~0 << MAX_ID_SHIFT)) >> (n + IDR_BITS))) {
+     ((id & ~(~0U << MAX_ID_SHIFT)) >> (n + IDR_BITS))) {
   return NULL;
  }

Использование отрицательных значений в качестве левого операнда операции сдвига влево ведет к неопределенному поведению согласно стандарту языка C.
  if (cli_api(cli,
        param, sizeof(param), 1024, /* Param, length, maxlen */
-       data, soffset, sizeof(data), /* data, length, maxlen */
+       data, soffset, data_size, /* data, length, maxlen */
        &rparam, &rprcnt,   /* return params, length */
        &rdata, &rdrcnt))   /* return data, length */
  {

В этом примере мы имеем дело с данными, которые когда-то были массивом, выделенным на стеке, но затем превратились в буфер, выделенный в куче, в то время как операцию sizeof подправить забыли.
   goto query;
  }
 
- if ((p->auth.auth_type != DCERPC_AUTH_TYPE_NTLMSSP) ||
-     (p->auth.auth_type != DCERPC_AUTH_TYPE_KRB5) ||
-     (p->auth.auth_type != DCERPC_AUTH_TYPE_SPNEGO)) {
+ if (!((p->auth.auth_type == DCERPC_AUTH_TYPE_NTLMSSP) ||
+       (p->auth.auth_type == DCERPC_AUTH_TYPE_KRB5) ||
+       (p->auth.auth_type == DCERPC_AUTH_TYPE_SPNEGO))) {
   return NT_STATUS_ACCESS_DENIED;
  }

До исправления условие всегда было истинным и функция всегда возвращала статус «доступ запрещен».
- Py_RETURN_NONE;
  talloc_free(frame);
+ Py_RETURN_NONE;
}

Py_RETURN_NONE — это макрос, который скрывает оператор return. В данном фрагменте, где используется привязка к коду на Python, многие функции возвращали результат прежде, чем освобождалась динамически выделенная память. Эта проблема встречалась в десятках функций.
  int i;
- for (i=0;ARRAY_SIZE(results);i++) {
+ for (i=0;i<ARRAY_SIZE(results);i++) {
   if (results[i].res == res) return results[i].name;
  }
  return "*";

В данном коде условие оператора for всегда оказывалось истинным.
 int create_unlink_tmp(const char *dir)
 {
+ if (!dir) {
+  dir = tmpdir();
+ }
+
  size_t len = strlen(dir);
  char fname[len+25];
  int fd;
  mode_t mask;
 
- if (!dir) {
-  dir = tmpdir();
- }
-

А здесь указатель dir использовался перед проверкой на ноль.

В целом я остался доволен анализатором PVS-Studio и охотно рекомендую его к использованию. К сожалению, официально он недоступен под Linux, но, как я понимаю, достаточно просто написать разработчикам, и они помогут вам с настройкой под эту операционную систему :)
PVS-Studio
Static Code Analysis for C, C++, C# and Java

Комментарии 22

    +4
    Здесь был комментарий про изменения в функции create_unlink_tmp(). Человек спросил: "а что собственно изменилось?". К сожалению, я промахнулся мышкой и нажал "отклонить комментарий read-only пользователя". Прошу прощения. Ответ:
    Изменения в том, что теперь проверка на NULL выполняется ДО использования указателя dir. Т.е. проверку перенеси выше.
      +15
      Хотел спросить «а как же оно работало?», но вовремя вспомнил — хреново оно работало.
        +4
        Недавно обнаружил у себя промах — сверяя расширение файла приводил его к верхнему регистру, а сверял со строкой в нижнем.
        Это, правда, Delphi (if AnsiUpperCase(ExtractFileExt(FileName)) = '.wav'). Но с месяц назад пробовал имеющиеся под Delphi статические анализаторы — никто из них не ткнул меня в это носом.
        Стало интересно — а Вы такое ловите? Т.е. смысл: отловить, что сравниваемая переменная, была преобразована прямо сейчас или строкой выше в определенный регистр — значит в правой части должны быть только символы этого регистра.
          +4
          Нет, такой диагностики нет. Но идея интересная. Пометил себе. Со временем возможно реализуем. Спасибо.
          0
          При чтении статьи сложилось впечатление, что pvs-studio самостоятельно генерирует patch-файлы ))
            +2
            Пока мы побаиваемся идти в сторону автоматического исправления ошибок. Человек-то иногда исправляет хуже, чем было… "Торопыжка был голодный, проглотил утюг холодный" и "Поспешишь — людей насмешишь" даже в статическом анализе актуально. Иногда после наших статей о проверке проектов кто-то сторонний начинает спешно править найденные ошибки в чужом проекте. И делает хуже, так как правит неправильно.
            0
            А где взять бинарник pvs для запуска на линукс?
            На официальном сайте ничего нет.
              +2
              Прошу написать нам от лица компании, и мы обсудим интересующие Вас вопросы.
                0
                Я уж было зашёл в этот пост посмотреть, вдруг как-то проще теперь под линуксом PVS потыкать. Ан нет, каждый раз одна и та же песня.

                Понимаете, во-первых, я, например, не уполномочен что бы то ни было делать от лица компании, у нас для этого есть специально обученные люди. Я могу, конечно, их попросить вам написать, и они даже, наверное, спустя несколько оборотов шестерёнок бюрократической машины напишут, месяца этак через два, а потом вы им напишете, а потом они вам. Расскажите только потом обязательно, как вам удалось с юристами и PR-менеджерами обсудить интересующие меня вопросы!

                Во-вторых, я вообще не понимаю, какое отношение моя компания имеет к моему желанию поиграться с вашим анализатором на своём собственном коде в своё свободное время. Ни моё свободное время, ни мой пет-проджект к компании не относятся. Вот будут у меня положительные впечатления от PVS, приду я к своему менеджеру, скажу, «чувак, слушай, PVS — клёвая штуковня, давай для начала купим пяток лицензий на наш отдел?», и дальше с вами уже будут общаться от лица компании и далее по тексту.

                В-третьих (и, пожалуй, «в основных»), просто больно видеть такие чудеса маркетинга и вовлечения потенциальных клиентов. Поучитесь у JetBrains, что ли.
                  0
                  У вас PR-менеджеры софт выбирают?

                  приду я к своему менеджеру


                  Ой, что-то у меня сомнения.

                  Так как кому нужна Linux-версия — спокойно работают с нами в почте. Критикуют, ругают, хвалят. В общем нормальный рабочий процесс. Но есть обязательно СЕКРЕТНЫЙ АНОНИМ, который будет строчить комменты, но не напишет с корпоративной почты никогда…
                    0
                    У вас PR-менеджеры софт выбирают?

                    У нас PR-менеджеры пишут от лица компании. Предполагается, что для выбора софта ничего от лица компании делать не нужно, кроме выделения бюджета и переговоров с поставщиком.

                    Собственно, в подавляющем большинстве случаев это так и есть: мне не нужно писать с корпоративной почты, чтобы попробовать VTune (и еще дюжину инструментов от Интела), Coverity, CLion.

                    Ой, что-то у меня сомнения.

                    Зря, с интеловским софтом так и было. Либо это у вас такая самокритика, тогда уважаю!

                    не напишет с корпоративной почты никогда

                    Ну я же написал уже, причем мой личный проект, выполняемый в свободное время, к корпоративной почте? Вы думаете, я сразу на боевых проектах буду это дело гонять?
                    0
                    Свершилось: PVS-Studio для Linux.
                  0
                  +1
                  Не подскажете, как ведёт себя статический анализатор, заточенный (?) под C++, на чисто сишном коде? Не даёт ли ложных срабатываний?
                  Я недавно пытался использовать libusb-1.0 в плюсовом проекте. Когда я пытался «в лоб» обернуть струтуру libusb_transfer в класс, словил ошибку компиляции, поскольку в декларации структуры есть замечательные строчки
                  struct libusb_transfer {


                  struct libusb_iso_packet_descriptor iso_packetdesc
                  #if defined(STDC_VERSION) && (STDC_VERSION>= 199901L)
                  [] / valid C99 code /
                  #else
                  [0] / non-standard, but usually working code /
                  #endif
                  ;
                  }
                  PS Извините, как ни глумился на редактором, нормальное форматирование не смог сделать, а тега [code] мне, как RO, не положено.
                    +2
                    Не подскажете, как ведёт себя статический анализатор, заточенный (?) под C++, на чисто сишном коде? Не даёт ли ложных срабатываний?
                    Понятия не имею. Наверное, плохо себя ведёт.

                    Анализатор PVS-Studio различает язык С и С++ и учитывает их особенности при анализе.
                    +1
                    Было бы круто, если бы c помощью pvs-studio нашли причину http://badlock.org/ до публикации патча. Правда, это может быть какая-нибудь ошибка в логике, которую никак не поймаешь анализатором.
                      0
                      А какая версия Samba проверялась? Внесли ли исправления найденных ошибок в проект, если да, с какой версии?
                        0
                        Напоминаю, что это перевод. Я думаю, есть смысл задать эти вопросы автору статьи. Впрочем, вопрос про версию уже задан. См. комментарии к статье.
                        –2

                        handle_main_input(regedit, key);
                        update_panels();
                        doupdate();
                        — } while (key != 'q' || key == 'Q');
                        + } while (key != 'q' && key != 'Q');

                        В данном коде необходимо выйти из цикла, если встретится буква 'q' в любом регистре.

                        Мне кажется, сложновато будет одновременно нажать 'q' и 'Q', а правильно будет, все же, так:
                        } while (key != 'q' || key != 'Q');
                          +2
                          Там отрицание. Ваше всегда будет true.
                            +1
                            while (key != 'q' || key != 'Q');
                            Нет, так не выйти, т.к. логическое выражение key != 'q' || key != 'Q' будет истинно всегда, т.к. в любом случае один из операндов || будет возвращать true. В предлагаемом же варианте key != 'q' && key != 'Q' при нажатии 'q' или 'Q' один из операнодв && примет значение false и в целом выражение тоже вернет false.
                              0
                              Да, теперь сообразил)) Спасибо!

                          Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                          Самое читаемое