Пару недель назад (а если быть точнее, то 2 июля 2021 года) исполнилось двадцать лет легендарному протоколу BitTorrent. Созданный Брэмом Коуэном (Bram Cohen) протокол с момента своего появления стремительно развивался и быстро стал одним из самых популярных способов для обмена файлами. Почему бы в честь этого не проверить парочку долгоживущих тематических проектов с помощью анализатора PVS-Studio для Linux?

Введение

Сегодня на рассмотрении у нас будет два проекта: libtorrent (он же "Rasterbar libtorrent" или "rb-libtorrent") и Transmission.

Libtorrent – свободная кроссплатформенная библиотека для работы с протоколом BitTorrent, написанная на языке С++. На официальном сайте в списке достоинств упоминается эффективное использование ресурсов центрального процессора и памяти, а также простота использования. Судя по английской wiki, на этой библиотеке базируется около половины доступных сейчас BitTorrent клиентов.

Transmission – кроссплатформенный BitTorrent клиент с открытым исходным кодом. Так же как и у libtorrent, основными преимуществами Transmission являются легкость в эксплуатации и эффективное использование ресурсов. Кроме того, программа может похвастаться полным отсутствием рекламы, аналитики, платных версий, наличием нативного GUI (графического пользовательского интерфейса) для различным платформ, а также headless версий (без GUI) для установки на серверах, роутерах и т. п.

Как проверялось

Для анализа использовалась Linux версия статического анализатора PVS-Studio, запущенная в контейнере с Ubuntu 20.04 через WSL2. Для установки в консоли необходимо выполнить следующие команды (для остальных систем также есть инструкции в документации):

wget -q -O - https://files.viva64.com/etc/pubkey.txt | \
  sudo apt-key add -

sudo wget -O /etc/apt/sources.list.d/viva64.list \
  https://files.viva64.com/etc/viva64.list

sudo apt-get update
sudo apt-get install pvs-studio

Далее перед проверкой необходимо ввести данные своей лицензии. Делается это с помощью следующей команды:

pvs-studio-analyzer credentials NAME KEY

(где NAME и KEY – имя и ключ лицензии соответственно).

Таким образом лицензия будет сохранена в каталоге ~/.config/PVS-Studio/, и её не придется указывать при каждом запуске анализатора.

К слову, о лицензиях… Мы активно поддерживаем разработчиков проектов с открытым исходным кодом и поэтому не только сообщаем о найденных багах в репозитории, но и даём им возможность использовать PVS-Studio бесплатно. Всех остальных приглашаем скачать и попробовать анализатор PVS-Studio в деле, воспользовавшись временной лицензией :)

Для запуска анализа воспользуемся самым простым способом – попросим сборочную систему сгенерировать файл compile_commands.json (в котором перечисляются все параметры и команды, необходимые для сборки проекта). А уже его передадим для анализа в PVS-Studio. С этой целью во время сборки добавим аргумент -DCMAKE_EXPORT_COMPILE_COMMANDS=On к вызову cmake. Например:

cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=On ..

Ну и наконец запустим анализ. Для этого в папке, содержащей сгенерированный файл compile_commands.json, выполним следующую команду:

pvs-studio-analyzer analyze -o transmission.log -j 8

где с помощью ключа -o указывается файл для сохранения результатов работы анализатора, а флаг -j позволяет распараллелить анализ требуемого количества потоков.

Если описанный выше способ взаимодействия с PVS-Studio вам не подходит, то у нас в документации есть примеры по взаимодействию с различными сборочными системами и компиляторами.

Ещё один примечательный момент – использование формата SARIF для просмотра отчёта анализатора. Это особенно актуально для разработчиков, предпочитающих редактор Visual Studio Code, ведь для него существует расширение Sarif Viewer, которое позволяет просматривать отчёт и прямо из него переходить по затронутым в коде местам. На скриншоте ниже вы как раз можете увидеть, как визуально выглядела проверка проекта Transmission.

Для создания отчета в формате SARIF при работе с PVS-Studio в Linux необходимо после работы анализатора выполнить следующую команду:

plog-converter -t sarif -o ./transmission.sarif ./transmission.log -d V1042

где с помощью -t sarif мы как раз указываем, что результат нужно сохранить в формате SARIF. Флаг -o указывает название файла-отчёта. А флагом -d мы подавляем нерелевантные в данном случае диагностики.

Более подробно прочитать про открытый стандарт обмена результатами статического анализа (SARIF) можно на сайте OASIS Open. А с примером взаимодействия с GitHub можно ознакомиться в нашей статье "Как в GitHub смотреть красивые отчеты об ошибках с помощью SARIF".

Результаты проверки

Хотелось бы похвалить разработчиков – код достаточно чистый и заслуживающих упоминания предупреждений нашлось совсем немного. Конечно же, хотелось для своей первой статьи найти какие-нибудь интересные ошибки, покопаться в деталях, но… увы. Проекты небольшие по объему, и видно, что ими занимаются опытные разработчики. Кроме того, в changelog'ах были найдены упоминания использования сторонних статических анализаторов (Coverity, Cppcheck). Но даже несмотря на всё это PVS-Studio удалось найти пару любопытных ошибок.

Transmission

Начнём, пожалуй, с проекта Transmission, как наиболее популярного и часто используемого. Только хочу заранее предупредить, что для удобства чтения код будет сокращаться и минимально рефакториться.

Фрагмент N1: использование memset для очистки памяти

static void freeMetaUI(gpointer p)
{
  MakeMetaUI* ui = p;

  tr_metaInfoBuilderFree(ui->builder);
  g_free(ui->target);
  memset(ui, ~0, sizeof(MakeMetaUI));
  g_free(ui);
}

Предупреждение V597 The compiler could delete the 'memset' function call, which is used to flush 'ui' object. The memset_s() function should be used to erase the private data. makemeta-ui.c:53

Уже немало раз было сказано да и немало статей написано про грабли, на которые разработчики периодически наступают при использовании функции memset для очистки памяти. Если вкратце, то компилятор имеет полное право удалить вызовы memset, если посчитает их бессмысленными (такое часто происходит, когда буфер очищается в конце операции и более не используется). Удостовериться в том, что компиляторы действительно могут убрать ненужный вызов, можно с помощью проверки аналогичного кода сервисом Compiler Explorer.

Как видно из рисунка выше, компилятор Clang 12.0.1 уже при использовании флага компиляции -O2 действительно будет вырезать вызов memset. Многие скажут: "Ну, удалил и удалил, чего бубнить-то", — а проблема в том, что незатёртыми могут оказаться приватные данные пользователя. Согласен, не думаю, что проблема приватности данных будет актуальна для торрента клиента. Но если разработчик написал так здесь, то что помешает ему сделать так же в более ответственном месте? Чтобы избежать этого, нужно использовать специально предназначенные функции (например, memset_s или RtlSecureZeroMemory). Более подробно про эту проблему уже писали мои коллеги раз, два и три.

Фрагмент N2: ошибки в библиотеках – тоже ошибки

void jsonsl_jpr_match_state_init(jsonsl_t jsn,
                                 jsonsl_jpr_t *jprs,
                                 size_t njprs)
{
  size_t ii, *firstjmp;
  ...
  jsn->jprs = (jsonsl_jpr_t *)malloc(sizeof(jsonsl_jpr_t) * njprs);
  jsn->jpr_count = njprs;
  jsn->jpr_root = (size_t*)calloc(1, sizeof(size_t) * njprs * jsn->levels_max);
  memcpy(jsn->jprs, jprs, sizeof(jsonsl_jpr_t) * njprs);

  /* Set the initial jump table values */
  firstjmp = jsn->jpr_root;
  for (ii = 0; ii < njprs; ii++) {
    firstjmp[ii] = ii+1;
  }
}

Предупреждение V575 The potential null pointer is passed into 'memcpy' function. Inspect the first argument. Check lines: 1142, 1139. jsonsl.c:1142

Предупреждение V522 There might be dereferencing of a potential null pointer 'firstjmp'. Check lines: 1147, 1141. jsonsl.c:1147

В этом фрагменте скрылись сразу две проблемы, и обе связаны с отсутствием проверки указателя, полученного из функции malloc/calloc. Да, возможно, эта ошибка вообще никогда не проявит себя, но именно этот код желательно поправить. Почему? Все просто – используя сторонние библиотеки, разработчик безоговорочно доверяет им часть работы и вычислений. И, я думаю, мало кому будет приятно, если программа вдруг повредит важные данные, тем более по вине сторонней библиотеки. Более подробно эта проблема и пути её решения были описаны в одной из наших прошлых статей "Почему важно проверять, что вернула функция malloc" и комментариях к ней.

Анализатор также выявил похожие подозрительные фрагменты кода:

• V522 There might be dereferencing of a potential null pointer 'jsn'. Check lines: 117, 113. jsonsl.c:117
• V522 There might be dereferencing of a potential null pointer 'i'. DetailsDialog.cc:133
• V522 There might be dereferencing of a potential null pointer. TorrentFilter.cc:320

libtorrent

На этом, пожалуй, закончим с Transmission и посмотрим, что же интересного нашлось в проекте libtorrent.

Фрагмент N1: недостаточная проверка индексов массивов

template <typename Handler>
void handshake2(error_code const& e, Handler h)
{
  ...
  std::size_t const read_pos = m_buffer.size();
  ...
  if (m_buffer[read_pos - 1] == '\n' && read_pos > 2) // <=
  {
    if (m_buffer[read_pos - 2] == '\n')
    {
      found_end = true;
    }
    else if (read_pos > 4
      && m_buffer[read_pos - 2] == '\r'
      && m_buffer[read_pos - 3] == '\n'
      && m_buffer[read_pos - 4] == '\r')
    {
      found_end = true;
    }
  }
  ...
}

Предупреждение V781 The value of the 'read_pos' index is checked after it was used. Perhaps there is a mistake in program logic. http_stream.hpp:166.

Классическая ошибка, в которой разработчик сначала пытается получить элемент массива m_buffer по индексу read_pos — 1, а уже потом read_pos проверяется на корректность (read_pos > 2). Сложно сказать, что в данном случае произойдёт на практике: возможно, будет прочитана другая переменная, а может, вообще возникнет Access Violation. Всё-таки неопределенное поведение (Undefined Behavior) не просто так назвали неопределенным :) Правильным решением здесь будет поменять эти действия местами:

if (read_pos > 2 && m_buffer[read_pos - 1] == '\n')

Фрагмент N2, N3: перезаписывание значений

void dht_tracker::dht_status(session_status& s)
{
  s.dht_torrents += int(m_storage.num_torrents());    // <=

  s.dht_nodes = 0;
  s.dht_node_cache = 0;
  s.dht_global_nodes = 0;
  s.dht_torrents = 0;                                 // <=
  s.active_requests.clear();
  s.dht_total_allocations = 0;

  for (auto& n : m_nodes)
    n.second.dht.status(s);
}

Предупреждение V519 The 's.dht_torrents' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 205, 210. dht_tracker.cpp 210.

В вышеприведённом фрагменте два раза изменяется переменная s.dht_torrents: в первый раз ей назначается значение, а через несколько строк оно обнуляется без использования между присвоениями. Т. е. мы имеем дело с так называемой тупиковой записью (Dead Store). Сложно сказать, как на самом деле должен выглядеть код, т. к. тип session_status содержит большое количество полей. Возможно, одно из присвоений здесь лишнее или случайно обнуляется не та переменная.

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

void torrent::bytes_done(torrent_status& st, status_flags_t const flags) const
{
  ...
  st.total_done = 0;
  st.total_wanted_done = 0;
  st.total_wanted = m_size_on_disk;
  ...
  if (m_seed_mode || is_seed())
  {
    st.total_done = m_torrent_file->total_size() - m_padding_bytes;
    st.total_wanted_done = m_size_on_disk;
    st.total_wanted = m_size_on_disk;
    ...
    return;
  }
  else if (!has_picker())
  {
    st.total_done = 0;
    st.total_wanted_done = 0;
    st.total_wanted = m_size_on_disk;
    return;
  }
  ...
}

Предупреждения PVS-Studio:

• V1048 The 'st.total_wanted' variable was assigned the same value. torrent.cpp 3784
• V1048 The 'st.total_done' variable was assigned the same value. torrent.cpp 3792
• V1048 The 'st.total_wanted_done' variable was assigned the same value. torrent.cpp 3793
• V1048 The 'st.total_wanted' variable was assigned the same value. torrent.cpp 3794

Фрагмент N4: неудачное явное приведение типа

void torrent::get_download_queue(std::vector<partial_piece_info>* queue) const
{
  ...
  const int blocks_per_piece = m_picker->blocks_in_piece(piece_index_t(0));
  ...
  int counter = 0;
  for (auto i = q.begin(); i != q.end(); ++i, ++counter)
  {
    partial_piece_info pi;
    ...
    pi.blocks = &blk[std::size_t(counter * blocks_per_piece)];
  }
}

Предупреждение V1028 Possible overflow. Consider casting operands of the 'counter * blocks_per_piece' operator to the 'size_t' type, not the result. torrent.cpp 7092

В данном случае явное приведение типа к size_t используется для корректного доступа к элементам массива. Проблема здесь в том, что оба операнда являются знаковыми целыми числами и при их перемножении может возникнуть переполнение. Очень часто такой код можно встретить, когда разработчики пытаются по-быстрому просто заглушить предупреждение компилятора, но при этом не задумываются, что только плодят ошибки. В данном случае, чтобы исправить проблему, достаточно привести хотя бы один операнд к типу size_t. Примерно так:

pi.blocks = &blk[std::size_t(counter) * blocks_per_piece];

Похожие проблемы также были найдены в следующих фрагментах:

• V1028 Possible overflow. Consider casting operands of the 'new_size_words + 1' operator to the 'size_t' type, not the result. bitfield.cpp 179
• V1028 Possible overflow. Consider casting operands of the 'm_capacity + amount_to_grow' operator to the 'size_t' type, not the result. heterogeneous_queue.hpp 207

Фрагмент N5: лишние условия

В проекте libtorrent так же, как и в Transmission, нашлось немало предупреждений, связанных с лишними условиями. Ложными их назвать нельзя, но перечислять их нет смысла, потому что интересного в этом мало. Чтобы было понятно, что именно я имею в виду, приведу такой фрагмент кода:

char const* operation_name(operation_t const op)
  {
    ...
    static char const* const names[] = {
      ...
    };

    int const idx = static_cast<int>(op);
    if (idx < 0 || idx >= int(sizeof(names) / sizeof(names[0])))
      return "unknown operation";
    return names[idx];
}

Предупреждение V560 A part of conditional expression is always false: idx < 0. alert.cpp 1885.

В нём анализатор справедливо предупреждает, что условие idx < 0 не имеет смысла, т. к. переменная index получает значение из перечисления, в котором лишь беззнаковые целые числа:

enum class operation_t : std::uint8_t

Стоит ли обращать внимание на подобные предупреждения? Я думаю, что на этот случай у каждого разработчика найдётся своё мнение. Кто-то скажет, что исправлять их нет смысла, потому что на реальные ошибки они не указывают, а кто-то, напротив, скажет, что не нужно засорять код. Я же считаю, что подобного рода диагностики – отличная возможность найти хорошие места для будущего рефакторинга.

Заключение

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

Ну и в конце хотелось бы еще раз напомнить, что команда PVS-Studio любит и активно поддерживает проекты с открытым исходным кодом. Именно поэтому мы не только сообщаем о найденных багах разработчикам, но и даём им возможность использовать PVS-Studio бесплатно. Также будет не лишним напомнить про бесплатные лицензии для студентов и преподавателей. В случае коммерческих проектов предлагаем скачать и попробовать анализатор PVS-Studio, запросив ознакомительную лицензию на нашем веб-сайте :)

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Mikhail Gelvikh. Checking BitTorrent in honor of the 20th anniversary. Time == quality.