27.07.2020 17:20
MrROBUST 2 740 Источник


Статический анализ кода показывает наибольшую эффективность во время внесения изменений в проект, поскольку ошибки всегда сложнее исправлять в будущем, чем не допустить их появления на ранних этапах. Мы продолжаем расширять варианты использования PVS-Studio в системах непрерывной разработки и покажем, как настроить анализ pull request-ов при помощи self-hosted агентов в Microsoft Azure DevOps, на примере игры Minetest.

Вкратце о том, с чем мы имеем дело


Minetest — это открытый кроссплатформенный игровой движок, содержащий около 200 тысяч строк кода на C, C++ и Lua. Он позволяет создавать разные игровые режимы в воксельном пространстве. Поддерживает мультиплеер, и множество модов от сообщества. Репозиторий проекта размещен здесь: https://github.com/minetest/minetest.

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

PVS-Studio — статический анализатор кода на языках C, C++, C# и Java для поиска ошибок и дефектов безопасности.

Azure DevOps — облачная платформа, предоставляющая возможность разработки, выполнения приложений и хранения данных на удаленных серверах.

Для выполнения задач разработки в Azure возможно использование виртуальных машин-агентов Windows и Linux. Однако запуск агентов на локальном оборудовании имеет несколько весомых преимуществ:

  • У локального хоста может быть больше ресурсов, чем у ВМ Azure;
  • Агент не "исчезает" после выполнения своей задачи;
  • Возможность прямой настройки окружения, и более гибкое управление процессами сборки;
  • Локальное хранение промежуточных файлов положительно влияет на скорость сборки;
  • Можно бесплатно выполнять более 30 задач в месяц.

Подготовка к использованию self-hosted агента


Процесс начала работы в Azure подробно описан в статье "PVS-Studio идёт в облака: Azure DevOps", поэтому перейду сразу к созданию self-hosted агента.

Для того, чтобы агенты имели право подключиться к пулам проекта, им нужен специальный Access Token. Получить его можно на странице "Personal Access Tokens", в меню "User settings".

image2.png

После нажатия на "New token" необходимо указать имя и выбрать Read & manage Agent Pools (может понадобиться развернуть полный список через "Show all scopes").

image3.png

Нужно скопировать токен, так как Azure больше его не покажет, и придется делать новый.

image4.png

В качестве агента будет использован Docker контейнер на основе Windows Server Core. Хостом является мой рабочий компьютер на Windows 10 x64 с Hyper-V.

Сначала понадобится расширить объём дискового пространства, доступный Docker контейнерам.

В Windows для этого нужно модифицировать файл 'C:\ProgramData\Docker\config\daemon.json' следующим образом:

{
  "registry-mirrors": [],
  "insecure-registries": [],
  "debug": true,
  "experimental": false,
  "data-root": "d:\\docker",
  "storage-opts": [ "size=40G" ]
}

Для создания Docker образа для агентов со сборочной системой и всем необходимым в директории 'D:\docker-agent' добавим Docker файл с таким содержимым:

# escape=`

FROM mcr.microsoft.com/dotnet/framework/runtime

SHELL ["cmd", "/S", "/C"]

ADD https://aka.ms/vs/16/release/vs_buildtools.exe C:\vs_buildtools.exe
RUN C:\vs_buildtools.exe --quiet --wait --norestart --nocache `
  --installPath C:\BuildTools `
  --add Microsoft.VisualStudio.Workload.VCTools `
  --includeRecommended

RUN powershell.exe -Command `
  Set-ExecutionPolicy Bypass -Scope Process -Force; `
  [System.Net.ServicePointManager]::SecurityProtocol =
    [System.Net.ServicePointManager]::SecurityProtocol -bor 3072; `
  iex ((New-Object System.Net.WebClient)
    .DownloadString('https://chocolatey.org/install.ps1')); `
  choco feature enable -n=useRememberedArgumentsForUpgrades;
  
RUN powershell.exe -Command `
  choco install -y cmake --installargs '"ADD_CMAKE_TO_PATH=System"'; `
  choco install -y git --params '"/GitOnlyOnPath /NoShellIntegration"'

RUN powershell.exe -Command `
  git clone https://github.com/microsoft/vcpkg.git; `
  .\vcpkg\bootstrap-vcpkg -disableMetrics; `
  $env:Path += '";C:\vcpkg"'; `
  [Environment]::SetEnvironmentVariable(
    '"Path"', $env:Path, [System.EnvironmentVariableTarget]::Machine); `
  [Environment]::SetEnvironmentVariable(
    '"VCPKG_DEFAULT_TRIPLET"', '"x64-windows"',
  [System.EnvironmentVariableTarget]::Machine)

RUN powershell.exe -Command `
  choco install -y pvs-studio; `
  $env:Path += '";C:\Program Files (x86)\PVS-Studio"'; `
  [Environment]::SetEnvironmentVariable(
    '"Path"', $env:Path, [System.EnvironmentVariableTarget]::Machine)

RUN powershell.exe -Command `
  $latest_agent =
    Invoke-RestMethod -Uri "https://api.github.com/repos/Microsoft/
                          azure-pipelines-agent/releases/latest"; `
  $latest_agent_version =
    $latest_agent.name.Substring(1, $latest_agent.tag_name.Length-1); `
  $latest_agent_url =
    '"https://vstsagentpackage.azureedge.net/agent/"' + $latest_agent_version +
  '"/vsts-agent-win-x64-"' + $latest_agent_version + '".zip"'; `
  Invoke-WebRequest -Uri $latest_agent_url -Method Get -OutFile ./agent.zip; `
  Expand-Archive -Path ./agent.zip -DestinationPath ./agent

USER ContainerAdministrator
RUN reg add hklm\system\currentcontrolset\services\cexecsvc
        /v ProcessShutdownTimeoutSeconds /t REG_DWORD /d 60  
RUN reg add hklm\system\currentcontrolset\control
        /v WaitToKillServiceTimeout /t REG_SZ /d 60000 /f

ADD .\entrypoint.ps1 C:\entrypoint.ps1
SHELL ["powershell", "-Command",
       "$ErrorActionPreference = 'Stop';
     $ProgressPreference = 'SilentlyContinue';"]
ENTRYPOINT .\entrypoint.ps1

В результате получится сборочная система на основе MSBuild для C++, с Chocolatey для установки PVS-Studio, CMake и Git. Для удобного управления библиотеками, от которых зависит проект, собирается Vcpkg. А также скачивается свежая версия, собственно, Azure Pipelines Agent.

Для инициализации агента из ENTRYPOINT-а Docker файла вызывается PowerShell скрипт 'entrypoint.ps1', в который нужно добавить URL "организации" проекта, токен пула агентов, и параметры лицензии PVS-Studio:

$organization_url = "https://dev.azure.com/<аккаунт Microsoft Azure>"
$agents_token = "<token агента>"

$pvs_studio_user = "<имя пользователя PVS-Studio>"
$pvs_studio_key = "<ключ PVS-Studio>"

try
{
  C:\BuildTools\VC\Auxiliary\Build\vcvars64.bat

  PVS-Studio_Cmd credentials -u $pvs_studio_user -n $pvs_studio_key
  
  .\agent\config.cmd --unattended `
    --url $organization_url `
    --auth PAT `
    --token $agents_token `
    --replace;
  .\agent\run.cmd
} 
finally
{
  # Agent graceful shutdown
  # https://github.com/moby/moby/issues/25982
  
  .\agent\config.cmd remove --unattended `
    --auth PAT `
    --token $agents_token
}

Команды для сборки образа и запуска агента:

docker build -t azure-agent -m 4GB .
docker run -id --name my-agent -m 4GB --cpu-count 4 azure-agent

image5.png

Агент запущен и готов выполнять задачи.

image6.png

Запуск анализа на self-hosted агенте


Для анализа PR создается новый pipeline со следующим скриптом:

image7.png

trigger: none

pr:
  branches:
    include:
    - '*'

pool: Default

steps:
- script: git diff --name-only
    origin/%SYSTEM_PULLREQUEST_TARGETBRANCH% >
    diff-files.txt
  displayName: 'Get committed files'

- script: |
    cd C:\vcpkg
    git pull --rebase origin
    CMD /C ".\bootstrap-vcpkg -disableMetrics"
    vcpkg install ^
    irrlicht zlib curl[winssl] openal-soft libvorbis ^
    libogg sqlite3 freetype luajit
    vcpkg upgrade --no-dry-run
  displayName: 'Manage dependencies (Vcpkg)'

- task: CMake@1
  inputs:
    cmakeArgs: -A x64
      -DCMAKE_TOOLCHAIN_FILE=C:/vcpkg/scripts/buildsystems/vcpkg.cmake
      -DCMAKE_BUILD_TYPE=Release -DENABLE_GETTEXT=0 -DENABLE_CURSES=0 ..
  displayName: 'Run CMake'

- task: MSBuild@1
  inputs:
    solution: '**/*.sln'
    msbuildArchitecture: 'x64'
    platform: 'x64'
    configuration: 'Release'
    maximumCpuCount: true
  displayName: 'Build'

- script: |
    IF EXIST .\PVSTestResults RMDIR /Q/S .\PVSTestResults
    md .\PVSTestResults
    PVS-Studio_Cmd ^
    -t .\build\minetest.sln ^
    -S minetest ^
    -o .\PVSTestResults\minetest.plog ^
    -c Release ^
    -p x64 ^
    -f diff-files.txt ^
    -D C:\caches
    PlogConverter ^
    -t FullHtml ^
    -o .\PVSTestResults\ ^
    -a GA:1,2,3;64:1,2,3;OP:1,2,3 ^
    .\PVSTestResults\minetest.plog
    IF NOT EXIST "$(Build.ArtifactStagingDirectory)" ^
    MKDIR "$(Build.ArtifactStagingDirectory)"
    powershell -Command ^
    "Compress-Archive -Force ^
    '.\PVSTestResults\fullhtml' ^
    '$(Build.ArtifactStagingDirectory)\fullhtml.zip'"
  displayName: 'PVS-Studio analyze'
  continueOnError: true

- task: PublishBuildArtifacts@1
  inputs:
    PathtoPublish: '$(Build.ArtifactStagingDirectory)'
    ArtifactName: 'psv-studio-analisys'
    publishLocation: 'Container'
  displayName: 'Publish analysis report'

Этот скрипт сработает при получении PR и будет выполнен на агентах, назначенных в пул по умолчанию. Необходимо только дать ему разрешение на работу с этим пулом.

image8.png


image9.png

В скрипте происходит сохранение списка измененных файлов, полученного при помощи git diff. Затем обновляются зависимости, генерируется solution проекта через CMake, и производится его сборка.

Если сборка прошла успешно, запускается анализ изменившихся файлов (флаг '-f diff-files.txt'), игнорируя созданные CMake вспомогательные проекты (выбираем только нужный проект флагом '-S minetest'). Для ускорения поиска связей между заголовочными и исходными C++ файлами создается специальный кэш, который будет храниться в отдельной директории (флаг '-D C:\caches').

Таким образом, мы теперь можем получать отчеты об анализе изменений в проекте.

image10.png


image11.png

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

image13.png

Некоторые ошибки, найденные в Minetest


Затирание результата

V519 The 'color_name' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 621, 627. string.cpp 627

static bool parseNamedColorString(const std::string &value,
                                  video::SColor &color)
{
  std::string color_name;
  std::string alpha_string;

  size_t alpha_pos = value.find('#');
  if (alpha_pos != std::string::npos) {
    color_name = value.substr(0, alpha_pos);
    alpha_string = value.substr(alpha_pos + 1);
  } else {
    color_name = value;
  }

  color_name = lowercase(value); // <=

  std::map<const std::string, unsigned>::const_iterator it;
  it = named_colors.colors.find(color_name);
  if (it == named_colors.colors.end())
    return false;
  ....
}

Эта функция должна производить разбор названия цвета с параметром прозрачности (например, Green#77) и вернуть его код. В зависимости от результата проверки условия в переменную color_name передается результат разбиения строки либо копия аргумента функции. Однако затем в нижний регистр переводится не сама полученная строка, а исходный аргумент. В результате её не удастся найти в словаре цветов при наличии параметра прозрачности. Можем исправить эту строку так:

color_name = lowercase(color_name);

Лишние проверки условий

V547 Expression 'nearest_emergefull_d == -1' is always true. clientiface.cpp 363

void RemoteClient::GetNextBlocks (....)
{
  ....
  s32 nearest_emergefull_d = -1;
  ....
  s16 d;
  for (d = d_start; d <= d_max; d++) {
    ....
      if (block == NULL || surely_not_found_on_disk || block_is_invalid) {
        if (emerge->enqueueBlockEmerge(peer_id, p, generate)) {
          if (nearest_emerged_d == -1)
            nearest_emerged_d = d;
        } else {
          if (nearest_emergefull_d == -1) // <=
            nearest_emergefull_d = d;
          goto queue_full_break;
        }
  ....
  }
  ....
queue_full_break:
  if (nearest_emerged_d != -1) { // <=
    new_nearest_unsent_d = nearest_emerged_d;
  } else ....
}

Переменная nearest_emergefull_d в процессе работы цикла не меняется, и её проверка не влияет на ход выполнения алгоритма. Либо это результат неаккуратного copy-paste, либо с ней забыли провести какие-то вычисления.

V560 A part of conditional expression is always false: y > max_spawn_y. mapgen_v7.cpp 262

int MapgenV7::getSpawnLevelAtPoint(v2s16 p)
{
  ....
  while (iters > 0 && y <= max_spawn_y) {               // <=
    if (!getMountainTerrainAtPoint(p.X, y + 1, p.Y)) {
      if (y <= water_level || y > max_spawn_y)          // <=
        return MAX_MAP_GENERATION_LIMIT; // Unsuitable spawn point

      // y + 1 due to biome 'dust'
      return y + 1;
    }
  ....
}

Значение переменной 'y' проверяется перед очередной итерацией цикла. Последующее, противоположное ей, сравнение всегда вернет false и, в целом, не влияет на результат проверки условия.

Потеря проверки указателя

V595 The 'm_client' pointer was utilized before it was verified against nullptr. Check lines: 183, 187. game.cpp 183

void gotText(const StringMap &fields)
{
  ....
  if (m_formname == "MT_DEATH_SCREEN") {
    assert(m_client != 0);
    m_client->sendRespawn();
    return;
  }

  if (m_client && m_client->modsLoaded())
    m_client->getScript()->on_formspec_input(m_formname, fields);
}

Перед обращением по указателю m_client проверяется не нулевой ли он при помощи макроса assert. Но это относится только к отладочной сборке. Такая мера предосторожности, при сборке в релиз, заменяется на пустышку, и появляется риск разыменования нулевого указателя.

Бит или не бит?

V616 The '(FT_RENDER_MODE_NORMAL)' named constant with the value of 0 is used in the bitwise operation. CGUITTFont.h 360

typedef enum  FT_Render_Mode_
{
  FT_RENDER_MODE_NORMAL = 0,
  FT_RENDER_MODE_LIGHT,
  FT_RENDER_MODE_MONO,
  FT_RENDER_MODE_LCD,
  FT_RENDER_MODE_LCD_V,

  FT_RENDER_MODE_MAX
} FT_Render_Mode;

#define FT_LOAD_TARGET_( x )   ( (FT_Int32)( (x) & 15 ) << 16 )
#define FT_LOAD_TARGET_NORMAL  FT_LOAD_TARGET_( FT_RENDER_MODE_NORMAL )

void update_load_flags()
{
  // Set up our loading flags.
  load_flags = FT_LOAD_DEFAULT | FT_LOAD_RENDER;
  if (!useHinting()) load_flags |= FT_LOAD_NO_HINTING;
  if (!useAutoHinting()) load_flags |= FT_LOAD_NO_AUTOHINT;
  if (useMonochrome()) load_flags |= 
    FT_LOAD_MONOCHROME | FT_LOAD_TARGET_MONO | FT_RENDER_MODE_MONO;
  else load_flags |= FT_LOAD_TARGET_NORMAL; // <=
}

Макрос FT_LOAD_TARGET_NORMAL разворачивается в ноль, и побитовое "или" не будет устанавливать никакие флаги в load_flags, ветка else может быть убрана.

Округление целочисленного деления

V636 The 'rect.getHeight() / 16' expression was implicitly cast from 'int' type to 'float' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. hud.cpp 771

void drawItemStack(....)
{
  float barheight = rect.getHeight() / 16;
  float barpad_x = rect.getWidth() / 16;
  float barpad_y = rect.getHeight() / 16;

  core::rect<s32> progressrect(
    rect.UpperLeftCorner.X + barpad_x,
    rect.LowerRightCorner.Y - barpad_y - barheight,
    rect.LowerRightCorner.X - barpad_x,
    rect.LowerRightCorner.Y - barpad_y);
}

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

Подозрительная последовательность операторов ветвления

V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. treegen.cpp 413

treegen::error make_ltree(...., TreeDef tree_definition)
{
  ....
  std::stack <core::matrix4> stack_orientation;
  ....
    if ((stack_orientation.empty() &&
      tree_definition.trunk_type == "double") ||
      (!stack_orientation.empty() &&
      tree_definition.trunk_type == "double" &&
      !tree_definition.thin_branches)) {
      ....
    } else if ((stack_orientation.empty() &&
      tree_definition.trunk_type == "crossed") ||
      (!stack_orientation.empty() &&
      tree_definition.trunk_type == "crossed" &&
      !tree_definition.thin_branches)) {
      ....
    } if (!stack_orientation.empty()) {                  // <=
  ....
  }
  ....
}

Здесь последовательности else-if в алгоритме генерации деревьев. Посередине очередной блок if находится на одной строке с закрывающей предыдущий else скобкой. Возможно, код работает правильно: до этого if-а создаются блоки ствола, а следом листьев; а может быть пропустили else. Наверняка это сказать может только разработчик.

Неправильная проверка выделения памяти

V668 There is no sense in testing the 'clouds' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. game.cpp 1367

bool Game::createClient(....)
{
  if (m_cache_enable_clouds) {
    clouds = new Clouds(smgr, -1, time(0));
    if (!clouds) {
      *error_message = "Memory allocation error (clouds)";
      errorstream << *error_message << std::endl;
      return false;
    }
  }
}

В случае, если new не сможет создать объект, будет брошено исключение std::bad_alloc, и оно должно быть обработано try-catch блоком. А проверка в таком виде бесполезна.

Чтение за границей массива

V781 The value of the 'i' index is checked after it was used. Perhaps there is a mistake in program logic. irrString.h 572

bool equalsn(const string<T,TAlloc>& other, u32 n) const
{
  u32 i;
  for(i=0; array[i] && other[i] && i < n; ++i) // <=
    if (array[i] != other[i])
      return false;

  // if one (or both) of the strings was smaller then they
  // are only equal if they have the same length
  return (i == n) || (used == other.used);
}

Происходит обращение к элементам массивов перед проверкой индекса, что может привести к ошибке. Возможно, стоит переписать цикл так:

for (i=0; i < n; ++i) // <=
  if (!array[i] || !other[i] || array[i] != other[i])
    return false;

Другие ошибки

Данная статья посвящена анализу pull request-ов в Azure DevOps и не ставит целью провести подробный обзор ошибок проекта Minetest. Здесь выписаны только некоторые фрагменты кода, которые мне показались интересными. Предлагаем авторам проекта не руководствоваться этой статьёй для исправления ошибок и выполнить более тщательный анализ предупреждений, которые выдаст PVS-Studio.

Заключение


Благодаря гибкой конфигурации в режиме командной строки, анализ PVS-Studio может быть встроен в самые разнообразные сценарии CI/CD. А правильное использование доступных ресурсов окупается увеличением производительности.

Нужно отметить, что режим проверки pull request-ов доступен только в Enterprise редакции анализатора. Чтобы получить демонстрационную Enterprise лицензию, укажите это в комментарии при запросе лицензии на странице скачивания. Более подробно о разнице между лицензиями можно узнать на странице заказа PVS-Studio.


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Alexey Govorov. PVS-Studio: analyzing pull requests in Azure DevOps using self-hosted agents.

Комментарии (2)