Nikita Sviridov

Jan 21 2025

Tags:

#CSharp

Ode to one diagnostic rule and to couple more, or Jellyfin check

Jan 21 2025

Author: Nikita Sviridov

Briefly about the project
How the V3022 diagnostic rule works
Errors in the project
Conclusion

Jellyfin is a free open-source media server, which is an alternative to Emby and Plex. In this article, we'll break down the diagnostic rule that issued most warnings in the Jellyfin code. Also, we'll look into a few more bugs in the project.

Briefly about the project

Jellyfin is based on the Emby 3.5.2 release and ported to the .NET Core platform to provide full cross-platform support.

The idea behind this media server is to help collect and manage all media files from the same location. So, when installing the server, you can specify all the collected data and organize it properly. For example, when setting up a movie category, you can specify the movie selection options you want to display on the dashboard (rating, genres, name, studios, and so on). The tool supports subtitle files from external sources.

This project continues the Emby project development. Since December 2018 Emby is not an open-source multimedia system any more. To try Jellyfin and enjoy all its features, the authors recommend starting with a fresh database and a library scan.

The Jellyfin media server is popular among users, but it remained unnoticed by PVS-Studio until now. Although its ancestor—the Emby project—was checked by our analyzer. At last, the PVS-Studio analyzer got to Jellyfin. So let's move on to the errors in the project. We used the Jellyfin source code for the check, which is available for download on GitHub.

How the V3022 diagnostic rule works

V3022 is a diagnostic rule that found most errors in this project. Let's see how it works to get a better understanding of how it proved so invaluable on this project.

The V3022 rule is based on data flow analysis. Data flow analysis is a mechanism that allows a static analyzer to make assumptions about the values of variables and expressions in different parts of source code. Assumed values refer to some specific values, ranges of values, or a set of possible values. Additionally, it collects and processes information: array sizes, whether the memory is freed by the pointer, and so on.

Assumptions about values are based on the movement analysis of the variable values along the control flow graph of the program. In most cases, the analyzer can't know the exact value of a variable or expression. But it can make assumptions about the ranges or sets of values that these expressions can take at various points in the graph. When doing this, the tool uses direct and indirect constraints placed on the expressions in question as it traverses the control flow graph.

All PVS-Studio analyzers use data flow analysis to clarify the operation of their diagnostic rules. It is required in the following case. Sometimes the information from AST or the semantic code structure is not enough for a decision about the insecurity of the code.

You can learn more about PVS-Studio static analyzer technologies in this article.

Errors in the project

Errors in the condition

As written above, the V3022 rule accounted for the greatest number of warnings. This article focuses on them. Also, I added a couple more warnings to show different types of errors in the project.

V3022 triggers for various types of errors: mutually exclusive checks, an error in a condition, an always true/false condition. All this relates to errors in conditions.

Issue 1

private void ApplyTranscodingConditions(....)
{
  ....

  if (condition.Condition == 
                     ProfileConditionType.GreaterThanEqual)  // <=
  {
    continue;
  }

  switch (condition.Property)
  {
    case ProfileConditionValue.AudioBitrate:
    {
      ....
      else if (condition.Condition ==
                              ProfileConditionType.GreaterThanEqual)  // <=
      {
        ....                        
      }

      break;
    }

    case ProfileConditionValue.AudioSampleRate:
    {
      ....
      else if (condition.Condition ==
                              ProfileConditionType.GreaterThanEqual)  // <=
      {
        ....                        
      }

      break;
      ....
    }
  }
}

The PVS-Studio warning: V3022 Expression 'condition.Condition == ProfileConditionType.GreaterThanEqual' is always false. StreamBuilder.cs 2193

This is the project's most widespread mistake. The author repeated it in 10 case blocks! The example shows just some of them, as in other blocks the error is the same.

The condition at the beginning of the program is to blame for this mess. If the expression condition.Condition == ProfileConditionType.GreaterThanEqual takes the true value, the program gets to the inside of the condition and jumps to the next loop iteration due to the continue operator. Hence, there will never be true below, in case blocks. The code inside them won't execute. The author forgot about the first check and continued to copy the error into 10 case blocks. This leads to unexpected program operation and loss of its functional.

Issue 2

private static bool AllowBoxSetCollapsing(InternalItemsQuery request)
{
  ....
  if (request.IsResumable.HasValue)
  {
    return false;
  }

  if (request.IsFolder.HasValue)
  {
    return false;
  }

  if (request.Genres.Count > 0)
  {
    return false;
  }

  if (request.GenreIds.Count > 0)  // <=
  {
    return false;
  }

  ....

  if (request.GenreIds.Count > 0)  // <=
  {
    return false;
  }

  ....
}

The PVS-Studio warning: V3022 Expression 'request.GenreIds.Count > 0' is always false. Folder.cs 1243

The analyzer reports a redundant condition. The author checked the request.GenreIds.Count variable, and then did it again. Most likely, the developer wanted to check another variable and didn't notice the error.

Issue 3

private IReadOnlyList<BaseItem> GetItemsForLatestItems(....)
{
  ....
  if (parents.Count == 0)  // <=
  {
    parents = _libraryManager.GetUserRootFolder().GetChildren(user, true)
      .Where(i => i is Folder)
      .Where(i => !user.GetPreferenceValues<Guid>
                        PreferenceKind.LatestItemExcludes)
      .Contains(i.Id))
      .ToList();
  }

  if (parents.Count == 0)  // <=
  {
    return Array.Empty<BaseItem>();
  }
  ....
  if (parents.Count == 0)  // <=
  {
    return _libraryManager.GetItemList(query, false);
  }
}

PVS-Studio warning: V3022 Expression 'parents.Count == 0' is always false. UserViewManager.cs 372

Here the author checks the number of parents.Count values. If it is 0, it fills parents with values. So far, it makes sense. Then the developer checks the parents.Count variable a second time, in case it is not filled. Again, sounds logical. Then there is a third check to ensure there is definitely no mistake. But, apparently, the author didn't take into account that after the second check, if parents.Count equals 0, the program will return Array.Empty<BaseItem>() and exit this method. Therefore, the program will never get to the third condition, it is redundant here.

Issue 4

public override string CreatePresentationUniqueKey()
{
  if (IndexNumber.HasValue)
  {
    var series = Series;
    if (series is not null)
    {
      return series.PresentationUniqueKey + "-" + 
               (IndexNumber ?? 0).ToString("000", CultureInfo.InvariantCulture);
    }
  }

  return base.CreatePresentationUniqueKey();
}

The PVS-Studio warning: V3022 Expression 'IndexNumber' is always not null. The operator '??' is excessive. Season.cs 135

This code first checks the value of the IndexNumber variable and then checks it again for null. But the check is expendable, since the variable obviously has a value if it made to this if.

Similar error, again with the operator ??.

Issue 5

private int? GetOutputVideoCodecLevel(StreamState state)
{
  string levelString = string.Empty;
  if (EncodingHelper.IsCopyCodec(state.OutputVideoCodec)
       && state.VideoStream is not null
       && state.VideoStream.Level.HasValue)
  {
    levelString = state.VideoStream.Level.Value.
                        ToString(CultureInfo.InvariantCulture) ?? string.Empty;
  }
}

The PVS-Studio warning: V3022 Expression 'state.VideoStream.Level.Value.ToString(CultureInfo.InvariantCulture)' is always not null. The operator '??' is excessive. DynamicHlsHelper.cs 619

The analyzer reports an error the code that uses the ?? operator. The author expected it to get rid of the null value, since this operator only compares values to null. But neither the ToString() method nor the string.Empty parameter returns null, so the check is redundant.

Let's look at the ToString() method to ensure it doesn't return null:

public virtual string ToString()
{
  return GetType().ToString();
}

Here's what we see below:

public override string ToString()
{
  return "Type: " + Name;
}

You can see from this code that the return value is a string. This method won't return null.

Issue 6

public AggregateFolder CreateRootFolder()
{
  ....
  var path = Path.Combine(_configurationManager.ApplicationPaths.DataPath,
                          "playlists");

  ....

  Folder folder = new PlaylistsFolder
  {
    Path = path
  };

  if (folder.Id.IsEmpty())
  {
    if (string.IsNullOrEmpty(folder.Path))
    {
      ....
    }
    ....
  }
  ....
}

The PVS-Studio warning: V3022 Expression 'string.IsNullOrEmpty(folder.Path)' is always false. LibraryManager.cs 754

In this snippet, the Combine() method always returns string. If any of the paths is null, it will issue the ArgumentNullException and return nothing. Check this out below:

public static string Combine(string path1, string path2)
{
  if (path1 == null || path2 == null)
  {
    throw new ArgumentNullException((path1 == null) ? "path1" : "path2");
  }

  CheckInvalidPathChars(path1);
  CheckInvalidPathChars(path2);
  return CombineNoChecks(path1, path2);
}

Therefore, the Combine() method will always return string. Its value is passed to the Path variable. Hence, Path can't be null, so this check is redundant.

Issue 7

private async Task<ActionResult> GetMasterPlaylistInternal(....)
{
  ....
  if (EnableAdaptiveBitrateStreaming(state, ....)
  {
    ....
  }
}

The PVS-Studio warning: V3022 Expression is always false. DynamicHlsHelper.cs 268

The analyzer reports that the expression will always be false. We should look into the EnableAdaptiveAdaptiveBitrateStreaming() method to plumb the depths of the error. Here is the method code:

private bool EnableAdaptiveBitrateStreaming(StreamState state,....)
{
  if (....)
  {
    return false;
  }
  
  if (....)
  {
    return false;
  }

  if (....)
  {
    return false;
  }
  
  if (....)
  {
    return false;
  }

  if (....)
  {
    return false;
  }

  if (....)
  {
    return false;
  }
  
  // Having problems in android
  return false;
  // return state.VideoRequest.VideoBitRate.HasValue;
}

Here we go! Found a mistake! The method always returns false. I guess, the author decided to comment the functional with the bugs during the fix and then forgot to uncomment it. Judging by the comment, the code wasn't working on Android.

Any developer can easily forget about such comments and lose crucial functional. Well, this is the moral of the story—check your comments in code.

Issue 8

private string GetCommandLineArgs(MediaSourceInfo mediaSource, 
string inputTempFile, string targetFile)
{
  ....
  if (EncodeVideo(mediaSource))  // <=
  {
    const int MaxBitrate = 25000000;
    videoArgs = string.Format(CultureInfo.InvariantCulture,
               "-codec:v:0 libx264 -force_key_frames \
               "expr:gte(t,n_forced*5)\" {0} 
               -pix_fmt yuv420p -preset superfast -crf 23
               -b:v {1} -maxrate {1} -bufsize ({1}*2) -profile:v high 
               -level 41",GetOutputSizeParam(),MaxBitrate);
  }
  ....
}

The PVS-Studio warning: V3022 Expression 'EncodeVideo(mediaSource)' is always false. EncodedRecorder.cs 128

Same error as above. Similar to the previous case, let's search for an error in the called EncodeVideo() method:

private static bool EncodeVideo(MediaSourceInfo mediaSource)
{
  return false;
}

The method that always returns false?! Why would a developer implement all the functional in if()? In cases where authors intentionally disable some functional, they have to remember to enable it after the work is done. The PVS-Studio analyzer helps out in such cases. It easily found two errors where an author may have omitted some functional due to forgetfulness.

Errors with a NRE

This part is about errors caused by incorrect use of variables, not checked for null. Such pitfalls may result in NullReferenceException.

Issue 9

protected override async Task<AuthenticateResult> HandleAuthenticateAsync()
{
  ....
  if (authorizationInfo.IsApiKey || 
      authorizationInfo.User.HasPermission(PermissionKind.IsAdministrator))
  {
    ....
  }
  
  var claims = new[]
  {
    new Claim(ClaimTypes.Name,
              authorizationInfo.User?.Username ?? string.Empty),
    ....
  };
  ....
}

The PVS-Studio warning: V3095 The 'authorizationInfo.User' object was used before it was verified against null. Check lines: 53, 60. CustomAuthenticationHandler.cs 53

The author missed a bug that can lead to a NRE. Here, the developer uses a call to the authorizationInfo.User.HasPermission(PermissionKind.IsAdministrator) method. If the authorizationInfo.User variable is null, its method will result in the NRE when used. Yes, the author is checking this variable but in the wrong place—the NRE error may have already occurred.

Issue 10

private void BuildStreamVideoItem(....)
{
  ....
  if (maxBitrateSetting.HasValue)
  {
    ....
    playlistItem.VideoBitrate = Math.Max(Math.Min(availableBitrateForVideo, 
                                currentValue), 64_000);
  }
  _logger.LogDebug(
      ....
      playlistItem?.PlayMethod,
      ....);
}

The PVS-Studio warning: V3095 The 'playlistItem' object was used before it was verified against null. Check lines: 1077, 1084. StreamBuilder.cs 1077

It is a similar bug. The developer first uses the playlistItem variable and then checks it for null. This error can also lead to the NRE.

Copy-paste error

A developer failed to notice a copy-paste error due to inattention. As a result, the program doesn't work as expected.

Issue 11

private void FetchBdInfo(....)
{
  ....
  if (blurayVideoStream is not null && ffmpegVideoStream is not null)
  {
    ....
    blurayVideoStream.Width = blurayVideoStream.Width.GetValueOrDefault()
          == 0 ? ffmpegVideoStream.Width : blurayVideoStream.Width;

    blurayVideoStream.Height = blurayVideoStream.Height.GetValueOrDefault()
          == 0 ? ffmpegVideoStream.Width : blurayVideoStream.Height;
    ....
  }
}

The PVS-Studio warning: V3127 Two similar code fragments were found. Perhaps, this is a typo and 'Height' variable should be used instead of 'Width' FFProbeVideoInfo.cs 357

The PVS-Studio analyzer issued an error about identical code snippets. Most likely, the bug stemmed from inattention or copying. Given the line above, the developer should have used the ffmpegVideoStream.Height variable, but made a mistake and copied ffmpegVideoStream.Width from the previous line. Therefore, the result will be different from the intended. Another article has already covered copy-paste errors. Actually, developers often make them.

Error in string formatting

The author didn't notice this error. Nevertheless, the information from the message can be lost due to it.

Issue 12

public static byte MaskToCidr(IPAddress mask)
{
  ....
  if (!mask.TryWriteBytes(bytes, out var bytesWritten))
  {
    Console.WriteLine("Unable to write address bytes, 
                       only ${bytesWritten} bytes written.");
  }
  ....
}

The PVS-Studio warning: V3138 String literal contains potential interpolated expression. Consider inspecting: bytesWritten. NetworkUtils.cs 105

Based on the logic of the code, here the developer displays a message about the number of bytes (bytesWritten). To print this variable value, the author has to put the expression in quotes. Without the quotes, the compiler takes this variable as a string. In this case, when the condition is met, the developer will see a string with the variable name and won't get the needed information from the message.

The string formatting is missing, which is obviously a human factor. We can all make and overlook such mistakes due to fatigue or inattention. The PVS-Studio static analyzer doesn't let them pass.