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.
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 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:
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. 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:
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?" article.
PVS-Studio is a SAST solution 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
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. Two or more case-branches perform the same actions.
The above snippet is a classic example of the last line effect, 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 :)
Issue 3: incorrect colors
Let's look at an error that affects color model conversion from RGB to HSV.
private void UpdateMarkerPosition(Color theColor)
{
....
var hsv = ColorExtensions.ConvertRgbToHsv( theColor.R,
theColor.G,
theColor.B);
....
}
The PVS-Studio warning:
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:
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
public static readonly DependencyProperty IsReversedProperty =
DependencyProperty
.Register("IsReversed",
typeof(bool),
typeof(DynamicGrid),
new FrameworkPropertyMetadata(false,
FrameworkPropertyMetadataOptions.AffectsMeasure
| FrameworkPropertyMetadataOptions.AffectsMeasure));
The analyzer warning:
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:
[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
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. 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.
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
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. 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" article.
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); // <=
The PVS-Studio message:
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
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. 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
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. 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
....
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. 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.
I hope you enjoyed the article and thank you for reading :)
Top comments (0)