# How can one code line crash application? Looking for issues and vulnerabilities in ScreenToGif

ScreenToGif is a useful application that enables you to create a GIF file from a screen or webcam recording, which you can easily edit in the built\-in editor\. In this article, we'll delve into some fascinating buggy snippets in the project source code, and discuss how one small error can break the entire program\.

![1194_ScreenToGif/image1.png](https://import.viva64.com/docx/blog/1194_ScreenToGif/image1.png)

## Introduction

I installed the app to see how it works\. It turned out to be quite handy\. It has a user\-friendly interface and a decent set of features for creating GIF files\. 

For example, you can do real\-time changes to the screen\-capture area\. When the reading is complete, the built\-in file editor opens, enabling a user to delete and edit the captured footage\. 

We've checked the [2\.41\.1](https://github.com/NickeManarin/ScreenToGif/tree/2.41.1) version of the ScreenToGif code\. The PVS\-Studio analyzer has found quite a number of erroneous code fragments, and we've picked out the most interesting and diverse ones to tell you about\. I'd like to highlight one potential vulnerability that we've managed to reproduce directly in the application\. Let's get it started :\)

## Issue 1: ReDoS vulnerability

We'll begin with the most interesting and intriguing part of the article, the following vulnerability:

```cpp
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);
}
```

The PVS\-Studio message:

[V5626](https://pvs-studio.com/en/docs/warnings/v5626/)\. Possible ReDoS vulnerability\. Potentially tainted data from the 'name' variable is processed by regular expression that contains unsafe pattern: '\(\.\.\.sfzgkt\]\+\.\.\.\)\+'

The code contains the ReDoS vulnerability that can slow down or completely shut down an application\. Sounds scary? Maybe\. The issue lies in using a vulnerable regular expression to which you can pass an arbitrary line\. If the input line is composed in a certain way, the application hangs\. 

The code contains the "\[?\]\(\[ymdhsfzgkt\]\+\[\-\_ \]\*\)\+\[?\]" vulnerable regular expression\. Let's simplify it for clarity: "\[?\]\(\[ymdhsfzgkt\]\+\)\+\[?\]"\. The "\(x\+\)\+" pattern is the dangerous one here\. It significantly slows down the regular expression when a certain string is passed\. This vulnerability is relevant only for algorithms based on a nondeterministic finite automaton\.

We've managed to reproduce it in the application without much difficulty:

![1194_ScreenToGif/image2.png](https://import.viva64.com/docx/blog/1194_ScreenToGif/image2.png)

As shown in the screenshot, if we enter a certain line in the output file name, the application hangs\.

In our case, we used the "?ymdhsfzgktymdhsfzgktymdhsfzgktymdhsfzgktymdhsfzgkt" line\.

Of course, it's unlikely that anyone would use a name like that, so the vulnerability is harmless here\. However, if one lets this happen in the server\-related code, it can slow down its work or even stop it\.

You can read more about this vulnerability in the "[Catastrophic backtracking: how can a regular expression cause a ReDoS vulnerability?](https://pvs-studio.com/en/blog/posts/csharp/1007/)" article\.

PVS\-Studio is a [SAST solution](https://pvs-studio.com/en/blog/terms/6631/) that uses static analysis to detect various potential vulnerabilities\. This helps improve the security of tested applications\. The analyzer can find many security flaws, including SQL and LDAP injections, XSS, XXE, and others\. 

## Issue 2: button loss

```cpp
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;
}
```

The analyzer message:

[V3139](https://pvs-studio.com/en/docs/warnings/v3139/)\. Two or more case\-branches perform the same actions\. 

The above snippet is a classic example of [the last line effect](https://pvs-studio.com/en/blog/posts/cpp/0260/), where a person using copy\-paste fixes everything except the last line\. Most likely, _case 11_ likely should return _Key\.F12_ instead of _Key\.F11_\. The developer probably copied and pasted the existing _cases_ but missed fixing the last value\.

Copying and pasting often leads to typos that aren't so easy to spot during code review\. Statistics show that the last copied code fragment contains four times more errors than the others, so we recommend you to check at least this fragment more carefully :\)

![1194_ScreenToGif/image3.png](https://import.viva64.com/docx/blog/1194_ScreenToGif/image3.png)

## Issue 3: incorrect colors

Let's look at an error that affects color model conversion from RGB to HSV\.

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

The PVS\-Studio warning:

[V3066](https://pvs-studio.com/en/docs/warnings/v3066/)\. Possible incorrect order of arguments passed to 'ConvertRgbToHsv' method: 'theColor\.G' and 'theColor\.B'\.

The analyzer assumes that the _theColor\.G_ and _theColor\.B_ color components are misplaced\. A developer looking at the code won't see anything suspicious, just the usual RGB order, which is also implied by the method name\. 

Let's take a look at the _ConvertRgbToHsv_ method:

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

Now it's clear that the method parameters are in the strange order of "rbg" instead of the usual "rgb"\. Apparently, developers made a mistake in the color component order when writing the method, and when using _ConvertRgbToHsv_, they overlooked the parameter location\.

## Issue 4: identical subexpressions

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

The analyzer warning: 

[V3001](https://pvs-studio.com/en/docs/warnings/v3001/)\. There are identical sub\-expressions 'FrameworkPropertyMetadataOptions\.AffectsMeasure' to the left and to the right of the '\|' operator\.

This time, the analyzer detected a strange code snippet where the '\|' operator has identical left and right operands\. Let's figure out why this happened\. 

The _FrameworkPropertyMetadataOptions_ enumeration contains flags with similar names, which can cause confusion when substituting them\. The developer might have wanted to use a different option in one of the subexpressions but mistakenly chose the same one\.

The following fragment shows some flags that could theoretically be used:

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

## Issue 5: leftDpiX or rightDpiX, that is the question

```cpp
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")}";    
  ....
}
```

The PVS\-Studio warning:

[V3127](https://pvs-studio.com/en/docs/warnings/v3127/)\. Two similar code fragments were found\. Perhaps, this is a typo and 'right' variable should be used instead of 'left'\.

The analyzer assumes that the specified string should contain _right\.DpiX_ instead of _left\.DpiX_\. It's hard to tell right away whether this is an error\.

Let's look at another assignment to the _CanvasSizeTextBlock\.Text_ property in the project\.

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

In the second fragment, we can see that a variable with "right" in its name is used in the first fragment, where we expect the error to occur, which confirms that a typo is possible\.

## Issue 6: NullRef properties

```cpp
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);
}
```

The analyzer warning:

[V3105](https://pvs-studio.com/en/docs/warnings/v3105/)\. The 'properties' variable was used after it was assigned through null\-conditional operator\. NullReferenceException is possible\.

In this fragment, you can see that the _properties_ variable is assigned a value obtained via the '?\.\.' operator, which indicates that this value can be _null_\. 

Next, _properties_ is used with a check for _null_: _var version \= properties?\.FirstOrDefault_\. However, just a few lines down, the devs use _properties_ as an enumerated expression in _foreach_ without any check\. Such use may result in the _NullReferenceException_ exception type being thrown\. Sometimes this exception in _foreach_ can come as a surprise to a developer\. Especially if they used the '?\.\.' operator to avoid the NRE, only to find out that it offers just the illusion of security\. You can read more on this topic in the "[The ?\. operator in foreach will not protect from NullReferenceException](https://pvs-studio.com/en/blog/posts/csharp/0832/)" article\.

## Issue 7: NullRef list

```cpp
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);                // <=
```

The PVS\-Studio message:

[V3105](https://pvs-studio.com/en/docs/warnings/v3105/)\. The 'list' variable was used after it was assigned through null\-conditional operator\. 

After a possible assignment of _null_ to the _list_ variable, it's further used without any check\. A possible return value of the _ReadObject_ method is _null_, so _project_ can also have a _null_ value\. Then _project?\.Frames_ is assigned to the _list_ variable, which may cause an exception the next time it's used\.

## Issue 8: unaccounted\-for condition

```cpp

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;
  }
  ....
}
```

The PVS\-Studio warning:

[V3022](https://pvs-studio.com/en/docs/warnings/v3022/)\. Expression 'Text\.Length \=\= 10' is always false\. 

The analyzer warns that _Text\.Length \=\= 10_ always returns _false_, and this is indeed true\. There's the _if\(Text\.Length \> 8\)_ check with _return_ a bit higher up the code, which makes it impossible to pass the _Text\.Length_ values greater than 8 into the subsequent conditions\. So, the _if\(Text\.Length \=\= 10\)_ check is pointless, or there's an error in the _Text\.Length \> 8_ condition\.

Data\-flow analysis helps detect such errors\. A static analyzer can't pinpoint the exact value of a variable, but it can assume a range of possible values\. This helps identify code fragments that use potentially incorrect values of variables or constants\. It can lead to the following issues: a possible division by zero, an expression that is always _false_ or _true_, array overruns, and more\.

In this example, the condition always returns _false_, making the _if_ body unreachable\. If this code was intended to carry out crucial operations, their loss could result in a bug or vulnerability\.

## Issue 9: incorrect boundaries

```cpp
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;
```

The PVS\-Studio message:

[V3127](https://pvs-studio.com/en/docs/warnings/v3127/)\. Two similar code fragments were found\. Perhaps, this is a typo and 'Bounds' variable should be used instead of 'NativeBounds'

In this fragment, the analyzer reports a mismatch between _Bounds_ in the _if_ condition and _NativeBounds_ in the _then_ body\. There are three more checks for _top_, _bottom_, and _right_ nearby, which shows the similarity of these checks\. However, only the _left_ check in question has _Bounds_ instead of _NativeBounds_\. The similar names may have caused the developer to make a typo and not realize that they used the wrong property\.

## Issue 10: duplicate message

```cpp
....
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);
```

The PVS\-Studio warning:

[V3029](https://pvs-studio.com/en/docs/warnings/v3029/)\. The conditional expressions of the 'if' statements situated alongside each other are identical\. 

The analyzer detected two identical _if_'s with the same bodies\. They use the '\+\=' operator to write the same message to the _output_ variable\. This variable is then passed to the _SetCommand_ method\. If we look closely, we can see the _message_ variable declaration next to the declaration of the similar _error_ variable which might have been intended for use in one of the _if_ statements\. Then, both _message_ and _error_ are written to the _output_\. The developer probably copied the _if_ block and forgot to change the variable\.

## Conclusion

In this article, we've looked at various buggy code fragments of the ScreenToGif project\. The application is feature\-rich and has a user\-friendly interface\. The developers have done a great job—we didn't find any major bugs :\)

In software development, many errors are discovered at a later stage, when the code has already been written\. Finding them takes a lot of time and effort, which can be reduced by using static analysis in the development process\. You can leverage the static analyzer at the code\-writing stage, preventing many errors \(including serious ones\) slip through to later stages\. While a one\-time project check, such as the one in the article, is good only for demonstrating some of the analyzer capabilities\.

If after reading this article you'd like to try out the static analysis capabilities for checking projects, you can use [the free version of PVS\-Studio](https://pvs-studio.com/en/pvs-studio/try-free/)\.

I hope you enjoyed the article and thank you for reading :\)