Checking the .NET Core Libraries Source Code by the PVS-Studio Static Analyzer
.NET Core libraries is one of the most popular C# projects on GitHub. It’s hardly a surprise, since it’s widely known and used. Owing to this, an attempt to reveal the dark corners of the source code is becoming more captivating. So this is what we’ll try to do with the help of the PVS-Studio static analyzer. What do you think — did I manage to eventually find something interesting?
I’ve been making my way toward this article for over a year and a half. At some point, I had an idea settled in my head that the .NET Core libraries is a tidbit, and its check is of great promise. I was checking the project several times, the analyzer kept finding more and more interesting code fragments, but it didn’t go further than just scrolling the list of warnings. And here it is — it finally happened! The project is checked, the article is right in front of you.
Details about the Project and Check
If you’re striving to dive into code investigation — you can omit this section. However, I’d very much like you to read it, as here I’m telling more about the project and analyzer, as well as about undertaking the analysis and reproducing errors.
Project under the Check
Perhaps, I could have skipped telling what CoreFX (.NET Core Libraries) is, but in case you haven’t heard about it, the description is given below. It’s the same as at the project page on GitHub, where you can also download the source code.
Description: This repo contains the library implementation (called «CoreFX») for .NET Core. It includes System.Collections, System.IO, System.Xml, and many other components. The corresponding .NET Core Runtime repo (called «CoreCLR») contains the runtime implementation for .NET Core. It includes RyuJIT, the .NET GC, and many other components. Runtime-specific library code (System.Private.CoreLib) lives in the CoreCLR repo. It needs to be built and versioned in tandem with the runtime. The rest of CoreFX is agnostic of runtime-implementation and can be run on any compatible .NET runtime (e.g. CoreRT).
Used Analyzer and the Analysis Method
I checked the code using the PVS-Studio static analyzer. Generally speaking, PVS-Studio can analyze not only the C# code, but also C, C++, Java. The C# code analysis is so far working only under Windows, whereas the C, C++, Java code can be analyzed under Windows, Linux, macOS.
Usually for checking C# projects I use the PVS-Studio plugin for Visual Studio (supports the 2010–2019 versions), because probably it’s the most simple and convenient analysis scenario in this case: open solution, run the analysis, handle the warnings list. However, it came out a little more complicated with CoreFX.
The tricky part is that the project doesn’t have a single .sln file, therefore opening it in Visual Studio and performing a full analysis, using the PVS-Studio plugin, isn’t possible. It’s probably a good thing — I don’t really know how Visual Studio would cope with a solution of this size.
However, there were no problems with the analysis, as the PVS-Studio distribution includes the analyzer command line version for MSBuild projects (and .sln). All that I had to do is write a small script, which would run «PVS-Studio_Cmd.exe» for each .sln in the CoreFX directory and save results in a separate directory (it is specified by a command line flag of the analyzer).
Presto! As a result, I’m having a Pandorra box with a set of reports storing some interesting stuff. If desired, these logs could be combined with the PlogConverter utility, coming as a part of the distributive. For me, it was more convenient to work with separate logs, so I didn’t merge them.
When describing some errors I refer to the documentation from docs.microsoft.com and NuGet packages, available to download from nuget.org. I assume that the code described in the documentation/packages may be slightly different from the code analyzed. However, it’d be very strange if, for example, the documentation didn’t describe generated exceptions when having a certain input dataset, but the new package version would include them. You must admit it would be a dubious surprise. Reproducing errors in packages from NuGet using the same input data that was used for debugging libraries shows that this problem isn’t new. Most importantly, you can 'touch' it without building the project from sources.
Thus, allowing for the possibility of some theoretical desynchronization of the code, I find it acceptable to refer to the description of relevant methods at docs.microsoft.com and to reproduce problems using packages from nuget.org.
In addition, I’d like to note that the description by the given links, the information (comments) in packages (in other versions) could have been changed over the course of writing the article.
Other Checked Projects
By the way, this article isn’t unique of its kind. We write other articles on project checks. By this link you can find the list of checked projects. Moreover, on our site you’ll find not only articles of project checks, but also various technical articles on C, C++, C#, Java, as well as some interesting notes. You can find all this in the blog.
My colleague has already previously checked .NET Core libraries in the year 2015. The results of the previous analysis can be found in the relevant article: «Christmas Analysis of .NET Core Libraries (CoreFX)».
Detected Errors, Suspicious and Interesting Fragments
As always, for greater interest, I suggest that you first search for errors in the given fragments yourself, and only then read the analyzer message and description of the problem.
For convenience, I’ve clearly separated the pieces from each other using Issue N labels — this way it’s easier to know where the description of one error ends, following by next one. In addition, it’s easier to refer to specific fragments.
Issue 1
abstract public class Principal : IDisposable
{
....
public void Save(PrincipalContext context)
{
....
if ( context.ContextType == ContextType.Machine
|| _ctx.ContextType == ContextType.Machine)
{
throw new InvalidOperationException(
SR.SaveToNotSupportedAgainstMachineStore);
}
if (context == null)
{
Debug.Assert(this.unpersisted == true);
throw new InvalidOperationException(SR.NullArguments);
}
....
}
....
}
PVS-Studio warning: V3095 The 'context' object was used before it was verified against null. Check lines: 340, 346. Principal.cs 340
Developers clearly state that the null value for the context parameter is invalid, they want to emphasize this by using the exception of the InvalidOperationException type. However, just above in the previous condition we can see an unconditional dereference of the reference context — context.ContextType. As a result, if the context value is null, the exception of the NullReferenceException type will be generated instead of the expected InvalidOperationExcetion.
Let’s try to reproduce the problem. We’ll add reference to the library System.DirectoryServices.AccountManagement to the project and execute the following code:
GroupPrincipal groupPrincipal
= new GroupPrincipal(new PrincipalContext(ContextType.Machine));
groupPrincipal.Save(null);
GroupPrincipal inherits from the Principal abstract class which implements the Save method we’re interested in. So we execute the code and see what was required to prove.
For the sake of interest, you can try to download the appropriate package from NuGet and repeat the problem in the same way. I installed the package 4.5.0 and obtained the expected result.
Issue 2
private SearchResultCollection FindAll(bool findMoreThanOne)
{
searchResult = null;
DirectoryEntry clonedRoot = null;
if (_assertDefaultNamingContext == null)
{
clonedRoot = SearchRoot.CloneBrowsable();
}
else
{
clonedRoot = SearchRoot.CloneBrowsable();
}
....
}
PVS-Studio warning: V3004 The 'then' statement is equivalent to the 'else' statement. DirectorySearcher.cs 629
Regardless of whether the _assertDefaultNamingContext == null condition is true or false, the same actions will be undertaken, as then and else branches of the if statement have the same bodies. Either there should be another action in a branch, or you can omit the if statement not to confuse developers and the analyzer.
Issue 3
public class DirectoryEntry : Component
{
....
public void RefreshCache(string[] propertyNames)
{
....
object[] names = new object[propertyNames.Length];
for (int i = 0; i < propertyNames.Length; i++)
names[i] = propertyNames[i];
....
if (_propertyCollection != null && propertyNames != null)
....
....
}
....
}
PVS-Studio warning: V3095 The 'propertyNames' object was used before it was verified against null. Check lines: 990, 1004. DirectoryEntry.cs 990
Again, we see a strange order of actions. In the method, there’s a check propertyNames!= null, i.e. developers cover their bases from null coming into the method. But above you can see a few access operations by this potentially null reference — propertyNames.Length and propertyNames[i]. The result is quite predictable — the occurrence of an exception of the NullReferenceExcepption type in case if a null reference is passed to the method.
What a coincidence! RefreshCache is a public method in the public class. What about trying to reproduce the problem? To do this, we’ll include the needed library System.DirectoryServices to the project and we’ll write code like this:
DirectoryEntry de = new DirectoryEntry();
de.RefreshCache(null);
After we execute the code, we can see what we expected.
Just for kicks, you can try reproducing the problem on the release version of the NuGet package. Next, we add reference to the System.DirectoryServices package (I used the version 4.5.0) to the project and execute the already familiar code. The result is below.
Issue 4
Now we’ll go from the opposite — first we’ll try to write the code, which uses a class instance, and then we’ll look inside. Let’s refer to the System.Drawing.CharacterRange structure from the System.Drawing.Common library and same-name NuGet package.
We’ll use this piece of code:
CharacterRange range = new CharacterRange();
bool eq = range.Equals(null);
Console.WriteLine(eq);
Just in case, in order to just jog our memory, we’ll address docs.microsoft.com to recall what returned value is expected from the expression obj.Equals (null):
The following statements must be true for all implementations of the Equals (Object) method. In the list, x, y, and z represent object references that are not null.
…
x.Equals (null) returns false.
Do you think the text «False» will be displayed in the console? Of course, not. It would be too easy. :) Therefore, we execute the code and look at the result.
It was the output from the above code using the NuGet System.Drawing.Common package of the version 4.5.1. The next step is to run the same code with the debugging library version. This is what we see:
Now let’s look at the source code, in particular, the implementation of the Equals method in the CharacterRange structure and the analyzer warning:
public override bool Equals(object obj)
{
if (obj.GetType() != typeof(CharacterRange))
return false;
CharacterRange cr = (CharacterRange)obj;
return ((_first == cr.First) && (_length == cr.Length));
}
PVS-Studio warning: V3115 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. CharacterRange.cs 56
We can observe, what had to be proved — the obj parameter is improperly handled. Due to this, the NullReferenceException exception occurs in the conditional expression when calling the instance method GetType.
Issue 5
While we’re exploring this library, let’s consider another interesting fragment — the Icon.Save method. Before the research, let’s look at the method description.
There’s no description of the method:
Let’s address docs.microsoft.com — «Icon.Save (Stream) Method». However, there are also no restrictions on inputs or information about the exceptions generated.
Now let’s move on to code inspection.
public sealed partial class Icon :
MarshalByRefObject, ICloneable, IDisposable, ISerializable
{
....
public void Save(Stream outputStream)
{
if (_iconData != null)
{
outputStream.Write(_iconData, 0, _iconData.Length);
}
else
{
....
if (outputStream == null)
throw new ArgumentNullException("dataStream");
....
}
}
....
}
PVS-Studio warning: V3095 The 'outputStream' object was used before it was verified against null. Check lines: 654, 672. Icon.Windows.cs 654
Again, it’s the story we already know — possible dereference of a null reference, as the method’s parameter is dereferenced without checking for null. Once again, a successful coincidence of circumstances — both the class and method are public, so we can try to reproduce the problem.
Our task is simple — to bring code execution to the expression outputStream.Write (_iconData, 0, _iconData.Length); and at the same time save the value of the variable outputStream — null. Meeting the condition _iconData!= null is enough for this.
Let’s look at the simplest public constructor:
public Icon(string fileName) : this(fileName, 0, 0)
{ }
It just delegates the work to another constructor.
public Icon(string fileName, int width, int height) : this()
{
using (FileStream f
= new FileStream(fileName, FileMode.Open,
FileAccess.Read, FileShare.Read))
{
Debug.Assert(f != null,
"File.OpenRead returned null instead of throwing an exception");
_iconData = new byte[(int)f.Length];
f.Read(_iconData, 0, _iconData.Length);
}
Initialize(width, height);
}
That’s it, that’s what we need. After calling this constructor, if we successfully read data from the file and there are no crashes in the Initialize method, the field _iconData will contain a reference to an object, this is what we need.
It turns out that we have to create the instance of the Icon class and specify an actual icon file to reproduce the problem. After this we need to call the Save method, having passed the null value as an argument, that’s what we do. The code might look like this, for example:
Icon icon = new Icon(@"D:\document.ico");
icon.Save(null);
The result of the execution is expected.
Issue 6
We continue the review and move on. Try to find 3 differences between the actions, executed in the case CimType.UInt32 and other case.
private static string
ConvertToNumericValueAndAddToArray(....)
{
string retFunctionName = string.Empty;
enumType = string.Empty;
switch(cimType)
{
case CimType.UInt8:
case CimType.SInt8:
case CimType.SInt16:
case CimType.UInt16:
case CimType.SInt32:
arrayToAdd.Add(System.Convert.ToInt32(
numericValue,
(IFormatProvider)CultureInfo.InvariantCulture
.GetFormat(typeof(int))));
retFunctionName = "ToInt32";
enumType = "System.Int32";
break;
case CimType.UInt32:
arrayToAdd.Add(System.Convert.ToInt32(
numericValue,
(IFormatProvider)CultureInfo.InvariantCulture
.GetFormat(typeof(int))));
retFunctionName = "ToInt32";
enumType = "System.Int32";
break;
}
return retFunctionName;
}
Of course, there are no differences, as the analyzer warns us about it.
PVS-Studio warning: V3139 Two or more case-branches perform the same actions. WMIGenerator.cs 5220
Personally, this code style is not very clear. If there is no error, I think, the same logic shouldn’t have been applied to different cases.
Issue 7
Microsoft.CSharp Library.
private static IList>
QueryDynamicObject(object obj)
{
....
List names = new List(mo.GetDynamicMemberNames());
names.Sort();
if (names != null)
{ .... }
....
}
PVS-Studio warning: V3022 Expression 'names!= null' is always true. DynamicDebuggerProxy.cs 426
I could probably ignore this warning along with many similar ones that were issued by the diagnostics V3022 and V3063. There were many (a lot of) strange checks, but this one somehow got into my soul. Perhaps, the reason lies in what happens before comparing the local names variable with null. Not only does the reference get stored in the names variable for a newly created object, but the instance Sort method is also called. Sure, it’s not an error but, as for me, is worth paying attention to.
Issue 8
Another interesting piece of code:
private static void InsertChildNoGrow(Symbol child)
{
....
while (sym?.nextSameName != null)
{
sym = sym.nextSameName;
}
Debug.Assert(sym != null && sym.nextSameName == null);
sym.nextSameName = child;
....
}
PVS-Studio warning: V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'sym' object SymbolStore.cs 56
Look what the thing is. The loop ends upon compliance of at least one of two conditions:
- sym == null;
- sym.nextSameName == null.
There are no problems with the second condition, which cannot be said about the first one. Since the names instance field is unconditionally accessed below and if sym — null, an exception of the NullReferenceException type will occur.
«Are you blind? There is the Debug.Assert call, where it’s checked that sym!= null» — someone might argue. Quite the opposite, that’s the point! When working in the Release version, Debug.Assert won’t be of any help and with the condition above, all that we’ll get is NullReferenceException. Moreover, I’ve already seen a similar error in another project from Microsoft — Roslyn, where a similar situation with Debug.Assert took place. Let me turn aside for a moment for Roslyn.
The problem could be reproduced either when using Microsoft.CodeAnalysis libraries, or right in Visual Studio when using Syntax Visualizer. In Visual Studio 16.1.6 + Syntax Visualizer 1.0 this problem can still be reproduced.
This code is enough for it:
class C1
{
void foo()
{
T1 val = default;
if (val is null)
{ }
}
}
Further, in Syntax Visualizer we need to find the node of the syntax tree of the ConstantPatternSyntax type, corresponding to null in the code and request TypeSymbol for it.
After that, Visual Studio will restart. If we go to Event Viewer, we’ll find some information on problems in libraries:
Application: devenv.exe
Framework Version: v4.0.30319
Description: The process was terminated due to an unhandled exception.
Exception Info:
System.Resources.MissingManifestResourceException
at System.Resources.ManifestBasedResourceGroveler
.HandleResourceStreamMissing(System.String)
at System.Resources.ManifestBasedResourceGroveler.GrovelForResourceSet(
System.Globalization.CultureInfo,
System.Collections.Generic.Dictionary'2
, Boolean, Boolean,
System.Threading.StackCrawlMark ByRef)
at System.Resources.ResourceManager.InternalGetResourceSet(
System.Globalization.CultureInfo, Boolean, Boolean,
System.Threading.StackCrawlMark ByRef)
at System.Resources.ResourceManager.InternalGetResourceSet(
System.Globalization.CultureInfo, Boolean, Boolean)
at System.Resources.ResourceManager.GetString(System.String,
System.Globalization.CultureInfo)
at Roslyn.SyntaxVisualizer.DgmlHelper.My.
Resources.Resources.get_SyntaxNodeLabel()
....
As for the issue with devenv.exe:
Faulting application name:
devenv.exe, version: 16.1.29102.190, time stamp: 0x5d1c133b
Faulting module name:
KERNELBASE.dll, version: 10.0.18362.145, time stamp: 0xf5733ace
Exception code: 0xe0434352
Fault offset: 0x001133d2
....
With debugging versions of Roslyn libraries, you can find the place where there was an exception:
private Conversion ClassifyImplicitBuiltInConversionSlow(
TypeSymbol source, TypeSymbol destination,
ref HashSet useSiteDiagnostics)
{
Debug.Assert((object)source != null);
Debug.Assert((object)destination != null);
if ( source.SpecialType == SpecialType.System_Void
|| destination.SpecialType == SpecialType.System_Void)
{
return Conversion.NoConversion;
}
....
}
Here, the same as in the code from .NET Core libraries considered above, there is a check of Debug.Assert which wouldn’t help when using release versions of libraries.
Issue 9
We’ve got a little offpoint here, so let’s get back to .NET Core libraries. The System.IO.IsolatedStorage package contains the following interesting code.
private bool ContainsUnknownFiles(string directory)
{
....
return (files.Length > 2 ||
(
(!IsIdFile(files[0]) && !IsInfoFile(files[0]))) ||
(files.Length == 2 && !IsIdFile(files[1]) && !IsInfoFile(files[1]))
);
}
PVS-Studio warning: V3088 The expression was enclosed by parentheses twice: ((expression)). One pair of parentheses is unnecessary or misprint is present. IsolatedStorageFile.cs 839
To say that code formatting is confusing is another way of saying nothing. Having a short look at this code, I would say that the left operandof the first || operator I came across was files.Length > 2, the right one is the one in brackets. At least the code is formatted like this. After looking a little more carefully, you can understand that it is not the case. In fact, the right operand — ((! IsIdFile (files[0]) && ! IsInfoFile (files[0]))). I think this code is pretty confusing.
Issue 10
PVS-Studio 7.03 introduced the V3138diagnostic rule, which searches for errors in interpolated string. More precisely, in the string that most likely had to be interpolated, but because of the missed $ symbol theyare not. In System.Net libraries I found several interesting occurrences of this diagnostic rule.
internal static void CacheCredential(SafeFreeCredentials newHandle)
{
try
{
....
}
catch (Exception e)
{
if (!ExceptionCheck.IsFatal(e))
{
NetEventSource.Fail(null, "Attempted to throw: {e}");
}
}
}
PVS-Studio warning: V3138 String literal contains potential interpolated expression. Consider inspecting: e. SSPIHandleCache.cs 42
It is highly likely, that the second argument of the Fail method had to be an interpolated string, in which the string representation of the e exception would be substituted. However, due to a missed $ symbol, no string representation was substituted.
Issue 11
Here’s another similar case.
public static async Task GetDigestTokenForCredential(....)
{
....
if (NetEventSource.IsEnabled)
NetEventSource.Error(digestResponse,
"Algorithm not supported: {algorithm}");
....
}
PVS-Studio warning: V3138 String literal contains potential interpolated expression. Consider inspecting: algorithm. AuthenticationHelper.Digest.cs 58
The situation is similar to the one above, again the $ symbol is missed, resulting in the incorrect string, getting into the Error method.
Issue 12
System.Net.Mail package. The method is small, I’ll cite it entirety in order to make searching for the bug more interesting.
internal void SetContent(Stream stream)
{
if (stream == null)
{
throw new ArgumentNullException(nameof(stream));
}
if (_streamSet)
{
_stream.Close();
_stream = null;
_streamSet = false;
}
_stream = stream;
_streamSet = true;
_streamUsedOnce = false;
TransferEncoding = TransferEncoding.Base64;
}
PVS-Studio warning: V3008 The '_streamSet' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 123, 119. MimePart.cs 123
Double value assignment to the variable _streamSet looks strange (first — under the condition, then — outside). Same story with resetting the stream variable. As a result, _stream will still have the value stream, and the _streamSet will be true.
Issue 13
An interesting code fragment from the System.Linq.Expressions library which triggers 2 analyzer warnings at once. In this case, it is more like a feature, than a bug. However, the method is quite unusual…
// throws NRE when o is null
protected static void NullCheck(object o)
{
if (o == null)
{
o.GetType();
}
}
PVS-Studio warnings:
- V3010 The return value of function 'GetType' is required to be utilized. Instruction.cs 36
- V3080 Possible null dereference. Consider inspecting 'o'. Instruction.cs 36
There’s probably nothing to comment on here.
Issue 14
Let’s consider another case, which we’ll handle «from the outside». First, we’ll write the code, detect the problems, and then we’ll look inside. We’ll take the System.Configuration.ConfigurationManager library and the same-name NuGet package for reviewing. I used the 4.5.0 version package. We’ll deal with the System.Configuration.CommaDelimitedStringCollection class.
Let’s do something unsophisticated. For example, we’ll create an object, extract its string representation and get this string’s length, then print it. The relevant code:
CommaDelimitedStringCollection collection
= new CommaDelimitedStringCollection();
Console.WriteLine(collection.ToString().Length);
Just in case, we’ll check out the ToString method description:
Nothing special — string representation of an object is returned. Just in case, I’ll check out docs.microsoft.com — «CommaDelimitedStringCollection.ToString Method». It seems like there is nothing special here.
Okay, let’s execute the code, aaand…
Hmm, surprise. Well, let’s try adding an item to the collection and then get its string representation. Next, we’ll «absolutely accidently» add an empty string:). The code will change and look like this:
CommaDelimitedStringCollection collection
= new CommaDelimitedStringCollection();
collection.Add(String.Empty);
Console.WriteLine(collection.ToString().Length);
Execute and see…
What, again?! Well, let’s finally address the implementation of the ToString method from the CommaDelimitedStringCollection class. The code is below:
public override string ToString()
{
if (Count <= 0) return null;
StringBuilder sb = new StringBuilder();
foreach (string str in this)
{
ThrowIfContainsDelimiter(str);
// ....
sb.Append(str.Trim());
sb.Append(',');
}
if (sb.Length > 0) sb.Length = sb.Length - 1;
return sb.Length == 0 ? null : sb.ToString();
}
PVS-Studio warnings:
- V3108 It is not recommended to return 'null' from 'ToSting ()' method. StringAttributeCollection.cs 57
- V3108 It is not recommended to return 'null' from 'ToSting ()' method. StringAttributeCollection.cs 71
Here we can see 2 fragments, where current ToString implementation can return null. At this point, we’ll recall the recommendation by Microsoft on the ToString method implementation. So let’s consult docs.microsoft.com — «Object.ToString Method»:
Notes to Inheritors…Overrides of the ToString () method should follow these guidelines:
- …
- Your ToString () override should not return Empty or a null string.
- …
This is what PVS-Studio warns about. Two code fragments given above that we were writing to reproduce the problem get different exit points — the first and second null return points respectively. Let’s dig a little deeper.
First case. Count is a property of the base StringCollection class. Since no elements were added, Count == 0, the condition Count <= 0 is true, the null value is returned.
In the second case we added the element, using the instance CommaDelimitedStringCollection.Add method for it.
public new void Add(string value)
{
ThrowIfReadOnly();
ThrowIfContainsDelimiter(value);
_modified = true;
base.Add(value.Trim());
}
Checks are successful in the ThrowIf… method and the element is added in the base collection. Accordingly, the Count value becomes 1. Now let’s get back to the ToString method. Value of the expression Count <= 0 — false, therefore the method doesn’t return and the code execution continues. The internal collection gets traversed, 2 elements are added to the instance of the StringBuilder type — an empty string and a comma. As a result, it turns out that sb contains only a comma, the value of the Length property respectively equals 1. The value of the expression sb.Length > 0 is true, subtraction and writing in sb.Length are performed, now the value of sb.Length is 0. This leads to the fact that the null value is again returned from the method.
Issue 15
All of a sudden, I got a craving for using the class System.Configuration.ConfigurationProperty. Let’s take a constructor with the largest number of parameters:
public ConfigurationProperty(
string name,
Type type,
object defaultValue,
TypeConverter typeConverter,
ConfigurationValidatorBase validator,
ConfigurationPropertyOptions options,
string description);
Let’s see the description of the last parameter:
// description:
// The description of the configuration entity.
The same is written in the constructor description at docs.microsoft.com. Well, let’s take a look how this parameter is used in the constructor’s body:
public ConfigurationProperty(...., string description)
{
ConstructorInit(name, type, options, validator, typeConverter);
SetDefaultValue(defaultValue);
}
Believe it or not, the parameter isn’t used.
PVS-Studio warning: V3117 Constructor parameter 'description' is not used. ConfigurationProperty.cs 62
Probably, code authors intentionally don’t use it, but the description of the relevant parameter is very confusing.
Issue 16
Here is another similar fragment: try to find the error yourself, I’m giving the constructor’s code below.
internal SectionXmlInfo(
string configKey, string definitionConfigPath, string targetConfigPath,
string subPath, string filename, int lineNumber, object streamVersion,
string rawXml, string configSource, string configSourceStreamName,
object configSourceStreamVersion, string protectionProviderName,
OverrideModeSetting overrideMode, bool skipInChildApps)
{
ConfigKey = configKey;
DefinitionConfigPath = definitionConfigPath;
TargetConfigPath = targetConfigPath;
SubPath = subPath;
Filename = filename;
LineNumber = lineNumber;
StreamVersion = streamVersion;
RawXml = rawXml;
ConfigSource = configSource;
ConfigSourceStreamName = configSourceStreamName;
ProtectionProviderName = protectionProviderName;
OverrideModeSetting = overrideMode;
SkipInChildApps = skipInChildApps;
}
PVS-Studio warning: V3117 Constructor parameter 'configSourceStreamVersion' is not used. SectionXmlInfo.cs 16
There is an appropriate property, but frankly, it looks a bit strange:
internal object ConfigSourceStreamVersion
{
set { }
}
Generally, the code looks suspicious. Perhaps the parameter / property is left for compatibility, but that’s just my guess.
Issue 17
Let’s take a look at interesting stuff in the System.Runtime.WindowsRuntime.UI.Xaml library and the same-name package code.
public struct RepeatBehavior : IFormattable
{
....
public override string ToString()
{
return InternalToString(null, null);
}
....
}
PVS-Studio warning: V3108 It is not recommended to return 'null' from 'ToSting ()' method. RepeatBehavior.cs 113
Familiar story that we already know — the ToString method can return the null value. Due to this, author of the caller code, who assumes that RepeatBehavior.ToString always returns a non-null reference, might be unpleasantly surprised at some point. Again, it contradicts Microsoft’s guidelines.
Well, but the method doesn’t make it clear that ToString can return null — we need to go deeper and peep into the InternalToString method.
internal string InternalToString(string format, IFormatProvider formatProvider)
{
switch (_Type)
{
case RepeatBehaviorType.Forever:
return "Forever";
case RepeatBehaviorType.Count:
StringBuilder sb = new StringBuilder();
sb.AppendFormat(
formatProvider,
"{0:" + format + "}x",
_Count);
return sb.ToString();
case RepeatBehaviorType.Duration:
return _Duration.ToString();
default:
return null;
}
}
The analyzer has detected that in case if the default branch executes in switch, InternalToString will return the null value. Therefore, ToString will return null as well.
RepeatBehavior is a public structure, and ToString is a public method, so we can try reproducing the problem in practice. To do it, we’ll create the RepeatBehavior instance, call the ToString method from it and while doing that we shouldn’t miss out that _Type mustn’t be equal to RepeatBehaviorType.Forever, RepeatBehaviorType.Count or RepeatBehaviorType.Duration.
_Type is a private field, which can be assigned via a public property:
public struct RepeatBehavior : IFormattable
{
....
private RepeatBehaviorType _Type;
....
public RepeatBehaviorType Type
{
get { return _Type; }
set { _Type = value; }
}
....
}
So far, so good. Let’s move on and see what is the RepeatBehaviorType type.
public enum RepeatBehaviorType
{
Count,
Duration,
Forever
}
As we can see, RepeatBehaviorType is the enumeration, containing all three elements. Along with this, all these three elements are covered in the switch expression we’re interested in. This, however, doesn’t mean that the default branch is unreachable.
To reproduce the problem we’ll add reference to the System.Runtime.WindowsRuntime.UI.Xaml package to the project (I was using the 4.3.0 version) and execute the following code.
RepeatBehavior behavior = new RepeatBehavior()
{
Type = (RepeatBehaviorType)666
};
Console.WriteLine(behavior.ToString() is null);
True is displayed in the console as expected, which means ToString returned null, as _Type wasn’t equal to any of the values in case branches, and the default branch received control. That’s what we were trying to do.
Also I’d like to note that neither comments to the method, nor docs.microsoft.com specifies that the method can return the null value.
Issue 18
Next, we’ll check into several warnings from System.Private.DataContractSerialization.
private static class CharType
{
public const byte None = 0x00;
public const byte FirstName = 0x01;
public const byte Name = 0x02;
public const byte Whitespace = 0x04;
public const byte Text = 0x08;
public const byte AttributeText = 0x10;
public const byte SpecialWhitespace = 0x20;
public const byte Comment = 0x40;
}
private static byte[] s_charType = new byte[256]
{
....
CharType.None,
/* 9 (.) */
CharType.None|
CharType.Comment|
CharType.Comment|
CharType.Whitespace|
CharType.Text|
CharType.SpecialWhitespace,
/* A (.) */
CharType.None|
CharType.Comment|
CharType.Comment|
CharType.Whitespace|
CharType.Text|
CharType.SpecialWhitespace,
/* B (.) */
CharType.None,
/* C (.) */
CharType.None,
/* D (.) */
CharType.None|
CharType.Comment|
CharType.Comment|
CharType.Whitespace,
/* E (.) */
CharType.None,
....
};
PVS-Studio warnings:
- V3001 There are identical sub-expressions 'CharType.Comment' to the left and to the right of the '|' operator. XmlUTF8TextReader.cs 56
- V3001 There are identical sub-expressions 'CharType.Comment' to the left and to the right of the '|' operator. XmlUTF8TextReader.cs 58
- V3001 There are identical sub-expressions 'CharType.Comment' to the left and to the right of the '|' operator. XmlUTF8TextReader.cs 64
The analyzer found usage of the CharType.Comment|CharType.Comment expression suspicious. Looks a bit strange, as (CharType.Comment | CharType.Comment) == CharType.Comment. When initializing other array elements, which use CharType.Comment, there is no such duplication.
Issue 19
Let’s continue. Let’s check out the information on the XmlBinaryWriterSession.TryAdd method’s return value in the method description and at docs.microsoft.com — «XmlBinaryWriterSession.TryAdd (XmlDictionaryString, Int32) Method»: Returns: true if the string could be added; otherwise, false.
Now let’s look into the body of the method:
public virtual bool TryAdd(XmlDictionaryString value, out int key)
{
IntArray keys;
if (value == null)
throw System.Runtime
.Serialization
.DiagnosticUtility
.ExceptionUtility
.ThrowHelperArgumentNull(nameof(value));
if (_maps.TryGetValue(value.Dictionary, out keys))
{
key = (keys[value.Key] - 1);
if (key != -1)
{
// If the key is already set, then something is wrong
throw System.Runtime
.Serialization
.DiagnosticUtility
.ExceptionUtility
.ThrowHelperError(
new InvalidOperationException(
SR.XmlKeyAlreadyExists));
}
key = Add(value.Value);
keys[value.Key] = (key + 1);
return true;
}
key = Add(value.Value);
keys = AddKeys(value.Dictionary, value.Key + 1);
keys[value.Key] = (key + 1);
return true;
}
PVS-Studio warning: V3009 It’s odd that this method always returns one and the same value of 'true'. XmlBinaryWriterSession.cs 29
It seems strange that the method either returns true or throws an exception, but the false value is never returned.
Issue 20
I came across the code with a similar problem, but in this case, on the contrary — the method always returns false:
internal virtual bool OnHandleReference(....)
{
if (xmlWriter.depth < depthToCheckCyclicReference)
return false;
if (canContainCyclicReference)
{
if (_byValObjectsInScope.Contains(obj))
throw ....;
_byValObjectsInScope.Push(obj);
}
return false;
}
PVS-Studio warning: V3009 It’s odd that this method always returns one and the same value of 'false'. XmlObjectSerializerWriteContext.cs 415
Well, we’ve already come a long way! So before moving on I suggest that you have a small break: stir up your muscles, walk around, give rest to your eyes, look out of the window…
I hope at this point you’re full of energy again, so let’s continue. :)
Issue 21
Let’s review some engaging fragments of the System.Security.Cryptography.Algorithms project.
public override byte[] GenerateMask(byte[] rgbSeed, int cbReturn)
{
using (HashAlgorithm hasher
= (HashAlgorithm)CryptoConfig.CreateFromName(_hashNameValue))
{
byte[] rgbCounter = new byte[4];
byte[] rgbT = new byte[cbReturn];
uint counter = 0;
for (int ib = 0; ib < rgbT.Length;)
{
// Increment counter -- up to 2^32 * sizeof(Hash)
Helpers.ConvertIntToByteArray(counter++, rgbCounter);
hasher.TransformBlock(rgbSeed, 0, rgbSeed.Length, rgbSeed, 0);
hasher.TransformFinalBlock(rgbCounter, 0, 4);
byte[] hash = hasher.Hash;
hasher.Initialize();
Buffer.BlockCopy(hash, 0, rgbT, ib,
Math.Min(rgbT.Length - ib, hash.Length));
ib += hasher.Hash.Length;
}
return rgbT;
}
}
PVS-Studio warning: V3080 Possible null dereference. Consider inspecting 'hasher'. PKCS1MaskGenerationMethod.cs 37
The analyzer warns that the hasher variable’s value can be null when evaluating the hasher.TransformBlock expression resulting in an exception of the NullReferenceException type. This warning’s occurrence became possible due to interprocedural analysis.
So to find out if hasher can take the null value in this case, we need to dip into the CreateFromName method.
public static object CreateFromName(string name)
{
return CreateFromName(name, null);
}
Nothing so far — let’s go deeper. The body of the overloaded CreateFromName version with two parameters is quite large, so I cite the short version.
public static object CreateFromName(string name, params object[] args)
{
....
if (retvalType == null)
{
return null;
}
....
if (cons == null)
{
return null;
}
....
if (candidates.Count == 0)
{
return null;
}
....
if (rci == null || typeof(Delegate).IsAssignableFrom(rci.DeclaringType))
{
return null;
}
....
return retval;
}
As you can see, there are several exit points in the method where the null value is explicitly returned. Therefore, at least theoretically, in the method above, that triggered a warning, an exception of the NullReferenceException type might occur.
Theory is great, but let’s try to reproduce the problem in practice. To do this, we’ll take another look at the original method and note the key points. Also, we’ll reduce the irrelevant code from the method.
public class PKCS1MaskGenerationMethod : .... // <= 1
{
....
public PKCS1MaskGenerationMethod() // <= 2
{
_hashNameValue = DefaultHash;
}
....
public override byte[] GenerateMask(byte[] rgbSeed, int cbReturn) // <= 3
{
using (HashAlgorithm hasher
= (HashAlgorithm)CryptoConfig.CreateFromName(_hashNameValue)) // <= 4
{
byte[] rgbCounter = new byte[4];
byte[] rgbT = new byte[cbReturn]; // <= 5
uint counter = 0;
for (int ib = 0; ib < rgbT.Length;) // <= 6
{
....
Helpers.ConvertIntToByteArray(counter++, rgbCounter); // <= 7
hasher.TransformBlock(rgbSeed, 0, rgbSeed.Length, rgbSeed, 0);
....
}
....
}
}
}
Let’s take a closer look at the key points:
1, 3. The class and method have public access modifiers. Hence, this interface is available when adding reference to a library — we can try reproducing this issue.
2. The class is non-abstract instance, has a public constructor. It must be easy to create an instance, which we’ll work with. In some cases, that I considered, classes were abstract, so to reproduce the issue I had to search for inheritors and ways to obtain them.
4. CreateFromName mustn’t generate any exceptions and must return null — the most important point, we’ll get back to it later.
5, 6. The cbReturn value has to be > 0 (but, of course, within adequate limits for the successful creation of an array). Compliance of the cbReturn > 0 condition is needed to meet the further condition ib < rgbT.Length and enter the loop body.
7. Helpres.ConvertIntToByteArray must work without exceptions.
To meet the conditions that depend on the method parameters, it is enough to simply pass appropriate arguments, for example:
- rgbCeed — new byte[] { 0, 1, 2, 3 };
- cbReturn — 42.
In order to «discredit» the CryptoConfig.CreateFromName method, we need to be able to change the value of the _hashNameValue field. Fortunately, we have it, as the class defines a wrapper property for this field:
public string HashName
{
get { return _hashNameValue; }
set { _hashNameValue = value ?? DefaultHash; }
}
By setting a 'synthetic' value for HashName (that is _hashNameValue), we can get the null value from the CreateFromName method at the first exit point from the ones we marked. I won’t go into the details of analyzing this method (hope you’ll forgive me for this), as the method is quite large.
As a result, the code which will lead to an exception of the NullReferenceException type, might look as follows:
PKCS1MaskGenerationMethod tempObj = new PKCS1MaskGenerationMethod();
tempObj.HashName = "Dummy";
tempObj.GenerateMask(new byte[] { 1, 2, 3 }, 42);
Now we add reference to the debugging library, run the code and get the expected result:
Just for the fun of it, I tried to execute the same code using the NuGet package of the 4.3.1 version.
There’s no information on generated exceptions, limitations of output parameters in the method description. Docs.microsoft.com PKCS1MaskGenerationMethod.GenerateMask (Byte[], Int32) Method» doesn’t specify it either.
By the way, right when writing the article and describing the order of actions to reproduce the problem, I found 2 more ways to «break» this method:
- pass a too large value as a cbReturn argument;
- pass the null value as rgbSeed.
In the first case, we’ll get an exception of the OutOfMemoryException type.
In the second case, we’ll get an exception of the NullReferenceException type when executing the rgbSeed.Length expression. In this case, it’s important, that hasher has a non-null value. Otherwise, the control flow won’t get to rgbSeed.Length.
Issue 22
I came across a couple of similar places.
public class SignatureDescription
{
....
public string FormatterAlgorithm { get; set; }
public string DeformatterAlgorithm { get; set; }
public SignatureDescription()
{
}
....
public virtual AsymmetricSignatureDeformatter CreateDeformatter(
AsymmetricAlgorithm key)
{
AsymmetricSignatureDeformatter item = (AsymmetricSignatureDeformatter)
CryptoConfig.CreateFromName(DeformatterAlgorithm);
item.SetKey(key); // <=
return item;
}
public virtual AsymmetricSignatureFormatter CreateFormatter(
AsymmetricAlgorithm key)
{
AsymmetricSignatureFormatter item = (AsymmetricSignatureFormatter)
CryptoConfig.CreateFromName(FormatterAlgorithm);
item.SetKey(key); // <=
return item;
}
....
}
PVS-Studio warnings:
- V3080 Possible null dereference. Consider inspecting 'item'. SignatureDescription.cs 31
- V3080 Possible null dereference. Consider inspecting 'item'. SignatureDescription.cs 38
Again, in FormatterAlgorithm and DeformatterAlgorithm properties we can write such values, for which the CryptoConfig.CreateFromName method return the null value in the CreateDeformatter and CreateFormatter methods. Further, when calling the SetKey instance method, a NullReferenceException exception will be generated. The problem, again, is easily reproduced in practice:
SignatureDescription signature = new SignatureDescription()
{
DeformatterAlgorithm = "Dummy",
FormatterAlgorithm = "Dummy"
};
signature.CreateDeformatter(null); // NRE
signature.CreateFormatter(null); // NRE
In this case, when calling CreateDeformatter as well as calling CreateFormatter, an exception of the NullReferenceException type is thrown.
Issue 23
Let’s review interesting fragments from the System.Private.Xml project.
public override void WriteBase64(byte[] buffer, int index, int count)
{
if (!_inAttr && (_inCDataSection || StartCDataSection()))
_wrapped.WriteBase64(buffer, index, count);
else
_wrapped.WriteBase64(buffer, index, count);
}
PVS-Studio warning: V3004 The 'then' statement is equivalent to the 'else' statement. QueryOutputWriterV1.cs 242
It looks strange that then and else branches of the if statement contain the same code. Either there’s an error here and another action has to be made in one of the branches, or the if statement can be omitted.
Issue 24
internal void Depends(XmlSchemaObject item, ArrayList refs)
{
....
if (content is XmlSchemaSimpleTypeRestriction)
{
baseType = ((XmlSchemaSimpleTypeRestriction)content).BaseType;
baseName = ((XmlSchemaSimpleTypeRestriction)content).BaseTypeName;
}
else if (content is XmlSchemaSimpleTypeList)
{
....
}
else if (content is XmlSchemaSimpleTypeRestriction)
{
baseName = ((XmlSchemaSimpleTypeRestriction)content).BaseTypeName;
}
else if (t == typeof(XmlSchemaSimpleTypeUnion))
{
....
}
....
}
PVS-Studio warning: V3003 The use of 'if (A) {…} else if (A) {…}' pattern was detected. There is a probability of logical error presence. Check lines: 381, 396. ImportContext.cs 381
In the if-else-if sequence there are two equal conditional expressions — content is XmlSchemaSimpleTypeRestriction. What is more, bodies of then branches of respective statements contain a different set of expressions. Anyway, either the body of the first relevant then branch will be executed (if the conditional expression is true), or none of them in case if the relevant expression is false.
Issue 25
To make it more intriguing to search for the error in the next method, I’ll cite is entire body.
public bool MatchesXmlType(IList seq, int indexType)
{
XmlQueryType typBase = GetXmlType(indexType);
XmlQueryCardinality card;
switch (seq.Count)
{
case 0: card = XmlQueryCardinality.Zero; break;
case 1: card = XmlQueryCardinality.One; break;
default: card = XmlQueryCardinality.More; break;
}
if (!(card <= typBase.Cardinality))
return false;
typBase = typBase.Prime;
for (int i = 0; i < seq.Count; i++)
{
if (!CreateXmlType(seq[0]).IsSubtypeOf(typBase))
return false;
}
return true;
}
If you’ve coped — congratulations!
If not — PVS-Studio to the rescue: V3102 Suspicious access to element of 'seq' object by a constant index inside a loop. XmlQueryRuntime.cs 738
The for loop is executed, the expression i < seq.Count is used as an exit condition. It suggests the idea that developers want to bypass the seq sequence. But in the loop, authors access sequence elements not by using the counter — seq[i], but a number literal — zero (seq[0]).
Issue 26
The next error fits in a small piece of code, but it’s no less interesting.
public override void WriteValue(string value)
{
WriteValue(value);
}
PVS-Studio warning: V3110 Possible infinite recursion inside 'WriteValue' method. XmlAttributeCache.cs 166
The method calls itself, forming recursion without an exit condition.
Issue 27
public IList DocOrderDistinct(IList seq)
{
if (seq.Count <= 1)
return seq;
XmlQueryNodeSequence nodeSeq = (XmlQueryNodeSequence)seq;
if (nodeSeq == null)
nodeSeq = new XmlQueryNodeSequence(seq);
return nodeSeq.DocOrderDistinct(_docOrderCmp);
}
PVS-Studio warning: V3095 The 'seq' object was used before it was verified against null. Check lines: 880, 884. XmlQueryRuntime.cs 880
The method can get the null value as an argument. Due to this, when accessing the Count property, an exception of the NullReferenceException type will be generated. Below the variable nodeSeq is checked. nodeSeq is obtained as a result of explicit seq casting, still it’s not clear why the check takes place. If the seq value is null, the control flow won’t get to this check because of the exception. If the seq value isn’t null, then:
- if casting fails, an exception of the InvalidCastException type will be generated;
- if casting is successful, nodeSeq definitely isn’t null.
Issue 28
I came across 4 constructors, containing unused parameters. Perhaps, they are left for compatibility, but I found no additional comments on these unused parameters.
PVS-Studio warnings:
- V3117 Constructor parameter 'securityUrl' is not used. XmlSecureResolver.cs 15
- V3117 Constructor parameter 'strdata' is not used. XmlEntity.cs 18
- V3117 Constructor parameter 'location' is not used. Compilation.cs 58
- V3117 Constructor parameter 'access' is not used. XmlSerializationILGen.cs 38
The first one interested me the most (at least, it got into the list of warnings for the article). What’s so special? Not sure. Perhaps, its name.
public XmlSecureResolver(XmlResolver resolver, string securityUrl)
{
_resolver = resolver;
}
Just for the sake of interest, I checked out what’s written at docs.microsoft.com — «XmlSecureResolver Constructors» about the securityUrl parameter:
The URL used to create the PermissionSet that will be applied to the underlying XmlResolver. The XmlSecureResolver calls PermitOnly () on the created PermissionSet before calling GetEntity (Uri, String, Type) on the underlying XmlResolver.
Issue 29
In the System.Private.Uri package I found the method, which wasn’t following exactly Microsoft guidelines on the ToString method overriding. Here we need to recall one of the tips from the page «Object.ToString Method»: Your ToString () override should not throw an exception.
The overridden method itself looks like this:
public override string ToString()
{
if (_username.Length == 0 && _password.Length > 0)
{
throw new UriFormatException(SR.net_uri_BadUserPassword);
}
....
}
PVS-Studio warning: V3108 It is not recommended to throw exceptions from 'ToSting ()' method. UriBuilder.cs 406
The code first sets an empty string for the _username field and a nonempty one for the _password field respectively through the public properties UserName and Password. After that it calls the ToString method. Eventually this code will get an exception. An example of such code:
UriBuilder uriBuilder = new UriBuilder()
{
UserName = String.Empty,
Password = "Dummy"
};
String stringRepresentation = uriBuilder.ToString();
Console.WriteLine(stringRepresentation);
But in this case developers honestly warn that calling might result in an exception. It is described in comments to the method and at docs.microsoft.com — «UriBuilder.ToString Method».
Issue 30
Look at the warnings, issued on the System.Data.Common project code.
private ArrayList _tables;
private DataTable GetTable(string tableName, string ns)
{
....
if (_tables.Count == 0)
return (DataTable)_tables[0];
....
}
PVS-Studio warning: V3106 Possibly index is out of bound. The '0' index is pointing beyond '_tables' bound. XMLDiffLoader.cs 277
Does this piece of code look unusual? What do you think it is? An unusual way to generate an exception of the ArgumentOutOfRangeException type? I wouldn’t be surprised by this approach. Overall, it’s very strange and suspicious code.
Issue 31
internal XmlNodeOrder ComparePosition(XPathNodePointer other)
{
RealFoliate();
other.RealFoliate();
Debug.Assert(other != null);
....
}
PVS-Studio warning: V3095 The 'other' object was used before it was verified against null. Check lines: 1095, 1096. XPathNodePointer.cs 1095
The expression other!= null as an argument of the Debug.Assert method suggests, that the ComparePosition method can obtain the null value as an argument. At least, the intention was to catch such cases. But at the same time, the line above the other.RealFoliate instance method is called. As a result, if other has the null value, an exception of the NullReferenceException type will be generated before checking through Assert.
Issue 32
private PropertyDescriptorCollection GetProperties(Attribute[] attributes)
{
....
foreach (Attribute attribute in attributes)
{
Attribute attr = property.Attributes[attribute.GetType()];
if ( (attr == null && !attribute.IsDefaultAttribute())
|| !attr.Match(attribute))
{
match = false;
break;
}
}
....
}
PVS-Studio warning: V3080 Possible null dereference. Consider inspect
