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

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\.

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

## Introduction



[Radarr](https://radarr.video/) 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](https://github.com/Radarr/Radarr/releases/tag/v5.18.2.9651)\.

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

## The project check

### A boxing error

**Code fragment 1**

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

The PVS\-Studio warning: [V3161](https://pvs-studio.com/en/docs/warnings/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:

```cpp
 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](https://pvs-studio.com/en/blog/posts/csharp/0772/) on the topic\.

**Code fragment 2**

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

The PVS\-Studio warning: [V3123](https://pvs-studio.com/en/docs/warnings/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:

```cpp
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](https://pvs-studio.com/en/docs/warnings/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:

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

[V3112](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/blog/posts/csharp/0692/)\.

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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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:

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

```cpp
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`\.


> Programmers often encounter this error pattern when using \`foreach\`\\\. We even have an article on this \[topic\]\(https://pvs\-studio\.com/en/blog/posts/csharp/0832/\)\\\.



**Code fragment 4**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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:

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

<details>
   <summary>Good job if you found the error\\\. This is where it lurks:</summary>

The PVS\-Studio warning: V3139 Two or more case\-branches perform the same actions\. TraktPopularRequestGenerator\.cs 65

The issue here is similar to the one in the previous code fragment\. I think this one is more obvious and is probably caused by careless copying and pasting\. We can see that the `link` variable in the last `case` should take the `movies/recommended/all` value instead of `movies/recommended/yearly`—the code above suggests this\.

Code review may not always catch this kind of error—human eyes tire quickly, but a static analyzer never does and will spot it almost instantly :\)


</details>


**Code fragment 9**

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

The PVS\-Studio warning: [V3017](https://pvs-studio.com/en/docs/warnings/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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/pvs-studio/try-free/)\.

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