Ivan Kishchenko

Mar 06 2017

Tags:

#CSharp

Brief analysis of Media Portal 2 bugs

Mar 06 2017

Author: Ivan Kishchenko

About the project Media Portal 2
The analysis results
Conclusion

Media Portal 2 is open software of a media center class, allowing the user to listen to music, watch videos, viewing pictures, and much more. For us, the developers of PVS-Studio static analyzer, this is another chance to check an interesting project, tell people (and developers) about the errors we find, and demonstrate the abilities of our analyzer of course.

About the project Media Portal 2

About the project Media Portal 2T, the project description taken from Wikipedia:

MediaPortal provides a 10-foot user interface for performing typical PVR/TiVo functionality, including playing, pausing, and recording live TV; playing DVDs, videos, and music; viewing pictures; and other functions. Plugins allow it to perform additional tasks, such as watching online video, listening to music from online services such as Last.fm, and launching other applications such as games. It interfaces with hardware commonly found in HTPCs, such as TV tuners, infrared receivers, and LCD displays.

A large part of the project is written in C#. There are separate units written in C++. Also, as far as I understand, the Media Portal 2 developers are already using ReSharper in their project. I drew this conclusion by seeing its mention in the .gitignore file. We do not like the idea of comparing PVS-Studio and ReSharper, because these are different types of tools. However, as you can see, using ReSharper did not prevent us from finding real errors in the code.

The analysis results

During the analysis we checked 3321 files. In total, there were 512,435 lines of code. As a result of the check we had 72 high level warnings. 57 of them pointed to actual errors, typos, issues and strange fragments in the code. There were also 79 second (medium) level warnings. In my own opinion, 53 warnings pointed to problematic or strange places in the code. We aren't going to look at the lowest level warnings, because these warnings usually don't indicate real errors, have quite a large number of false positives, and contain warnings that aren't relevant for most of the projects.

So the analyzer detected 0.2 errors per 1000 lines of code. The percentage of false positives is just 27%, which is a very good result.

I should say right away that sometimes it's very hard to determine what exactly programmer meant to achieve when was writing a particular piece of code. That's why those fragments that I considered erroneous can have some distorted logic, but in the scope of a certain algorithm, can work quite normally. But if this code is reused in another application, by another person who is unaware of all the nuances of implementation, then most likely it will lead to bugs in their system.

Also, I want to note that the article doesn't cover all errors, since there are too many of them for a single article.

So, let's take a look at the most interesting bugs we found; the project authors can do a more detailed review of the bugs by doing the project check themselves, or by making a request for a temporary license. Also, dear readers, if you are non-commercial or individual developers, I suggest using the free version of our static analyzer. Its functional abilities are absolutely identical to the paid version and so, it is the perfect fit for students, individual developers, and teams of enthusiasts.

Typos when using Copy-Paste

I will start the description with quite widespread errors when a programmer has copied a code block, but forgot to change one or several variables in it due to inattention.

V3127 Two similar code fragments were found. Perhaps, this is a typo and 'AllocinebId' variable should be used instead of 'CinePassionId' MovieRelationshipExtractor.cs 126

if (movie.CinePassionId > 0)
  ids.Add(ExternalIdentifierAspect.SOURCE_CINEPASSION,
    movie.CinePassionId.ToString());
if (movie.CinePassionId > 0)                            // <=
  ids.Add(ExternalIdentifierAspect.SOURCE_ALLOCINE,
    movie.AllocinebId.ToString());

It's very hard to find such bugs by doing a simple code review. Since the code is very stuck together, most likely, the programmer just hasn't noticed the defect. Looking at the line marked with a comment, you'll notice that the word Allocine is used instead of CinePassion everywhere in the second if block, but in the condition of the check the variable CinePassionId was not replaced with AllocinebId.

The diagnostic V3127 found several more interesting typos showing the danger of Copy-Paste.

V3127 Two similar code fragments were found. Perhaps, this is a typo and 'Y' variable should be used instead of 'X' PointAnimation.cs 125

double distx = (to.X - from.X) / duration;
distx *= timepassed;
distx += from.X;

double disty = (to.X - from.Y) / duration;      // <=
disty *= timepassed;
disty += from.Y;

V3127 Two similar code fragments were found. Perhaps, this is a typo and 'X' variable should be used instead of 'Y' Point2DList.cs 935

double dx1 = this[upper].Y - this[middle].X;    // <=
double dy1 = this[upper].Y - this[middle].Y;

V3127 Two similar code fragments were found. Perhaps, this is a typo and 'attrY' variable should be used instead of 'attrX' AbstractSortByComparableValueAttribute.cs 94

if (attrX != null)
{
  valX = (T?)aspectX.GetAttributeValue(attrX);
}
if (attrY != null)
{
  valX = (T?)aspectX.GetAttributeValue(attrX);   // <=
}

In all cases in the first block, the evaluations are with the x axis; in the second block with the Y axis. If we look at the commented lines, we can see that the programmer forgot to change X to Y or vice versa, when copy pasting one of the blocks.

Access by null reference

Programming language is continuously evolving, but the main way to shoot yourself in the foot remain the same. In the example of the code cited below, the programmer first verifies the BannerPath variable against null. If it is null, then he checks that it is equal to an empty string with an Equals method, which may cause a potential NullReferenceException.

V3080 Possible null dereference. Consider inspecting 'BannerPath'. TvdbBannerWithThumb.cs 91

if (ThumbPath == null && 
   (BannerPath != null || BannerPath.Equals("")))    // <=
{
  ThumbPath = String.Concat("_cache/", BannerPath);
}

This code fragment is rather strange, if we take into account the code after the check. To my mind, the check should be triggered if the variable BannerPath is not null and not an empty string.

Correct variant can be like this:

if (ThumbPath == null &&
    !string.IsNullOrEmpty(BannerPath))
{
  ThumbPath = String.Concat("_cache/", BannerPath);
}

Incorrect operator precedence

I suggest taking a look at another quite amusing warning related to incorrect precedence of logic operators.

V3130 Priority of the '&&' operator is higher than that of the '||' operator. Possible missing parentheses. BinaryCacheProvider.cs 495

return config.EpisodesLoaded || !checkEpisodesLoaded &&
       config.BannersLoaded || !checkBannersLoaded &&
       config.ActorsLoaded || !checkActorsLoaded;

The programmer who wrote this code apparently did not take into account that the logical AND operator (&&) has a higher precedence than the logical OR (||) operator. Once more, I would recommend specifying explicitly the precedence of operations, and putting parentheses between them.

Here is one more error caused by incorrect operator precedence. The programmer ignores the fact that the + operator has a higher priority than the ?? operator.

V3022 Expression '"Invalid header name: " + name' is always not null. The operator '??' is excessive. HttpRequest.cs 309

...("Invalid header name: " + name ?? "<null>");

As a result, if the variable name is zero, it will be added to the string "Invalid header name:" as an empty string, and will not be replaced by the expression "<null>". It's not a very crucial error in and of itself, and in this case it won't lead to a crash.

The corrected variant will be as follows.

...("Invalid header name: " + (name ?? "<null>"));

A typo after the type casting

One more common typo that is caused by inattention. Note the variables other and obj.

V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'obj', 'other'. EpisodeInfo.cs 560

EpisodeInfo other = obj as EpisodeInfo;
if (obj == null) return false;           // <=
if (TvdbId > 0 && other.TvdbId > 0)
  return TvdbId == other.TvdbId;
....

In this code fragment a variable obj is explicitly cast to the EpisodeInfo type, and the result is returned to the variable other. Note that further on we see a variable other being used, but the variable obj gets verified against null. In the case that the variable obj we'll have is a type different from what it is cast to, then to work with the other variable will lead to an exception.

Here is how a fixed code fragment may look:

EpisodeInfo other = obj as EpisodeInfo;
if (other == null) return false;
if (TvdbId > 0 && other.TvdbId > 0)
  return TvdbId == other.TvdbId;
....

Double assignment

One more amusing error that was found by the analyzer. The following code fragment would be meaningless, because the Released variable will always be equal to null.

V3008 The 'Released' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 57, 56. OmDbSeasonEpisode.cs 57

DateTime releaseDate;
if (DateTime.TryParse(value, out releaseDate))
  Released = releaseDate;                       // <=
Released = null; // <=

Most likely this statement with the nullification should be written in the else block. Then the correct code fragment will be like this:

DateTime releaseDate;
if (DateTime.TryParse(value, out releaseDate))
  Released = releaseDate;                    // <=
else
  Released = null;                           // <=

When a minute doesn't always have 60 seconds

V3118 Milliseconds component of TimeSpan is used, which does not represent full time interval. Possibly 'TotalMilliseconds' value was intended instead. Default.cs 60

private void WaitForNextFrame()
{
  double msToNextFrame = _msPerFrame - 
    (DateTime.Now - _frameRenderingStartTime).Milliseconds;
  if (msToNextFrame > 0)
    Thread.Sleep(TimeSpan.FromMilliseconds(msToNextFrame));
}

Another fairly common typo that occurs because of the TimeSpan type implementation. But apparently, the programmer did not know that the Seconds property of the object of TimeSpan type returns not the total number of seconds in this interval, but the remaining number of seconds.

For example, if the time interval is 1minute, 150 seconds, then the call of the Milliseconds method will return only 150 miliseconds. If it is necessary to return a total number of seconds, we should use the method TotalMilliseconds. For this example it will be 1150 milliseconds.

Then the correct code could be as follows:

double msToNextFrame = _msPerFrame - 
  (DateTime.Now - _frameRenderingStartTime).TotalMilliseconds;

Incorrect order of arguments

One more mistake caused by inattentiveness. The method TryCreateMultimediaCDDrof iveHandler gets enumerations of identifiers for videos, pictures, and audio in the specified sequence.

V3066 Possible incorrect order of arguments passed to 'TryCreateMultimediaCDDriveHandler' method. RemovableMediaManager.cs 109

public static MultimediaDriveHandler
  TryCreateMultimediaCDDriveHandler(DriveInfo driveInfo,
    IEnumerable<Guid> videoMIATypeIds, 
    IEnumerable<Guid> imageMIATypeIds,           // <= 
    IEnumerable<Guid> audioMIATypeIds)           // <= 
  { .... }

Since these parameters have the same types, the programmer didn't pay attention to the fact that when he was passing arguments to the method, he misplaced pictures and audios:

public static ....()
{
  MultimediaDriveHandler.TryCreateMultimediaCDDriveHandler(driveInfo,
    Consts.NECESSARY_VIDEO_MIAS, 
    Consts.NECESSARY_AUDIO_MIAS,          // <= 
    Consts.NECESSARY_IMAGE_MIAS)          // <=
}

A condition that is always false

This code is pretty strange, so I was not sure if I should put it here or not.

V3022 Expression 'IsVignetteLoaded' is always false. TvdbFanartBanner.cs 219

if (IsVignetteLoaded)         // <=
{
  Log.Warn(....);
  return false;
}
try
{
  if (IsVignetteLoaded)       // <=
  {
    LoadVignette(null);
  }
....

I can assume that the first check was added for debugging, and that most likely, the programmer forgot to remove it. As a result it blocks the second check which can lead to incorrect program execution.

Redundant check or an egregious error?

V3001 There are identical sub-expressions 'screenWidth != _screenSize.Width' to the left and to the right of the '||' operator. MainForm.cs 922

if (bitDepth != _screenBpp ||
    screenWidth != _screenSize.Width ||
    screenWidth != _screenSize.Width)      // <=
{
  ....
}

Pay attention to the last check: Most likely, the programmer wanted to check the width and height, but after Copy-Paste forgot to replace Width with Height in the last check.

The analyzer found one more similar bug:

V3001 There are identical sub-expressions 'p == null' to the left and to the right of the '||' operator. TriangulationConstraint.cs 141

public static uint CalculateContraintCode(
  TriangulationPoint p, TriangulationPoint q)
{
  if (p == null || p == null)                 // <=
  {
    throw new ArgumentNullException();
  }
  
  ....
}

Looking at the body of the method more thoroughly, you may notice that the p parameter is verified against null twice, at the same time the logic of this method presupposes using the q parameter. Most likely, the right part of the check should contain a check of the q variable instead of p.

A forgotten condition and some more Copy-Paste

As you may have noticed, the biggest part of errors in this article are caused by Copy-paste, the following bug is no exception.

V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 452, 462. Scanner.cs 452

if (style == NumberStyles.Integer)
{
  int ivalue;
  if (int.TryParse(num, out ivalue))
    return ivalue;
  ....
}
else if (style == NumberStyles.Integer) // <=
{
  return double.Parse(num);
}

In both checks the variable style is compared with one and the same value in the enumeration. As a result, the second check will never be executed. If we keep in mind the fact that during the first check the string is converted to an integer, and in the second check to a floating-point number. I may assume that the condition of the second check should be as follows:

....
}
else if (style == NumberStyles.Double) // <=
{
  return double.Parse(num);
}

Conclusion

There were many more errors, typos, and issues found in this project. But they didn't seem interesting enough to describe in this article. In general, I can say that the codebase of the project is not very readable, and contains a lot of strange fragments. The majority weren't cited in the article, but I would still consider them bad coding style. This could include using a foreach loop, to get the first element of the collection and exit using the break at the end of the first iteration, numerous redundant checks, large unseperated blocks of code, etc.

The Media Portal 2 developers can easily find all the issues, using the PVS-Studio tool. You may also find bugs in your projects with the help of the mentioned tool.

I would like to mention that the biggest benefit from static analysis, is gained by regular use. It's not enough to download the tool, and do a one-time check. As an analogy, programmers regularly review the compiler warnings, not just 3 times a year before release. If the analyzer is used on a regular basis, it will save significant time that is usually spent on searching for typos and errors.

#CSharp