Static code analyzer

Один из самых актуальных сценариев использования анализатора PVS-Studio его интеграция с CI системами. И хотя анализ проекта PVS-Studio практически из-под любой continuous integration системы можно встроить всего в несколько команд, мы продолжаем делать этот процесс ещё удобнее. В PVS-Studio появилась поддержка преобразования вывода анализатора в формат для TeamCity TeamCity Inspections Type. Давайте посмотрим, как это работает.

Информация об используемом ПО

PVS-Studio статический анализатор С, С++, C# и Java кода, предназначенный для облегчения задачи поиска и исправления различного рода ошибок. Анализатор можно использовать в Windows, Linux и macOS. В данной статье мы будем активно использовать не только сам анализатор, но и некоторые утилиты из его дистрибутива.

CLMonitor представляет собой сервер мониторинга, который осуществляет отслеживание запусков компиляторов. Его необходимо запустить непосредственно перед началом сборки вашего проекта. В режиме отслеживания сервер будет перехватывать запуски всех поддерживаемых компиляторов. Стоит отметить, что данную утилиту можно использовать только для анализа C/С++ проектов.

PlogConverter утилита для конвертации отчёта анализатора в разные форматы.

Информация об исследуемом проекте

Давайте попробуем данную функциональность на практическом примере проанализируем проект OpenRCT2.

OpenRCT2 открытая реализация игры RollerCoaster Tycoon 2 (RCT2), расширяющая её новыми функциями и исправляющая ошибки. Игровой процесс вращается вокруг строительства и содержания парка развлечений, в котором находятся аттракционы, магазины и объекты. Игрок должен постараться получить прибыль и поддерживать хорошую репутацию парка, сохраняя при этом гостей счастливыми. OpenRCT2 позволяет играть как в сценарии, так и в песочнице. Сценарии требуют, чтобы игрок выполнил определенную задачу в установленное время, в то время как песочница позволяет игроку построить более гибкий парк без каких-либо ограничений или финансов.

Настройка

В целях экономии времени я, пожалуй, опущу процесс установки и начну с того момента, когда у меня на компьютере запущен сервер TeamCity. Нам нужно перейти: localhost:{указанный в процессе установки порт}(в моём случае, localhost:9090) и ввести данные для авторизации. После входа нас встретит:


Нажмём на кнопку Create Project. Далее выберем Manually, заполним поля.


После нажатия на кнопку Create, нас встречает окно с настройками.


Нажмём Create build configuration.


Заполняем поля, нажимаем Create. Мы видим окно с предложением выбора системы контроля версий. Так как исходники уже лежат локально, жмём Skip.


Наконец, мы переходим к настройкам проекта.


Добавим шаги сборки, для этого жмём: Build steps -> Add build step.


Тут выберем:

• Runner type -> Command Line
• Run -> Custom Script

Так как мы будем проводить анализ во время компиляции проекта, сборка и анализ должны быть одним шагом, поэтому заполним поле Custom Script:


На отдельных шагах мы остановимся позже. Важно, чтобы загрузка анализатора, сборка проекта, его анализ, вывод отчёта и его форматирование заняло всего одиннадцать строк кода.

Последнее, что нам нужно сделать, установить переменные окружения, которыми я обозначил некоторые пути для улучшения их читабельности. Для этого перейдём: Parameters -> Add new parameter и добавим три переменные:


Остаётся нажать на кнопку Run в правом верхнем углу. Пока идёт сборка и анализ проекта расскажу вам о скрипте.

Непосредственно скрипт

Для начала нам нужно выкачать свежий дистрибутив PVS-Studio. Для этого мы используем пакетный менеджер Сhocolatey. Для тех, кто хочет узнать об этом поподробнее, есть соответствующая статья:

choco install pvs-studio -y

Далее запустим утилиту отслеживания сборки проекта CLMonitor.

%CLmon% monitor -attach

Потом произведём сборку проекта, в качестве переменной окружения MSB выступает путь к нужной мне для сборки версии MSBuild

%MSB% %ProjPath% /t:clean%MSB% %ProjPath% /t:rebuild /p:configuration=release%MSB% %ProjPath% /t:g2%MSB% %ProjPath% /t:PublishPortable

Введём логин и ключ лицензии PVS-Studio:

%PVS-Studio_cmd% credentials --username %PVS_Name% --serialNumber %PVS_Key%

После завершения сборки ещё раз запустим CLMonitor для генерации препроцессированных файлов и статического анализа:

%CLmon% analyze -l "c:\ptest.plog"

После воспользуемся ещё одной утилитой из нашего дистрибутива. PlogConverter преобразует отчёт из стандартного в специфичный для TeamCity формат. Благодаря этому мы сможем посмотреть его прямо в окне сборки.

%PlogConverter% "c:\ptest.plog" --renderTypes=TeamCity -o "C:\temp"

Последним действием выведем форматированный отчёт в stdout, где его подхватит парсер TeamCity.

type "C:\temp\ptest.plog_TeamCity.txt"

Полный код скрипта:

choco install pvs-studio -y%CLmon% monitor --attachset platform=x64%MSB% %ProjPath% /t:clean%MSB% %ProjPath% /t:rebuild /p:configuration=release%MSB% %ProjPath% /t:g2%MSB% %ProjPath% /t:PublishPortable%PVS-Studio_cmd% credentials --username %PVS_Name% --serialNumber %PVS_Key%%CLmon% analyze -l "c:\ptest.plog"%PlogConverter% "c:\ptest.plog" --renderTypes=TeamCity -o "C:\temp"type "C:\temp\ptest.plog_TeamCity.txt"

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


Теперь кликнем на Inspections Total, чтоб перейти к просмотру отчёта анализатора:


Предупреждения сгруппированы по номерам диагностических правил. Для осуществления навигации по коду нужно кликнуть на номер строки с предупреждением. Нажатие на знак вопроса в правом верхнем углу откроет вам новую вкладку с документацией. Также можно осуществить навигацию по коду, нажав на номер строки с предупреждением анализатора. Навигация с удалённого компьютера возможна при применении SourceTreeRoot маркера. Тот, кому интересен данный режим работы анализатора, может ознакомиться с соответствующим разделом документации.

Просмотр результатов работы анализатора

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

Предупреждение N1

V773 [CWE-401] The exception was thrown without releasing the 'result' pointer. A memory leak is possible. libopenrct2 ObjectFactory.cpp 443

Object* CreateObjectFromJson(....){  Object* result = nullptr;  ....  result = CreateObject(entry);  ....  if (readContext.WasError())  {    throw std::runtime_error("Object has errors");  }  ....}Object* CreateObject(const rct_object_entry& entry){  Object* result;  switch (entry.GetType())  {    case OBJECT_TYPE_RIDE:      result = new RideObject(entry);      break;    case OBJECT_TYPE_SMALL_SCENERY:      result = new SmallSceneryObject(entry);      break;    case OBJECT_TYPE_LARGE_SCENERY:      result = new LargeSceneryObject(entry);      break;    ....    default:      throw std::runtime_error("Invalid object type");  }  return result;}

Анализатор заметил ошибку, заключающуюся в том, что после динамического выделения памяти в CreateObject, при возникновении исключения память не очищается, соответственно, возникает утечка памяти.

Предупреждение N2

V501 There are identical sub-expressions '(1ULL << WIDX_MONTH_BOX)' to the left and to the right of the '|' operator. libopenrct2ui Cheats.cpp 487

static uint64_t window_cheats_page_enabled_widgets[] = {  MAIN_CHEAT_ENABLED_WIDGETS |  (1ULL << WIDX_NO_MONEY) |  (1ULL << WIDX_ADD_SET_MONEY_GROUP) |  (1ULL << WIDX_MONEY_SPINNER) |  (1ULL << WIDX_MONEY_SPINNER_INCREMENT) |  (1ULL << WIDX_MONEY_SPINNER_DECREMENT) |  (1ULL << WIDX_ADD_MONEY) |  (1ULL << WIDX_SET_MONEY) |  (1ULL << WIDX_CLEAR_LOAN) |  (1ULL << WIDX_DATE_SET) |  (1ULL << WIDX_MONTH_BOX) |  // <=  (1ULL << WIDX_MONTH_UP) |  (1ULL << WIDX_MONTH_DOWN) |  (1ULL << WIDX_YEAR_BOX) |  (1ULL << WIDX_YEAR_UP) |  (1ULL << WIDX_YEAR_DOWN) |  (1ULL << WIDX_DAY_BOX) |  (1ULL << WIDX_DAY_UP) |  (1ULL << WIDX_DAY_DOWN) |  (1ULL << WIDX_MONTH_BOX) |  // <=  (1ULL << WIDX_DATE_GROUP) |  (1ULL << WIDX_DATE_RESET),  ....};

Мало кто, кроме статического анализатора, смог бы пройти данный тест на внимательность. Данный пример копипасты хорош именно этим.

Предупреждения N3

V703 It is odd that the 'flags' field in derived class 'RCT12BannerElement' overwrites field in base class 'RCT12TileElementBase'. Check lines: RCT12.h:570, RCT12.h:259. libopenrct2 RCT12.h 570

struct RCT12SpriteBase{  ....  uint8_t flags;  ....};struct rct1_peep : RCT12SpriteBase{  ....  uint8_t flags;  ....};

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

Предупреждение N4

V793 It is odd that the result of the 'imageDirection / 8' statement is a part of the condition. Perhaps, this statement should have been compared with something else. libopenrct2 ObservationTower.cpp 38

void vehicle_visual_observation_tower(...., int32_t imageDirection, ....){  if ((imageDirection / 8) && (imageDirection / 8) != 3)  {    ....  }  ....}

Давайте разберёмся поподробнее. Выражение imageDirection / 8 будет false в том случае, если imageDirection находится в диапазоне от -7 до 7. Вторая часть: (imageDirection / 8) != 3 проверяет imageDirection на нахождение вне диапазона: от -31 до -24 и от 24 до 31 соответственно. Мне кажется довольно странным проверять числа на вхождение в определённый диапазон таким способом и, даже если в данном фрагменте кода нет ошибки, я бы рекомендовал переписать данные условия на более явные. Это существенно упростило бы жизнь людям, которые будут читать и поддерживать этот код.

Предупреждение N5

V587 An odd sequence of assignments of this kind: A = B; B = A;. Check lines: 1115, 1118. libopenrct2ui MouseInput.cpp 1118

void process_mouse_over(....){  ....  switch (window->widgets[widgetId].type)  {    case WWT_VIEWPORT:      ebx = 0;      edi = cursorId;                                 // <=      // Window event WE_UNKNOWN_0E was called here,      // but no windows actually implemented a handler and      // it's not known what it was for      cursorId = edi;                                 // <=      if ((ebx & 0xFF) != 0)      {        set_cursor(cursorId);        return;      }      break;      ....  }  ....}

Данный фрагмент кода, скорее всего, был получен путем декомпиляции. Затем, судя по оставленному комментарию, была удалена часть нерабочего кода. Однако осталась пара операций над cursorId, которые также не несут особого смысла.

Предупреждение N6

V1004 [CWE-476] The 'player' pointer was used unsafely after it was verified against nullptr. Check lines: 2085, 2094. libopenrct2 Network.cpp 2094

void Network::ProcessPlayerList(){  ....  auto* player = GetPlayerByID(pendingPlayer.Id);  if (player == nullptr)  {    // Add new player.    player = AddPlayer("", "");    if (player)                                          // <=    {      *player = pendingPlayer;       if (player->Flags & NETWORK_PLAYER_FLAG_ISSERVER)       {         _serverConnection->Player = player;       }    }    newPlayers.push_back(player->Id);                    // <=  }  ....}

Данный код поправить довольно просто, нужно или третий раз проверять player на нулевой указатель, либо внести его в тело условного оператора. Я бы предложил второй вариант:

void Network::ProcessPlayerList(){  ....  auto* player = GetPlayerByID(pendingPlayer.Id);  if (player == nullptr)  {    // Add new player.    player = AddPlayer("", "");    if (player)    {      *player = pendingPlayer;      if (player->Flags & NETWORK_PLAYER_FLAG_ISSERVER)      {        _serverConnection->Player = player;      }      newPlayers.push_back(player->Id);    }  }  ....}

Предупреждение N7

V547 [CWE-570] Expression 'name == nullptr' is always false. libopenrct2 ServerList.cpp 102

std::optional<ServerListEntry> ServerListEntry::FromJson(...){  auto name = json_object_get(server, "name");  .....  if (name == nullptr || version == nullptr)  {    ....  }  else  {    ....    entry.name = (name == nullptr ? "" : json_string_value(name));    ....  }  ....}

Можно одним махом избавиться от трудночитаемой строки кода и решить проблему с проверкой на nullptr. Предлагаю изменить код следующим образом:

std::optional<ServerListEntry> ServerListEntry::FromJson(...){  auto name = json_object_get(server, "name");  .....  if (name == nullptr || version == nullptr)  {    name = ""    ....  }  else  {    ....    entry.name = json_string_value(name);    ....  }  ....}

Предупреждение N8

V1048 [CWE-1164] The 'ColumnHeaderPressedCurrentState' variable was assigned the same value. libopenrct2ui CustomListView.cpp 510

void CustomListView::MouseUp(....){  ....  if (!ColumnHeaderPressedCurrentState)  {    ColumnHeaderPressed = std::nullopt;    ColumnHeaderPressedCurrentState = false;    Invalidate();  }}

Код выглядит довольно странно. Мне кажется, имела место быть опечатка либо в условии, либо при повторном присвоении переменной ColumnHeaderPressedCurrentState значения false.

Вывод

Как мы видим, интегрировать статический анализатор PVS-Studio в свой проект на TeamCity довольно просто. Для этого достаточно написать всего один маленький файл конфигурации. Проверка кода же позволит выявлять проблемы сразу после сборки, что поможет устранять их тогда, когда сложность и стоимость правок ещё малы.


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Vladislav Stolyarov. PVS-Studio and Continuous Integration: TeamCity. Analysis of the Open RollerCoaster Tycoon 2 project.

Статический анализ кода показывает наибольшую эффективность во время внесения изменений в проект, поскольку ошибки всегда сложнее исправлять в будущем, чем не допустить их появления на ранних этапах. Мы продолжаем расширять варианты использования 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".


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


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


В качестве агента будет использован 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/runtimeSHELL ["cmd", "/S", "/C"]ADD https://aka.ms/vs/16/release/vs_buildtools.exe C:\vs_buildtools.exeRUN C:\vs_buildtools.exe --quiet --wait --norestart --nocache `  --installPath C:\BuildTools `  --add Microsoft.VisualStudio.Workload.VCTools `  --includeRecommendedRUN 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 ./agentUSER ContainerAdministratorRUN 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 /fADD .\entrypoint.ps1 C:\entrypoint.ps1SHELL ["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

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

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

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

trigger: nonepr:  branches:    include:    - '*'pool: Defaultsteps:- 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 и будет выполнен на агентах, назначенных в пул по умолчанию. Необходимо только дать ему разрешение на работу с этим пулом.




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

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

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




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

Некоторые ошибки, найденные в 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.