We are looking for and analyzing errors in the code Media Portal 2

Media Portal 2 is an open source media center software that allows you to watch videos, photos, listen to music and more. For us, the developers of the PVS-Studio static analyzer, this is another opportunity to check an interesting project, tell people (and developers including) about the errors found and, in turn, once again show the capabilities of our analyzer.

About Media Portal 2

The project description is taken from Wikipedia :

MediaPortal allows the computer to solve various entertainment-oriented tasks, such as recording, pausing and rewinding television broadcasts, like DVR systems (such as TiVo ). Other functionality includes watching videos, listening to music with building dynamic playlists based on data from the music social network Last.fm, launching games, recording radio broadcasts and viewing image collections. MediaPortal supports a system of plug-ins and design schemes, allowing to extend the basic functionality.
')
The overwhelming part of the project is written in C #. There are separate modules written in C ++. As far as I understand, the developers of Media Portal 2 use in their work on the ReSharper project. I make this conclusion in connection with its mentioning in the .gitignore file. We do not like the idea of ​​comparing ReSharper and PVS-Studio, since these are tools of different types. But, as you can see, using ReSharper does not prevent us from finding real errors in the code.

Test results

3321 files participated in the check. In total, they contained 512,435 lines of code. As a result of checking in the server project at the first (high) level, 72 warnings were received. Of these, 57 explicitly or indirectly pointed out errors, typos, problem and strange places in the code. At the second level (average), 79 warnings were received. In my subjective opinion, the 53 warnings correctly indicated problematic or strange places in the code. We will not consider the third (low) level of warnings, as these are warnings with a low level of confidence and, as a rule, among them there are a lot of false positives or there are warnings that are not relevant for many types of projects.

The analyzer revealed 0.2 errors per 1000 lines of code. The percentage of false positives is only 27%, which is a very good result.

I’ll just make a reservation that it’s pretty hard to determine what the programmer meant when writing this or that code, so the places that I thought were wrong could have perverted logic and work fine within a particular algorithm, but if this code is reused in another a person who does not know all the nuances of implementation, then with a high degree of probability this can lead to errors. I also want to note that the article presents not all errors.

So, we proceed to the consideration of the most interesting of the errors found. In more detail, the authors of the project will be able to study the errors by requesting a temporary license from us and performing the verification themselves. Also, dear readers, if you are a non-commercial developer, then I suggest you use the free version of our static analyzer. Its functionality is absolutely identical with a paid license, so it is ideal for students, individual developers and enthusiast teams.

Typos with Copy-Paste

I’ll start, perhaps, with a number of fairly common mistakes, when a programmer, having copied one block of code, inadvertently forgot to replace one or more variables in it.

V3127 Two code fragments were found. Perhaps, this is a typo and an 'AllocinebId' variable should be used instead of a '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()); 

Such errors are very difficult to find in the usual Code Review. Since the code is very heavily plastered, it is likely that the programmer simply does not notice the defect. Looking at the line marked with a comment, you can see that in the second if block, the word Allocine is used instead of CinePassion everywhere , but in the verification condition you probably forgot to replace the variable CinePassionId with AllocinebId .

In addition to this error, the V3127 diagnostics has found some more interesting typos that show all the danger of Copy-Paste.

V3127 Two 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 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 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, calculations are performed with the X axis, and in the second block, with the Y axis. If you look at the lines marked with comments, you can see that the programmer forgot to replace X with Y or vice versa during Copy-Paste of one of the blocks.

Access via null link

Programming languages ​​evolve, but the main ways to shoot yourself in the foot remain. In the code section below, the programmer first checks whether the BannerPath variable is not zero. If, however, it is still zero, then the Equals method checks its equality to the empty string, which in fact may be a potential cause of the NullReferenceException exception.

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

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

This piece of code is rather strange, considering the code under verification. In my opinion, the check should work if the BannerPath variable is not null and an empty string.

Then the corrected version could look like this:

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

Incorrect operator priority

I propose to consider another equally interesting triggering associated with the wrong priority of logical 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 priority than the logical OR operator (||) . Again, once again, I advise you to explicitly indicate the order of operations and separate them with brackets.

Another error due to incorrect operator precedence. The programmer did not take into account 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 name variable is zero, it will be added to the string “Invalid header name:” as an empty string, and will not be replaced with the expression "<null>" . In itself, this is not a gross mistake and in this context will not lead to a fall.

The corrected version would look like this:

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

Typo after type casting

Another fairly common typo made by inattention. Notice the other and obj variables.

V3019 Possibly incorrectly variable conversion 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 part of the code, the obj variable is first explicitly cast to the EpisodeInfo type, and the result of the cast is written to the other variable. Notice that further the variable other is used everywhere, but the variable obj is checked for null . If the obj variable is of a type other than the one to which it leads, then further work with the other variable will result in an exception.

The corrected part of the code will look like this:

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

Double assignment

Another interesting error found by the static analyzer. The code section below will not make sense, since the Released variable will always be set to zero.

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 the expression with zeroing should be in the else block. Then the corrected version of this code section would look like this:

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

When in the minute is not always 60 seconds

V3118 Milliseconds component of TimeSpan is used, which does not constitute the 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, arising from the not quite obvious implementation of the TimeSpan type. The programmer apparently did not know that the Milliseconds property of an object of type TimeSpan does not return the total number of milliseconds in this time interval, but the residual number of milliseconds.

For example, if we have a time span of 1 second 150 milliseconds, then the call to the Milliseconds method will return us only 150 milliseconds. If you need to return a total quantity, you must use the TotalMilliseconds property. For this example, it will be 1150 milliseconds.

The correct version would look like this:

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

Wrong order of arguments

Another mistake from the category of inadvertent misprints. The TryCreateMultimediaCDDriveHandler method accepts enumerations of identifiers for video, images and audio in the specified sequence.

V3066 PossibleCreateMultimediaCDDriveHandler 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 did not pay attention to the fact that when passing arguments to the method, I messed up the image and audio:

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

The condition is always false

This code is rather strange, so I wondered for a long time whether to include it in the article:

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 purposes, and apparently the programmer forgot to remove it. As a result, it blocks the second check, which can lead to incorrect operation of the program.

Excess check, or gross typo?

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

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

Check out the latest check. Most likely the programmer wanted to check the width and height, but after Copy-Paste he forgot to replace Width with Height in the last test.

The analyzer also found another similar error:

V3001 There are identical sub-expressions "p == null" || operator. TriangulationConstraint.cs 141

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

If we look at the body of the method in more detail, we can notice that the p parameter is checked twice for null , and the logic of this method also implies the use of the q parameter. Most likely, the right part of the check should contain checking the variable q instead of p .

Forgotten condition or some more copy-paste

As you can see, most of the errors in this article are typos during Copy-Paste, and the following error is no exception.

V3003 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility 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 tests, the style variable is compared with the same value in the enumeration. As a consequence, the second check will never be performed. If we take into account the fact that in the first check the string is converted to an integer, and in the second check - to a floating point number. Most likely the condition of the second check should look like this:

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

Conclusion

In this project, there were other errors, typos, shortcomings. But they did not seem interesting to me to describe them in the article. In general, I can say that the code base of the project is poorly readable and contains many strange places. Most of them were not described in the article, but I still attribute them to the poor style of writing code. This could include using the foreach loop to get the first element of the collection and exit from it using break at the end of the first iteration, numerous redundant checks, large solid-cast blocks of code, etc.

Media Portal 2 developers can easily find all the shortcomings using the PVS-Studio tool. You can also look for errors in your projects using the static analyzer proposed above.

I want to remind you that the main advantage of static analysis is its regular use. Download and check one-time code - this is not serious. For example, programmers watch the compiler warnings regularly, rather than turning them on every 3 years before one of the releases. With regular use, a static analyzer will save a lot of time searching for typos and errors.

IMPORTANT

We decided to try a new format to popularize static code analysis and the PVS-Studio tool. Now we will record streams and demonstrate how to use PVS-Studio to find errors, deal with bugs and false positives. We will answer questions and practice interactive communication with the audience, trying to adapt to their interests and show what most corresponds to the wishes of the audience.

The first stream devoted to this article. In online mode, we show you how to check a Media Portal 2 project and find errors. This is the first experiment, so please treat with understanding. We warn you in advance that due to inexperience we may face technical overlays. However, we ask all readers to join the viewing. If you like the format, we will practice such events regularly.

The stream will be held today (03/06/2017) at 15:00. We are waiting for all pvs_studio_ru !

And, of course, we offer everyone to subscribe to our channel.



If you want to share this article with an English-speaking audience, then please use the link to the translation: Ivan Kishchenko. Brief analysis of Media Portal 2 bugs