C++/C# bad practices: learn how to make a good code by bad example

Analysis of .NET Core Libraries (CoreFX)

About a year ago Microsoft made the CoreCLR and CoreFX source code open. The latter project wasn’t of a big interest to us until recently, as it was written in C#, not in C++. But with the release of PVS-Studio 6.00 that now supports C# I decided to go back to the CoreFX and write an article about its analysis.

Introduction

.NET Core is a modular runtime and library implementation that includes a subset of the .NET Framework.NET Core consists of a set of libraries, called “CoreFX”, and a small, optimized runtime, called “CoreCLR”.

CoreFX project that we are going to discuss in this article was checked with the help of a static code analyzer PVS-Studio 6.00 that now has C# support.

Analysis results.

Preparing an article about the check of an open source project, we report only about a certain number of all of the warnings issued by the analyzer. Therefore we recommend the authors of the project to run the analyzer on their code themselves and study the complete analysis results.

The most dangerous code fragments.

V3027 The variable ‘start.BaseMapping’ was utilized in the logical expression before it was verified against null in the same logical expression. Mappings.cs 598

We see a serious logical error here! An object with the ‘start’ name in the body of the loop gets changed during every iteration and the loop executes while the object is in a particular state. BUT the check of the “start.BaseMapping != null” condition is done only after the access to “start.BaseMapping.IsSequence”, which can lead to the dereference of the null reference.

Object of any type or null can be passed to the function. If the null gets passed, this case will be handled incorrectly. If it is an object of a type that cannot be converted to “CredentialHostKey” type, then there will be an error accessing the “comparedCredentialKey.AuthenticationType”, because the variable “comparedCredentialKey” can be null.

A substring from the ‘localName’, that has a “colon” length, is saved into the ‘prefix’ variable and then the value is replaced with a different one. Further on we see that the remaining substring from the ‘localName’ is still used while the first part is lost. A very questionable code fragment.

The analyzer detected a condition that has already been checked. If you look at the code fragment, the last check in the ‘else’ – “baseTableRowCounts == null” makes no sense. You may also see that if the “baseTableRowCounts” variable is null, the programmer tries to change its value by calling the CalculateBaseCounts() function. Most likely an extra “baseTableRowCounts == null” check is missing after this function. I.e. the code was probably meant to look like this:

The variable “readercount” has an unsigned type, so the condition “readercount >= 0” is meaningless. Perhaps it is used to be a signed type variable, so there was some chance for the ExitMyLOck() function to execute in the last ‘else’. Now this code never gets control. This fragment has to be rewritten.

V3014 It is likely that a wrong variable is being incremented inside the ‘for’ operator. Consider reviewing ‘i’. RegexCharClass.cs 1094

The analyzer detected a change of one loop counter in a different loop. It is difficult to say whether there is an error in this function, but the code is written not very clearly. It is quite possible to make a mistake somewhere in the index when accessing the array, because it’s difficult to monitor the changes of one counter in several loops.

V3004 The ‘then’ statement is equivalent to the ‘else’ statement. XmlSerializationWriterILGen.cs 1213

There are also too many similar code fragments, although in the commentary it is written that the situations are different.

Conclusion

Here it is – another Microsoft project analysis. The code is rather qualitative, taking into account the considerable size of the project. But the programmers can still make errors. This article gives only an overview of the bugs found and the list of the warnings provided here is far from being complete.

Two main factors that facilitate safe and high-quality code:

Regular, not casual static analysis;

Review of the analyzer warnings should be done by the authors of the corresponding fragments.

We hope you enjoyed this article. In the future there will be more articles about the checks of the projects written in C/C++ and C#.