Copy-paste on big screen. Analyzing errors and oddities in Radarr code

• Introduction
• The project check
  • A boxing error
  • Treacherous !=
  • Errors and suspicious fragments caused by copy-paste
  • Challenge your focus
• Conclusion

Writing the same thing over and over again is an incredibly tedious endeavor! That's why programmers often resort to copy-pasting. While it does save time, it also increases the risk of errors. We'll break down bugs and weird parts of the Radarr project to see for ourselves.

Introduction

Radarr is an open-source movie manager for Usenet and BitTorrent users. The tool can monitor multiple RSS feeds to download and update movies as they become available in the desired formats and higher resolutions.

The project code is quite well-written and has few errors. However, it still has a significant number of weaknesses due to slight carelessness and copy-paste.

To perform the analysis, we used the source code available on GitHub via this link.

Let's move on to the errors our analyzer detected!

The project check

A boxing error

Code fragment 1

public bool Equals(OsPath other, bool ignoreTrailingSlash)
{
  if (ReferenceEquals(other, null))
  {
    return false;
  }
  ....
}

The PVS-Studio warning: V3161 Comparing value type variables with 'ReferenceEquals' is incorrect because 'other' will be boxed. OsPath.cs 379

In this case, a value type object is compared by reference. The ReferenceEquals method takes parameters of the Object type. When a value type is passed, it gets boxed. The reference created on the heap won't match any other reference, meaning the method will always return false.

Here's an example showing why we should keep in mind nullable value types:

 static void NullableTest()
{
  int? a = null;
  object aObj = a;

  int? b = new int?();
  object bObj = b;

  Console.WriteLine(Object.ReferenceEquals(aObj, bObj)); // True or False?
}

I invite you to take a closer look at the code snippet above and guess what it outputs to the console. If you'd like to challenge yourself or explore this issue further, here's a link to an article on the topic.

Code fragment 2

switch (format.ToUpperInvariant())
{
  case "L": // Long format
    var stringBuilder = new StringBuilder();
    stringBuilder.AppendLine("Guid: " + Guid ?? "Empty");
    ....
  default:
    return ToString();
}

The PVS-Studio warning: V3123 Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. ReleaseInfo.cs 87

At first glance, this code fragment isn't suspicious, but its author forgot that the ?? and ?: operators have a lower precedence than the + operator. So, the "Guid: " + Guid expression is executed first, and even if the Guid variable is null, ?? "Empty" is still not executed.

The situation when a programmer makes a mistake once and then multiplies it through copy-pasting is quite common even in large projects. For example, in the Azure SDK for .NET project, you could encounter the following code fragments:

public struct BlobSasBuilder : IEquatable<BlobSasBuilder>
{
  ....
  public bool Equals(BlobSasBuilder other) =>
    BlobName == other.BlobName &&
    CacheControl == other.CacheControl &&
    BlobContainerName == other.BlobContainerName &&
    ContentDisposition == other.ContentDisposition &&
    ContentEncoding == other.ContentEncoding &&         // <=
    ContentLanguage == other.ContentEncoding &&         // <=
    ....
}

The PVS-Studio warning: V3112 An abnormality within similar comparisons. It is possible that a typo is present inside the expression 'ContentLanguage == other.ContentEncoding'. BlobSasBuilder.cs 410

Another code fragment has exactly the same error:

public struct FileSasBuilder : IEquatable<FileSasBuilder>
{
  ....
  public bool Equals(FileSasBuilder other)
    => CacheControl == other.CacheControl
    && ContentDisposition == other.ContentDisposition
    && ContentEncoding == other.ContentEncoding         // <=
    && ContentLanguage == other.ContentEncoding         // <=
    ....
}

V3112 An abnormality within similar comparisons. It is possible that a typo is present inside the expression 'ContentLanguage == other.ContentEncoding'. FileSasBuilder.cs 265

You can read more about the check of Azure SDK for .NET in this article.

One can easily overlook such a basic thing and end up with erroneous code like the one above. The analyzers built into IDEs can't detect such patterns, and the issue remains undetected. However, static analysis tools can help in this case.

Code fragment 3

var qualityComparer = new QualityModelComparer(qualityProfile);
var qualityCompare = qualityComparer.Compare(newQuality?.Quality, // <=
                                          currentQuality.Quality);
var downloadPropersAndRepacks = _configService.DownloadPropersAndRepacks;

if (qualityCompare > 0 && 
    QualityCutoffNotMet(qualityProfile, 
                        currentQuality, 
                        newQuality))
{
  _logger.Debug("New item has a better quality. 
                 Existing: {0}. New: {1}", 
                 currentQuality, 
                 newQuality);
  return UpgradeableRejectReason.None;
}

The PVS-Studio warning: V3105 The result of null-conditional operator is dereferenced inside the 'Compare' method. NullReferenceException is possible. Inspect the first argument 'newQuality?.Quality'. UpgradableSpecification.cs 34

The analyzer warns that a NullReferenceException may occur during program execution. In the code, the devs passed a field to a variable without checking it for null. The key thing is that the variable value is evaluated using an expression that uses the null-conditional operator. Also, in the Compare method, the argument isn't checked for null, which means a NullReferenceException may be thrown when we try to access the Quality field.

Look at the following example:

void UsersProcessing(Users users)
{
  ....
  LogUserNames(users?.GetUsersCollection());
}

void LogUserNames(IEnumerable<User> usersList)
{
  foreach (var user in usersList)
  {
    ....
  }
}

The usersList variable is passed as an argument to the LogUserNames method. It can have the null value since it's obtained using the null-conditional operator. Within 'LogUserNames', the passed collection is traversed. To do this, foreach is used, so the GetEnumerator method is called on the collection. If userList is null, the NullReferenceException is thrown.

The fixed version can look like this:

void UsersProcessing(Users users)
{
  ....
  LogUserNames(   users?.GetUsersCollection() 
               ?? Enumerable.Empty<User>());
}

void LogUserNames(IEnumerable<User> usersList)
{
  foreach (var user in usersList)
  {
    ....
  }
}

The result of executing users?.GetUsersCollection() is assigned to the usersList variable. If this variable value is null, an empty collection is passed to the 'LogUserNames' method. This helps avoid the NullReferenceException when traversing usersList in foreach.

Code fragment 4

if (resource.Languages != null)
{
  // Don't allow user to set movieFile with 'Any' or 'Original' language
  movieFile.Languages = resource.Languages
                                .Where(l =>   l != Language.Any 
                                           || l != Language.Original 
                                           || l != null)
                                .ToList();
}

The PVS-Studio warning: V3063 A part of conditional expression is always true if it is evaluated: l != null. MovieFileController.cs 122

The condition here is a bit of a mess. The comment shows that a user shouldn't have an option to set the language to Any or Original, but the logic allows all elements. Let's say an element equal to Language.Any is being checked. It fails the first check but passes the second. The same is true for elements equal to Language.Original. An element unequal to Language.Any and Language.Original satisfies any of the conditions.

So, we can say that the developers made a mistake and should have written && instead of ||.

Code fragment 5

var rootFolder = Path.GetDirectoryName(path);
var movie = _movieService.GetMovie(movieId);

var languageParse = LanguageParser.ParseLanguages(path);

if (   languageParse.Count <= 1 
    && languageParse.First() == Language.Unknown              // <=
    && movie != null)
{
}

The PVS-Studio warning: V3179 Calling the 'First' method for potentially empty collection 'languageParse' may result in exception. ManualImportService.cs 107

The analyzer has issued a low-level warning for this code snippet, but it still piques our attention. In the if condition, we check whether the languageParse collection has one element or less. In case languageParse has no elements at all, the program gets an exception when trying to get the first element using the First function.

The suspicious code fragment is insignificant in this situation, since it's very unlikely that languageParse is empty due to the previous code.

Treacherous !=

Code fragment 6

public virtual bool IsServiceRunning(string name)
{
  _logger.Debug("Checking if '{0}' service is running", name);

  ....

  return service != null && (
       service.Status != ServiceControllerStatus.Stopped 
    || service.Status == ServiceControllerStatus.StopPending 
    || service.Status == ServiceControllerStatus.Paused 
    || service.Status == ServiceControllerStatus.PausePending);
}

The PVS-Studio warning: V3063 A part of conditional expression is always false if it is evaluated: service.Status == ServiceControllerStatus.StopPending. ServiceProvider.cs 55

The analyzer warns that the second condition is always false if it's reached. That's right: if the first sevice.Status check is false, which can happen only if service.Status == ServiceControllerStatus.Stopped, then all other checks are also false. The programmer may have made a typo and wanted to use == instead of != in the service.Status != ServiceControllerStatus.Stopped condition.

Errors and suspicious fragments caused by copy-paste

Code fragment 7

switch (torrent.Status)
{
  ....
  case "error":
    status = DownloadItemStatus.Failed;
    break;
  case "complete":
    status = DownloadItemStatus.Completed;
    break;
  case "removed":
    status = DownloadItemStatus.Failed;
    break;
}

The PVS-Studio warning: V3139 Two or more case-branches perform the same actions. Aria2.cs 118

The analyzer warns us that two or more case branches perform the same assignment. Without knowing how the code is supposed to work, it's difficult to determine whether there's an error.

To prove that we might have an error here, I'd like to note that the DownloadItemStatus enumeration has an unused Warning element, which probably aligns with the intended logic.

Challenge your focus

Code fragment 8

The developers left a simple mistake for attentive readers, so I invite you to find the typo:

switch (Settings.TraktListType)
{
  case (int)TraktPopularListType.Trending:
    link += "movies/trending";
    break;
  case (int)TraktPopularListType.Popular:
    link += "movies/popular";
    break;
  case (int)TraktPopularListType.Anticipated:
    link += "movies/anticipated";
    break;
  case (int)TraktPopularListType.BoxOffice:
    link += "movies/boxoffice";
    break;
  case (int)TraktPopularListType.TopWatchedByWeek:
    link += "movies/watched/weekly";
    break;
  case (int)TraktPopularListType.TopWatchedByMonth:
    link += "movies/watched/monthly";
    break;
  case (int)TraktPopularListType.TopWatchedByYear:
    link += "movies/watched/yearly";
    break;
  case (int)TraktPopularListType.TopWatchedByAllTime:
    link += "movies/watched/all";
    break;
  case (int)TraktPopularListType.RecommendedByWeek:
    link += "movies/recommended/weekly";
    break;
  case (int)TraktPopularListType.RecommendedByMonth:
    link += "movies/recommended/monthly";
    break;
  case (int)TraktPopularListType.RecommendedByYear:
    link += "movies/recommended/yearly";
    break;
  case (int)TraktPopularListType.RecommendedByAllTime:
    link += "movies/recommended/yearly";          
    break;
}

Code fragment 9

public override bool Enable 
             =>    OnGrab 
                || OnDownload 
                ||(OnDownload && OnUpgrade) 
                || OnRename 
                || OnMovieAdded 
                || OnMovieDelete 
                || OnMovieFileDelete 
                ||(OnMovieFileDelete && OnMovieFileDeleteForUpgrade) 
                || OnHealthIssue 
                || OnHealthRestored 
                || OnApplicationUpdate 
                || OnManualInteractionRequired;

The PVS-Studio warning: V3017 A pattern was detected: (OnDownload) || ((OnDownload) && ...). The expression is excessive or contains a logical error. NotificationDefinition.cs 62

It happens that a single line contains two errors at once. I hope it's clear that conditions shouldn't be duplicated and that it's always a good idea to write code in an easy-to-read way. To make it even clearer: the OnDownload ||(OnDownload && OnUpgrade) condition is redundant because OnDownload being true is enough. The same is true for the OnMovieFileDelete || (OnMovieFileDelete && OnMovieFileDeleteForUpgrade) conditions.

Code fragment 10

if (importList == null || !definition.Enable)
{
  _logger.Debug("Import List {0} ({1}) is not enabled, skipping.", 
                importList.Name,                             // <=
                importList.Definition.Name);                 // <=
  return result;
}

The PVS-Studio warning: V3125 The 'importList' object was used after it was verified against null. Check lines: 147, 145. FetchAndParseImportListService.cs 147

There's a sloppy error here. It's obvious that the developers wanted to do a null check. However, something went wrong, and if importList is null, null dereference occurs in code snippets marked with // <=, causing the error.

Code fragment 11

if (mediaInfo.VideoHdrFormatCompatibility
             .IsNotNullOrWhiteSpace())
{
  if (mediaInfo.VideoHdrFormatCompatibility
               .ContainsIgnoreCase("HLG"))
  {
    return HdrFormat.Hlg10;
  }

  if (mediaInfo.VideoHdrFormatCompatibility
               .ContainsIgnoreCase("dolby"))
  {
    return HdrFormat.DolbyVision;
  }

  if (mediaInfo.VideoHdrFormatCompatibility           // <=
               .ContainsIgnoreCase("dolby"))
  {
    return HdrFormat.DolbyVision;                     // <=
  }

  if (mediaInfo.VideoHdrFormatCompatibility
               .ContainsIgnoreCase("hdr10+"))
  {
    return HdrFormat.Hdr10Plus;
  }

  if (mediaInfo.VideoHdrFormatCompatibility
               .ContainsIgnoreCase("hdr10"))
  {
    return HdrFormat.Hdr10;
  }
}

The PVS-Studio warning: V3022 Expression 'mediaInfo.VideoHdrFormatCompatibility.ContainsIgnoreCase("dolby")' is always false. 199_mediainfo_to_ffmpeg.cs 351

There's also a clear error here, caused by careless copy-pasting. We can safely remove the if statement with the identical condition, because it will never serve its purpose. I guess the developers forgot to update the parameter and the HdrFormat return element within the mediaInfo.VideoHdrFormatCompatibility.ContainsIgnoreCase method. I got this idea because the HdrFormat enumeration also includes DolbyVisionHdr10, DolbyVisionSdr, DolbyVisionHlg, and DolbyVisionHdr10Plus.

Code fragment 12

if (videoFormat.ContainsIgnoreCase("MPEG-4 Visual"))
{
  m.VideoFormat = "mpeg4";

  ....

  return;
}

if (   videoFormat.ContainsIgnoreCase("MPEG-4 Visual")   // <=
    || videoFormat.ContainsIgnoreCase("mp4v"))
{
  m.VideoFormat = "mpeg4";

  ....

  return;
}

The PVS-Studio warning: V3063 A part of conditional expression is always false if it is evaluated: videoFormat.ContainsIgnoreCase("MPEG-4 Visual"). 199_mediainfo_to_ffmpeg.cs 205

The first part of the condition marked with the if statement is always false, because if the condition is true, the program will stop at the first if. Based on the value assigned to the m.VideoFormat variable, I would assume this is a copy error. The condition should check whether the video format is mp4v, so mp4v should be written to the m.VideoFormat variable.

Again, without knowing the intended behavior of the program, we can't say for sure that this is an error. This is more of a program quirk that should be reported to the developers.

Code fragment 13

if (Type == TokenType.UnknownSymbol)                    // <=
{
  Value = Buffer[Index].ToString();
  Index++;
  return Type;
}
....
if (   end >= Buffer.Length 
    || Buffer[end] == ',' 
    || Buffer[end] == ')' 
    || Buffer[end] == '(' 
    || char.IsWhiteSpace(Buffer[end]))
{
  Index = end;
}
else if (Type == TokenType.UnknownSymbol)               // <=
{
  Value = Buffer[Index].ToString();
  Index++;
  return Type;
}

The PVS-Studio warning: V3022 Expression 'Type == TokenType.UnknownSymbol' is always false. SqliteSyntaxReader.cs 166

The lower Type == TokenType.UnknownSymbol expression is always false here, because the top has exactly the same condition with return within it. If the condition is true, the program execution within this method will end in the first if block.

Conclusion

To sum it up, we can say that the project doesn't contain too many errors, although we haven't covered all of them. These were mostly simple careless typos. Still, the case study of this application shows that even small things like neglecting operation precedence can sometimes cause big issues, even in large projects like Azure.

Also, one should remember that the larger the project, the greater the responsibility and the greater the density of errors. That's why the best way to catch them is to use a static analyzer at the early development stages.

If you want to write secure code and stop worrying about typos, try the PVS-Studio analyzer here.

Good luck with your projects and don't forget to check your code!