[Перевод] Проверка проекта Samba с помощью PVS-Studio под Linux04.04.2016 15:03

Если вы следили за новостями о последних разработках в области инструментов анализа 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


В данном коде условие оператора 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();
- }
-