Это вторая статья про использование статического анализатора PVS-Studio в облачных CI-системах, и на этот раз мы рассмотрим платформу Azure DevOps – облачное CI\CD-решение от компании Microsoft. В качестве анализируемого проекта в этот раз рассмотрим ShareX.

Нам потребуется три компонента. Первый — статический анализатор PVS-Studio. Второй — Azure DevOps, с которой мы будем интегрировать анализатор. Третий — проект, который мы будем проверять для демонстрации возможностей PVS-Studio при работе в облаке. Итак, приступим.

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

Azure DevOps. В состав платформы Azure DevOps входят такие инструменты, как Azure Pipeline, Azure Board, Azure Artifacts и другие, позволяющие ускорить процесс создания программного обеспечения и повысить его качество.

ShareX – бесплатное приложение, позволяющее захватывать и записывать любую часть экрана. Проект написан на C# и отлично подходит для демонстрации настройки запуска статического анализатора. Исходный код проекта доступен на GitHub.

Вывод команды cloc для проекта ShareX:
Другими словами — проект маленький, но вполне достаточный, чтобы продемонстрировать на наём работу PVS-Studio в связке с облачной платформой.

Приступим к настройке

Для начала работы в Azure DevOps перейдем по ссылке и нажмём кнопку «Start free with GitHub».

Предоставим приложению Microsoft доступ к данным GitHub-аккаунта.

Для окончания регистрации придется создать аккаунт Microsoft.

После регистрации создадим проект:

Далее нам необходимо перейти в раздел «Pipelines» — «Builds» и создать новый Build pipeline

На вопрос, где расположен наш код, ответим – GitHub.

Авторизуем приложение Azure Pipelines и выберем репозиторий с проектом, для которого мы будем настраивать запуск статического анализатора

В окне выбора шаблона выберем «Starter pipeline».

Запускать статический анализ кода проекта мы можем двумя путями: используя Microsoft-hosted либо self-hosted агенты.

В первом варианте мы будем использовать Microsoft-hosted агенты. Такие агенты представляют собой обычные виртуальные машины, которые запускаются, когда мы запускаем наш pipeline, и удаляются после окончания задачи. Использование таких агентов позволяет не тратить время на их поддержку и обновление, но накладывает некоторые ограничения, например – невозможность установки дополнительного программного обеспечения, которое используется для сборки проекта.

Заменим предлагаемую нам конфигурацию по умолчанию на следующую для использования Microsoft-hosted агентов:

# Настройка триггеров запуска
# Запускаем для изменений только в master-ветке
trigger:
- master

# Так как установка произвольного ПО в виртуальные машины 
# запрещена, мы воспользуемся Docker-контейнером,
# запущенном в виртуальной машине с Windows Server 1803
pool:
  vmImage: 'win1803'
container: microsoft/dotnet-framework:4.7.2-sdk-windowsservercore-1803
           
steps:
# Скачиваем дистрибутив анализатора
- task: PowerShell@2
  inputs:
    targetType: 'inline'
    script: 'Invoke-WebRequest 
               -Uri https://files.viva64.com/PVS-Studio_setup.exe 
               -OutFile PVS-Studio_setup.exe'
- task: CmdLine@2
  inputs:
    workingDirectory: $(System.DefaultWorkingDirectory)
    script: |
# Восстанавливаем проект и скачиваем зависимости
      nuget restore .\ShareX.sln
# Создаем директорию, куда будут сохранены файлы с отчетами анализатора
      md .\PVSTestResults
# Устанавливаем анализатор
      PVS-Studio_setup.exe /VERYSILENT /SUPPRESSMSGBOXES 
                     /NORESTART /COMPONENTS=Core
# Создаем файл с настройками и лицензионной информацией
      "C:\Program Files (x86)\PVS-Studio\PVS-Studio_Cmd.exe" 
          credentials 
    -u $(PVS_USERNAME) 
    -n $(PVS_KEY)

# Запускаем статический анализатор и конвертируем отчет в html. 
      "C:\Program Files (x86)\PVS-Studio\PVS-Studio_Cmd.exe" 
         -t .\ShareX.sln  
         -o .\PVSTestResults\ShareX.plog
      "C:\Program Files (x86)\PVS-Studio\PlogConverter.exe" 
         -t html 
         -o .\PVSTestResults\ 
         .\PVSTestResults\ShareX.plog
    
# Сохраняем отчеты анализатора
- task: PublishBuildArtifacts@1
  inputs:
    pathToPublish: PVSTestResults
    artifactName: PVSTestResults

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

Сохраним pipeline и создадим переменные, которые будут использоваться для создания файла лицензии. Для этого откроем окно редактирования pipeline и в правом верхнем углу нажмем кнопку «Variables».

Добавим две переменные — PVS_USERNAME и PVS_KEY, содержащие имя пользователя и лицензионный ключ соответственно. При создании переменной PVS_KEY не забываем отметить пункт «Keep this value secret» для шифрования значения переменной 2048-битным RSA ключом, а также подавления вывода значения переменной в лог выполнения задачи.

Сохраняем переменные и запускаем pipeline кнопкой «Run».

Второй вариант запуска анализа – использовать self-hosted агент. Self-hosted агенты — это агенты, настраиваемые и управляемые нами самостоятельно. Такие агенты дают больше возможностей для установки программного обеспечения, которое необходимо для сборки и тестирования нашего программного продукта.

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

Для запуска задачи на self-hosted агенте заменим предлагаемую конфигурацию по умолчанию на следующую:

# Настройка триггеров запуска
# Запускаем анализ для master-ветки
trigger:
- master

# Задача запускается на self-hosted агенте из пула 'MyPool' 
pool: 'MyPool'

steps:
- task: CmdLine@2
  inputs:
    workingDirectory: $(System.DefaultWorkingDirectory)
    script: |
# Восстанавливаем проект и скачиваем зависимости
      nuget restore .\ShareX.sln
# Создаем директорию, куда будут сохранены файлы с отчетами анализатора
      md .\PVSTestResults
# Запускаем статический анализатор и конвертируем отчет в html. 
      "C:\Program Files (x86)\PVS-Studio\PVS-Studio_Cmd.exe" 
         -t .\ShareX.sln
         -o .\PVSTestResults\ShareX.plog
      "C:\Program Files (x86)\PVS-Studio\PlogConverter.exe"
         -t html
         -o .\PVSTestResults         .\PVSTestResults\ShareX.plog
# Сохраняем отчеты анализатора
- task: PublishBuildArtifacts@1
  inputs:
    pathToPublish: PVSTestResults
    artifactName: PVSTestResults

После окончания выполнения задачи архив с отчетами анализатора можно скачать на вкладке «Summary», либо же мы можем воспользоваться расширением Send Mail, позволяющим настроить отправку электронной почты, или поискать более удобный для нас инструмент на Marketplace.

О результатах анализа

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

Избыточные проверки

Для разминки давайте начнём с простых недочётов в коде, а именно — с избыточных проверок:

private void PbThumbnail_MouseMove(object sender, MouseEventArgs e)
{
  ....
  IDataObject dataObject 
    = new DataObject(DataFormats.FileDrop,
                     new string[] { Task.Info.FilePath });

  if (dataObject != null)
  {
    Program.MainForm.AllowDrop = false;
    dragBoxFromMouseDown = Rectangle.Empty;
    pbThumbnail.DoDragDrop(dataObject, 
        DragDropEffects.Copy | DragDropEffects.Move);
    Program.MainForm.AllowDrop = true;
  }
  ....
}

Предупреждение PVS-Studio: V3022 [CWE-571] Expression 'dataObject != null' is always true. TaskThumbnailPanel.cs 415

Обратим внимание на проверку переменной dataObject на null. Для чего она здесь? dataObject просто не может иметь значение null в данном случае, так как инициализируется ссылкой на создаваемый объект. В итоге имеем избыточную проверку. Критично? Нет. Выглядит лаконично? Нет. Эту проверку явно лучше убрать, чтобы не загромождать код.

Давайте взглянем ещё на один фрагмент кода, к которому можно предъявить аналогичные замечания:

private static Image GetDIBImage(MemoryStream ms)
{
  ....
  try
  {
    ....
    return new Bitmap(bmp);
    ....
  }
  finally
  {
    if (gcHandle != IntPtr.Zero)
    {
      GCHandle.FromIntPtr(gcHandle).Free();
    }
  }
  ....
}
private static Image GetImageAlternative()
{
  ....
  using (MemoryStream ms = dataObject.GetData(format) as MemoryStream)
  {
    if (ms != null)
    {
      try
      {
        Image img = GetDIBImage(ms);
        if (img != null)
        {
          return img;
        }
      }
      catch (Exception e)
      {
        DebugHelper.WriteException(e);
      }
    }
  }
  ....
}

Предупреждение PVS-Studio: V3022 [CWE-571] Expression 'img != null' is always true. ClipboardHelpers.cs 289

В методе GetImageAlternative опять выполняется проверка на то, что переменная img не имеет значения null сразу после того, как создали новый экземпляр класса Bitmap. Отличие от предыдущего примера тут состоит в том, что для инициализации переменной img мы используем не конструктор, а метод GetDIBImage. Автор кода предполагает, что в этом методе может возникнуть исключительная ситуация, но объявляет только блоки try и finally, опуская catch. Следовательно, если произойдёт исключение, то вызывающий метод — GetImageAlternative — не получит ссылку на объект типа Bitmap, а будет вынужден обрабатывать исключение в собственном блоке catch. В этом случае переменная img не будет проинициализирована, и поток исполнения даже не дойдёт до проверки img != null, а сразу попадёт в блок catch. Следовательно, анализатор действительно указал на избыточную проверку.

Рассмотрим следующий пример предупреждения с кодом V3022:

private void btnCopyLink_Click(object sender, EventArgs e)
{
  ....
  if (lvClipboardFormats.SelectedItems.Count == 0)
  {
    url = lvClipboardFormats.Items[0].SubItems[1].Text;
  }
  else if (lvClipboardFormats.SelectedItems.Count > 0)
  {
    url = lvClipboardFormats.SelectedItems[0].SubItems[1].Text;
  }
  ....
}

Предупреждение PVS-Studio: V3022 [CWE-571] Expression 'lvClipboardFormats.SelectedItems.Count > 0' is always true. AfterUploadForm.cs 155

Присмотримся ко второму условному выражению. Там мы проверяем значение свойства Count, доступного только для чтения. Данное свойство показывает количество элементов в экземпляре коллекции SelectedItems. Условие выполняется только в том случае, если свойство Count больше нуля. Всё было бы хорошо, да вот только во внешнем операторе if уже выполняется проверка на то, что Count равен нулю. Экземпляр коллекции SelectedItems не может иметь количество элементов меньше нуля, следовательно, Count принимает значение либо равное нулю, либо больше нуля. Раз мы уже выполнили проверку в первом операторе if на то, что Count равен нулю, и оно оказалось ложным, бессмысленно писать в ветке else ещё одну проверку на то, что Count больше нуля.

Заключительным примером ошибки с номером V3022 будет следующий фрагмент кода:

private void DrawCursorGraphics(Graphics g)
{
  ....
  int cursorOffsetX = 10, cursorOffsetY = 10, itemGap = 10, itemCount = 0;
  Size totalSize = Size.Empty;

  int magnifierPosition = 0;
  Bitmap magnifier = null;

  if (Options.ShowMagnifier)
  {
    if (itemCount > 0) totalSize.Height += itemGap;
    ....
  }
  ....
}

Предупреждение PVS-Studio: V3022 Expression 'itemCount > 0' is always false. RegionCaptureForm.cs 1100.

Анализатор заметил, что условие itemCount > 0 всегда будет ложным, так как чуть выше выполняется объявление и одновременное присваивание переменной itemCount значения, равного нулю. Вплоть до самого условия эта переменная нигде не используется и не изменяется, следовательно, анализатор сделал правильный вывод об условном выражении, значение которого всегда ложно.

Что ж, давайте теперь рассмотрим что-то действительно интересное.

Самый хороший способ понять баг — это визуализировать баг

Как нам кажется, довольно интересная ошибка обнаружилась в этом месте:

public static void Pixelate(Bitmap bmp, int pixelSize)
{
  ....
  float r = 0, g = 0, b = 0, a = 0;
  float weightedCount = 0;

  for (int y2 = y; y2 < yLimit; y2++)
  {
    for (int x2 = x; x2 < xLimit; x2++)
    {
      ColorBgra color = unsafeBitmap.GetPixel(x2, y2);

      float pixelWeight = color.Alpha / 255;

      r += color.Red * pixelWeight;
      g += color.Green * pixelWeight;
      b += color.Blue * pixelWeight;
      a += color.Alpha * pixelWeight;

      weightedCount += pixelWeight;
    }
  }
  ....
  ColorBgra averageColor = new ColorBgra((byte)(b / weightedCount),
    (byte)(g / weightedCount), (byte)(r / weightedCount),
    (byte)(a / pixelCount));
  ....
}

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

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

Данный метод принимает на вход два параметра: объекта типа Bitmap и значение типа int, которое обозначает размер пикселизации. Алгоритм работы достаточно прост:

1) Разбиваем полученный на входе фрагмент изображения на квадраты со стороной, равной размеру пикселизации. К примеру, если у нас размер пикселизации равен 15, то мы получим квадрат, содержащий 15x15=225 пикселей.

2) Далее мы обходим каждый пиксель в этом квадрате и аккумулируем значения полей Red, Green, Blue и Alpha в промежуточные переменные, причём предварительно перемножая значение соответствующего цвета и значение альфа канала на переменную pixelWeight, получаемую путём деления значения Alpha на 255 (переменная Alpha имеет тип byte). Также при обходе пикселей мы суммируем значения, записанные в pixelWeight, в переменную с именем weightedCount.

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

ColorBgra color = unsafeBitmap.GetPixel(x2, y2);

float pixelWeight = color.Alpha / 255;

r += color.Red * pixelWeight;
g += color.Green * pixelWeight;
b += color.Blue * pixelWeight;
a += color.Alpha * pixelWeight;

weightedCount += pixelWeight;

Кстати, обратите внимание на то, что если значение переменной Alpha равно нулю, то и pixelWeight не будет добавлять к переменной weightedCount никакого значения для данного пикселя. Это нам понадобится в дальнейшем.

3) После того, как мы обошли все пиксели в текущем квадрате, мы можем составить общий «усреднённый» цвет для данного квадрата. Код, выполняющий эти действия, выглядит следующим образом:

ColorBgra averageColor = new ColorBgra((byte)(b / weightedCount),
    (byte)(g / weightedCount), (byte)(r / weightedCount),
    (byte)(a / pixelCount));

4) Теперь, когда мы получили итоговый цвет и записали его в переменную averageColor, мы можем опять обойти каждый пиксель квадрата и присвоить ему значение из averageColor.

5) Возвращаемся к пункту 2 до тех пор, пока ещё остались необработанные квадраты.

Ещё раз обратим внимание, что переменная weightedCount не равна количеству всех пикселей в квадрате. К примеру, если в изображении встречается абсолютно прозрачный пиксель (значение ноль по альфа каналу), то переменная pixelWeight будет равна нулю для данного пикселя (0 / 255 = 0), следовательно, этот пиксель не внесёт никакой вклад во формирование значения переменной weightedCount. Это и логично — нет смысла учитывать цвета абсолютно прозрачного пикселя.

Всё кажется вполне разумным — пикселизация должна работать правильно. И она действительно работает правильно. Вот только не для png изображений, которые имеют в своём составе пиксели со значениями в альфа канале меньше 255 и неравными нулю. Обратите внимание на пикселизированную картинку снизу:

Увидели пикселизацию? И мы нет. Хорошо, теперь давайте раскроем эту небольшую интригу и поясним, где именно прячется баг в этом методе. Ошибка закралась в строку вычисления значения переменной pixelWeight:

float pixelWeight = color.Alpha / 255;

Дело в том, что автор кода, объявляя переменную pixelWeight типом float, подразумевал, что при делении поля Alpha на число 255 помимо нуля и единицы должны получаться дробные числа. Здесь и кроется проблема, так как переменная Alpha имеет тип byte, и при делении её на число 255 мы получаем целочисленное значение, и только потом оно будет неявно приведено к типу float, следовательно, происходит потеря дробной части.

Невозможность провести пикселизацию для изображений формата png, которые имеют некоторую степень прозрачности, легко объяснить. Так как значения альфа канала у данных пикселей лежит в диапазоне 0 < Alpha < 255, то, при делении переменной Alpha на число 255, мы всегда будем получать 0. Следовательно, значения переменных pixelWeight, r, g, b, a, weightedCount тоже всегда будут равны нулю. Как итог, наш усреднённый цвет averageColor будет с нулевыми значениями по всем каналам: красный — 0, синий — 0, зелёный — 0, альфа — 0. Закрашивая квадрат в такой цвет, мы не изменяем исходный цвет пикселей, так как averageColor является абсолютно прозрачным. Для того, чтобы исправить эту ошибку, необходимо просто явно привести поле Alpha к типу float. Исправленная строка кода может выглядеть следующим образом:

float pixelWeight = (float)color.Alpha / 255;

И пора привести сообщение, которое выдавал PVS-Studio на ещё некорректный код:

Предупреждение PVS-Studio: V3041 [CWE-682] The 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;. ImageHelpers.cs 1119.

И для сравнения приведём скриншот действительно пикселизированного изображения, полученного на исправленной версии приложения:

Потенциально возможное исключение NullReferenceException

public static bool AddMetadata(Image img, int id, string text)
{
  ....
  pi.Value = bytesText;

  if (pi != null)
  {
    img.SetPropertyItem(pi);
    return true;
  }
  ....
}

Предупреждение PVS-Studio: V3095 [CWE-476] The 'pi' object was used before it was verified against null. Check lines: 801, 803. ImageHelpers.cs 801

Данный фрагмент кода показывает, что его автор ожидал, что переменная pi может иметь значение null, именно поэтому перед тем, как вызывать метод SetPropertyItem, выполняется проверка pi != null. Странно, что перед этой проверкой происходит присваивание свойству pi.Value массива байт, ведь если pi будет равно null, то будет выброшено исключение типа NullReferenceException.

Аналогичная ситуация была замечена и в другом месте:

private static void Task_TaskCompleted(WorkerTask task)
{
  ....
  task.KeepImage = false;

  if (task != null)
  {
    if (task.RequestSettingUpdate)
    {
      Program.MainForm.UpdateCheckStates();
    }
    ....
  }
  ....
}

Предупреждение PVS-Studio: V3095 [CWE-476] The 'task' object was used before it was verified against null. Check lines: 268, 270. TaskManager.cs 268

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

Предупреждение PVS-Studio: V3095 [CWE-476] The 'Config.PhotobucketAccountInfo' object was used before it was verified against null. Check lines: 216, 219. UploadersConfigForm.cs 216

Одно и тоже возвращаемое значение

Подозрительный фрагмент кода был обнаружен в методе EvalWindows класса WindowsList, который при любых обстоятельствах возвращает true:

public class WindowsList
{
  public List<IntPtr> IgnoreWindows { get; set; }
  ....
  public WindowsList()
  {
    IgnoreWindows = new List<IntPtr>();
  }

  public WindowsList(IntPtr ignoreWindow) : this()
  {
    IgnoreWindows.Add(ignoreWindow);
  }
  ....
  private bool EvalWindows(IntPtr hWnd, IntPtr lParam)
  {
    if (IgnoreWindows.Any(window => hWnd == window))
    {
      return true;  // <=
    }

    windows.Add(new WindowInfo(hWnd));

    return true;  // <=
  }
}

Предупреждение PVS-Studio: V3009 It's odd that this method always returns one and the same value of 'true'. WindowsList.cs 82

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

Список IgnoreWindows может заполняться либо при вызове конструктора WindowsList(IntPtr ignoreWindow), либо напрямую через доступ к свойству, так как оно публичное. Так или иначе, если верить Visual Studio, на данный момент в коде этот список никак не заполняется. Это ещё одно странное место этого метода.

Небезопасный вызов обработчиков событий

protected void OnNewsLoaded()
{
  if (NewsLoaded != null)
  {
    NewsLoaded(this, EventArgs.Empty);
  }
}

Предупреждение PVS-Studio: V3083 [CWE-367] Unsafe invocation of event 'NewsLoaded', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. NewsListControl.cs 111

В данном случае у нас может произойти следующая неприятная ситуация: после проверки переменной NewsLoaded на неравенство null метод, выполняющий обработку события, может быть отписан, к примеру, в другом потоке, и, когда мы попадём в тело условного оператора if, переменная NewsLoaded уже будет равна null. Попытка вызвать подписчиков у события NewsLoaded, которое имеет значение null, приведёт к возникновению исключения NullReferenceException. Гораздо безопаснее воспользоваться null-условным оператором и переписать приведённый выше код следующим образом:

protected void OnNewsLoaded()
{
  NewsLoaded?.Invoke(this, EventArgs.Empty);
}

Анализатор указал на ещё 68 аналогичных мест. Описывать здесь их не будем — паттерн вызова события в них подобный.

Возвращаем null из ToString

Не так давно из одной интересной статьи моего коллеги я узнал, что Microsoft не рекомендует возвращать null из переопределяемого метода ToString. PVS-Studio хорошо осведомлён об этом:

public override string ToString()
{
  lock (loggerLock)
  {
    if (sbMessages != null && sbMessages.Length > 0)
    {
      return sbMessages.ToString();
    }

    return null;
   }
 }

Предупреждение PVS-Studio: V3108 It is not recommended to return 'null' from 'ToSting()' method. Logger.cs 167

Зачем присваивать, если не используешь?

public SeafileCheckAccInfoResponse GetAccountInfo()
{
  string url = URLHelpers.FixPrefix(APIURL);
  url = URLHelpers.CombineURL(APIURL, "account/info/?format=json");
....
}

Предупреждение PVS-Studio: V3008 The 'url' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 197, 196. Seafile.cs 197

Как видно из примера, при объявлении переменной url ей присваивается некоторое значение, возвращаемое из метода FixPrefix. В последующей строке мы «перетираем» полученное значение, даже нигде его не использовав. Получаем что-то похожее на «мёртвый код» — работу выполняет, на итоговый результат никак не влияет. Данная ошибка, скорее всего, является результатом копипаста, так как подобные фрагменты кода встречаются ещё в 9 методах.
Для примера, приведём два метода с аналогичной первой строкой:

public bool CheckAuthToken()
{
  string url = URLHelpers.FixPrefix(APIURL);
  url = URLHelpers.CombineURL(APIURL, "auth/ping/?format=json");
  ....
}
....
public bool CheckAPIURL()
{
  string url = URLHelpers.FixPrefix(APIURL);
  url = URLHelpers.CombineURL(APIURL, "ping/?format=json");
  ....
}

Итого

Как мы видим, сложность настройки автоматической проверки анализатором не зависит от выбранной CI-системы – буквально за 15 минут и несколько кликов мышкой мы настроили проверку кода нашего проекта статическим анализатором.

В заключение предлагаем и вам загрузить и попробовать анализатор на своих проектах.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Oleg Andreev, Ilya Gainulin. PVS-Studio in the Clouds: Azure DevOps.