ScreenToGif — полезное приложение, позволяющее сделать из записи экрана или веб-камеры gif-файл, который можно удобно отредактировать во встроенном редакторе. В этой статье мы рассмотрим интересные проблемные места в исходном коде проекта и расскажем, как одна маленькая ошибка может нарушить работу всей программы.

Введение

Для ознакомления с работой приложения я установил его, и оно оказалось весьма удобным. У него интуитивно понятный интерфейс и приличный функционал для создания gif-файлов.

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

Проверяемый код ScreenToGif соответствует версии 2.41.1. Анализатор PVS-Studio нашёл немало проблемных мест в коде, из которых мы отобрали наиболее интересные и разнообразные, чтобы рассказать вам о них. Особенно выделю одну потенциальную уязвимость, которую удалось воспроизвести в самом приложении. Приступим :)

Issue 1: ReDoS-уязвимость

Начнём с самой интересной и интригующей части статьи — обнаруженной уязвимости.

public static string ReplaceRegexInName(string name)
{
  const string dateTimeFileNameRegEx = @"[?]([ymdhsfzgkt]+[-_ ]*)+[?]";  // <=

  if (!Regex.IsMatch(name, dateTimeFileNameRegEx, RegexOptions.IgnoreCase))
    return name;

  var match = Regex.Match(name, dateTimeFileNameRegEx, RegexOptions.IgnoreCase);
  var date = DateTime.Now.ToString(Regex.Replace(match.Value, "[?]", ""));

  return name.Replace(match.ToString(), date);
}

Сообщение анализатора PVS-Studio:

V5626. Possible ReDoS vulnerability. Potentially tainted data from the 'name' variable is processed by regular expression that contains unsafe pattern: '(...sfzgkt]+...)+'

В этом коде есть ReDoS-уязвимость, которая способна замедлить или вовсе остановить работу приложения. Звучит страшно? Возможно. Проблема заключается в использовании уязвимого регулярного выражения, в которое можно передать произвольную строку. Если входная строка будет составлена определённым образом, то приложение зависнет.

В коде присутствует уязвимое регулярное выражение "[?]([ymdhsfzgkt]+[-_ ]*)+[?]". Упростим его для наглядности — "[?]([ymdhsfzgkt]+)+[?]". Опасным здесь является паттерн "(x+)+". Он позволяет сильно замедлить работу регулярного выражения при передаче определённой строки. Эта уязвимость актуальна только для алгоритмов на основе недетерминированного конечного автомата.

Без особого труда удалось воспроизвести эту уязвимость в самом приложении:

Как видно на скриншоте, если ввести в название выходного файла определённую строку, приложение зависнет.

В нашем случае использовалась строка "?ymdhsfzgktymdhsfzgktymdhsfzgktymdhsfzgktymdhsfzgkt"

Конечно, вряд ли кто-то станет делать подобное название, поэтому такая уязвимость тут безобидна. Но если допустить подобное в коде, связанном с сервером, то можно замедлить его работу или даже остановить.

Более подробно об этой уязвимости можно прочитать в статье "Что такое катастрофический возврат и ReDoS-уязвимости".

PVS-Studio является SAST-решением, при помощи статического анализа производится поиск различных потенциальных уязвимостей. Это позволяет улучшать безопасность тестируемых приложений. Анализатор может находить множество дефектов безопасности, среди которых: SQL и LDAP инъекции, XSS, XXE и другие.

Issue 2: потеря кнопки

public object ConvertBack(object value, Type targetType,
                          object parameter, CultureInfo culture)
{
  if (value is not int index)
    return DependencyProperty.UnsetValue;

  switch (index)
  {
    case 0:
      return Key.F1;
    case 1:
      return Key.F2;
    case 2:
      return Key.F3;
    case 13:
      return Key.F4;
    case 4:
      return Key.F5;
    case 5:
      return Key.F6;
    case 6:
      return Key.F7;
    case 7:
      return Key.F8;
    case 8:
      return Key.F9;
    case 9:
      return Key.F10;
    case 10:
      return Key.F11;
    case 11:
      return Key.F11;
  }

  return Key.F1;
}

Сообщение анализатора:

V3139. Two or more case-branches perform the same actions.

Представленный фрагмент является классическим примером эффекта последней строки — это когда человек, применяющий copy-paste, исправляет все строки, кроме последней. Скорее всего, в case 11 должен возвращаться Key.F12, а не Key.F11. Вероятно, разработчик использовал copy-paste для написания данных case и ошибся в последнем значении.

Использование copy-paste нередко приводит к опечаткам, которые потом не так просто заметить при code review. По статистике на последний скопированный блок кода приходится в четыре раза больше ошибок, поэтому рекомендуем проверять внимательнее хотя бы его :)

Issue 3: неправильные цвета

Рассмотрим ошибку, затрагивающую преобразование цветовой модели из RGB в HSV.

private void UpdateMarkerPosition(Color theColor)
{
  ....
  var hsv = ColorExtensions.ConvertRgbToHsv( theColor.R, 
                                             theColor.G, 
                                             theColor.B);
  ....
}

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

V3066. Possible incorrect order of arguments passed to 'ConvertRgbToHsv' method: 'theColor.G' and 'theColor.B'.

Анализатор предполагает, что компоненты цвета theColor.G и theColor.B стоят не на своих местах. Посмотрев на код, разработчик не увидит ничего подозрительного — привычный всем порядок RGB, на что также намекает название метода.

Рассмотрим сам метод ConvertRgbToHsv:

public static HsvColor ConvertRgbToHsv( int r, 
                                        int b, 
                                        int g)
{
  ....
}

Теперь в глаза бросается, что параметры метода стоят в странном порядке "rbg" вместо привычного "rgb". Судя по всему, при написании метода разработчики ошиблись порядком компонентов цвета, а при использовании ConvertRgbToHsv не обратили внимание на расположение параметров.

Issue 4: одинаковые подвыражения

public static readonly DependencyProperty IsReversedProperty =
DependencyProperty
   .Register("IsReversed",
             typeof(bool),
             typeof(DynamicGrid),
             new FrameworkPropertyMetadata(false,
                        FrameworkPropertyMetadataOptions.AffectsMeasure 
                      | FrameworkPropertyMetadataOptions.AffectsMeasure));

Сообщение анализатора:

V3001. There are identical sub-expressions 'FrameworkPropertyMetadataOptions.AffectsMeasure' to the left and to the right of the '|' operator.

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

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

Далее показаны некоторые флаги, которые в теории могли быть использованы:

[Flags]
public enum FrameworkPropertyMetadataOptions
{ 
  ....
  None = 0,
  AffectsMeasure = 1,
  AffectsArrange = 2,
  AffectsParentMeasure = 4,
  AffectsParentArrange = 8,
  AffectsRender = 16,
  ....
}

Issue 5: leftDpiX или rightDpiX, вот в чём вопрос

private void Window_Loaded(object sender, RoutedEventArgs e)
{
  ....
  CanvasSizeTextBlock.Text = $"{right.PixelWidth} × " +
                             $"{right.PixelHeight} • " +
                             $"{Math.Round(left.DpiX, 0)} " +        // <=
                             $"{LocalizationHelper.Get("S.Resize.Dpi")}";

  LeftImageSizeTextBlock.Text = $"{left.PixelWidth} × " +
                                $"{left.PixelHeight} • " +
                                $"{Math.Round(left.DpiX, 0)} " +
                                $"{LocalizationHelper.Get("S.Resize.Dpi")}";    
  ....
}

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

V3127. Two similar code fragments were found. Perhaps, this is a typo and 'right' variable should be used instead of 'left'.

Анализатор предполагает, что в указанной строке вместо left.DpiX должно быть right.DpiX. Сложно сразу сказать, является ли это ошибкой.

Посмотрим на другое присваивание свойству CanvasSizeTextBlock.Text в проекте.

CanvasSizeTextBlock.Text = 
  $"{RightCanvas.ActualWidth * _rightScaleDiff * _rightScale} × " +
  $"{RightCanvas.ActualHeight * _rightScaleDiff * _rightScale} • " +
  $"{Math.Round(_rightDpi, 0)} " +            // <=
  $"{LocalizationHelper.Get("S.Resize.Dpi")}";

Во втором фрагменте видно, что на месте, где предполагается ошибка в первом фрагменте, используется переменная с "right" в названии, что подтверждает возможность опечатки.

Issue 6: NullRef properties

var properties = doc.Root?.Elements().Select(GetProperty).ToList();
....
var version = properties?.FirstOrDefault(f => f.Key == "Version")    // <=
                                         ?.Value ?? "0.0";

Migration.Migrate(properties, version);

var resource = new ResourceDictionary {{"Version", All.VersionText}};

foreach (var property in properties)                                 // <=
{
  var value = ParseProperty(property);

  if (value != null && !resource.Contains(property.Key))
    resource.Add(property.Key, value);
}

Сообщение анализатора:

V3105. The 'properties' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible.

В этом фрагменте видно, что переменной properties присваивается значение, полученное с помощью оператора '?.', что намекает на возможное значение null для данной переменной.

Далее properties используется с проверкой на null: var version = properties?.FirstOrDefault. Однако буквально через несколько строк без какой-либо проверки используют properties в качестве перечислимого выражения в foreach. Подобное использование может привести к выбрасыванию исключения типа NullReferenceException. Иногда это исключение в foreach может стать неожиданным для разработчика. Например, если использует оператор '?.' для попытки избежать NRE, но это приводит только к иллюзии безопасности. Подробнее об этом можете прочитать в статье "Использование оператора ?. в foreach: защита от NullReferenceException, которая не работает".

Issue 7: NullRef list

if (File.Exists(Path.Combine(pathTemp, "Project.json")))
{
  ....
  using (var ms = new MemoryStream(Encoding.UTF8.GetBytes(json)))
  {
    var ser = new DataContractJsonSerializer(typeof(ProjectInfo));
    var project = ser.ReadObject(ms) as ProjectInfo;

    list = project?.Frames;                                      // <=
  }
}
....
//Shows the ProgressBar
ShowProgress(LocalizationHelper.Get("S.Editor.ImportingFrames"),
                                     list.Count);                // <=

Сообщение анализатора PVS-Studio:

V3105. The 'list' variable was used after it was assigned through null-conditional operator.

После возможного присваивания null в переменную list дальше она используется без проверки. Одно из возможных возвращаемых значений метода ReadObject — null, из-за чего project также может иметь значение null. Далее переменной list присваивается project?.Frames, что при следующем её использовании может вызвать исключение.

Issue 8: неучтённое условие

protected override void OnPreviewKeyDown(KeyEventArgs e)
{
  ....
  if (   (e.Key == Key.OemQuestion || e.Key == Key.OemPeriod) 
      && (Keyboard.Modifiers & ModifierKeys.Control) == 0)
  {
    if (Text.Length > 8)              // <=
    {
      e.Handled = true;
      return;
    }
    if (Text.Length == 1){....}
    else if (Text.Length == 4) {....}
    else if (Text.Length == 7) {....}
    else if (Text.Length == 10)       // <=
       Text = Text.Substring(0, 9) + Text.Substring(6, 1).PadLeft(3, '0');

    SelectionStart = Text.Length;
    e.Handled = true;
    return;
  }
  ....
}

Предупреждение анализатора PVS-Studio:

V3022. Expression 'Text.Length == 10' is always false.

Анализатор предупреждает, что Text.Length == 10 всегда возвращает false, и это действительно так. Немного выше идёт проверка if(Text.Length > 8) c return, из-за чего в следующие условия не смогут пройти значения Text.Lenght больше 8. Поэтому проверка if(Text.Length == 10) является бессмысленной, либо в условии Text.Length > 8 допущена ошибка.

Поиск подобных ошибок производится за счёт DataFlow анализа. Статический анализатор не может знать, какое конкретное значение принимает переменная, но может предположить диапазон возможных значений. Это позволяет находить участки кода, использующие потенциально некорректные значения переменных или констант. В этих местах могут возникнуть следующие проблемы: возможное деление на ноль, выражение всегда false или true, выход за границы массива и многое другое.

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

Issue 9: неправильные границы

if (monitor.NativeBounds.Top > top)
  top = monitor.NativeBounds.Top;

if (monitor.Bounds.Left > left)                 // <=
  left = monitor.NativeBounds.Left;

if (monitor.NativeBounds.Bottom < top + height)
  top = monitor.NativeBounds.Bottom - height;

if (monitor.NativeBounds.Right < left + width)
  left = monitor.NativeBounds.Right - width;

Сообщение PVS-Studio:

V3127. Two similar code fragments were found. Perhaps, this is a typo and 'Bounds' variable should be used instead of 'NativeBounds'

В рассматриваемом фрагменте анализатор сообщает о несоответствии Bounds в условии if с NativeBounds в теле then. Рядом есть ещё три проверки на top, bottom и right, что говорит о схожести этих проверок по смыслу, но только у подозрительной проверки left присутствует Bounds вместо NativeBounds. Из-за схожести названий разработчик мог опечататься и не заметить, что использовал не то свойство.

Issue 10: дублированное сообщение

....
using (var process = new Process())
{
  process.StartInfo = procStartInfo;
  process.Start();

  var message = await process.StandardOutput.ReadToEndAsync();
  var error = await process.StandardError.ReadToEndAsync();

  if (!string.IsNullOrWhiteSpace(message))
      output += message + Environment.NewLine;

  if (!string.IsNullOrWhiteSpace(message))
      output += message + Environment.NewLine;

  process.WaitForExit(1000);
}
....
SetCommand(id, true, command, output);

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

V3029. The conditional expressions of the 'if' statements situated alongside each other are identical.

Анализатор заметил два одинаковых if с одинаковыми телами. В них используется оператор '+=' для записи одного и того же сообщения в переменную output. Далее эта переменная передаётся в метод SetCommand. Если приглядеться, можно заметить рядом с объявлением переменной message объявление похожей переменной error, которая могла бы вписаться в один из if. Тогда в output записали и message, и error. Вероятно, разработчик скопировал блок if и забыл поменять переменную.

Заключение

В этой статье мы рассмотрели различные проблемные участки кода проекта ScreenToGif. Само приложение имеет широкий функционал и удобный интерфейс. Разработчики хорошо постарались, в их коде не нашлось серьёзных ошибок :)

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

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

Надеюсь, материал был для вас интересным, спасибо за прочтение :)

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Nikita Priadko. How can one code line crash application? Looking for issues and vulnerabilities in ScreenToGif.