.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 – will we 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 Pandora 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<KeyValuePair<string, object>>
QueryDynamicObject(object obj)
{
....
List<string> names = new List<string>(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<T1, T2>
{
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
<System.String,System.Resources.ResourceSet>, 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<DiagnosticInfo> 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<string> 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); // NREIn 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<XPathItem> 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<XPathNavigator> DocOrderDistinct(IList<XPathNavigator> 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 inspecting 'attr'. DbConnectionStringBuilder.cs 534
Conditional expression of the if statement looks quite suspicious. Match is an instance method. According to the check attr == null, null is the acceptable (expected) value for this variable. Therefore, if control flow gets to the right operand of the || operator (if attr — null), we'll get an exception of the NullReferenceException type.
Accordingly, conditions of the exception occurrence are the following:
- The value of attr — null. The right operand of the && operator is evaluated.
- The value of !attribute.IsDefaultAttribute() — false. The overall result of the expression with the && operator — false.
- Since the left operand of the || operator is of the false value, the right operand is evaluated.
- Since attr — null, when calling the Match method, an exception is generated.
Issue 33
private int ReadOldRowData(
DataSet ds, ref DataTable table, ref int pos, XmlReader row)
{
....
if (table == null)
{
row.Skip(); // need to skip this element if we dont know about it,
// before returning -1
return -1;
}
....
if (table == null)
throw ExceptionBuilder.DiffgramMissingTable(
XmlConvert.DecodeName(row.LocalName));
....
}PVS-Studio warning: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless XMLDiffLoader.cs 301
There are two if statements, containing the equal expression — table == null. With that, then branches of these statements contain different actions — in the first case, the method exits with the value -1, in the second one — an exception is generated. The table variable isn't changed between the checks. Thus, the considered exception won't be generated.
Issue 34
Look at the interesting method from the System.ComponentModel.TypeConverter project. Well, let's first read the comment, describing it:
Removes the last character from the formatted string. (Remove last character in virtual string). On exit the out param contains the position where the operation was actually performed. This position is relative to the test string. The MaskedTextResultHint out param gives more information about the operation result. Returns true on success, false otherwise.
The key point on the return value: if an operation is successful, the method returns true, otherwise — false. Let's see what happens in fact.
public bool Remove(out int testPosition, out MaskedTextResultHint resultHint)
{
....
if (lastAssignedPos == INVALID_INDEX)
{
....
return true; // nothing to remove.
}
....
return true;
}PVS-Studio warning: V3009 It's odd that this method always returns one and the same value of 'true'. MaskedTextProvider.cs 1529
In fact, it turns out that the only return value of the method is true.
Issue 35
public void Clear()
{
if (_table != null)
{
....
}
if (_table.fInitInProgress && _delayLoadingConstraints != null)
{
....
}
....
}PVS-Studio warning: V3125 The '_table' object was used after it was verified against null. Check lines: 437, 423. ConstraintCollection.cs 437
The _table != null check speaks for itself — the _table variable can have the null value. At least, in this case code authors get reinsured. However, below they address the instance field via _table but without the check for null — _table .fInitInProgress.
Issue 36
Now let's consider several warnings, issued for the code of the System.Runtime.Serialization.Formatters project.
private void Write(....)
{
....
if (memberNameInfo != null)
{
....
_serWriter.WriteObjectEnd(memberNameInfo, typeNameInfo);
}
else if ((objectInfo._objectId == _topId) && (_topName != null))
{
_serWriter.WriteObjectEnd(topNameInfo, typeNameInfo);
....
}
else if (!ReferenceEquals(objectInfo._objectType, Converter.s_typeofString))
{
_serWriter.WriteObjectEnd(typeNameInfo, typeNameInfo);
}
}PVS-Studio warning: V3038 The argument was passed to method several times. It is possible that other argument should be passed instead. BinaryObjectWriter.cs 262
The analyzer was confused by the last call _serWriter.WriteObjectEnd with two equal arguments — typeNameInfo. It looks like a typo, but I can't say for sure. I decided to check out what is the callee WriteObjectEnd method.
internal void WriteObjectEnd(NameInfo memberNameInfo, NameInfo typeNameInfo)
{ }Well… Let's move on. :)
Issue 37
internal void WriteSerializationHeader(
int topId,
int headerId,
int minorVersion,
int majorVersion)
{
var record = new SerializationHeaderRecord(
BinaryHeaderEnum.SerializedStreamHeader,
topId,
headerId,
minorVersion,
majorVersion);
record.Write(this);
}When reviewing this code, I wouldn't say at once what's wrong here or what looks suspicious. But the analyzer may well say what's the thing.
PVS-Studio warning: V3066 Possible incorrect order of arguments passed to 'SerializationHeaderRecord' constructor: 'minorVersion' and 'majorVersion'. BinaryFormatterWriter.cs 111
See the callee constructor of the SerializationHeaderRecord class.
internal SerializationHeaderRecord(
BinaryHeaderEnum binaryHeaderEnum,
int topId,
int headerId,
int majorVersion,
int minorVersion)
{
_binaryHeaderEnum = binaryHeaderEnum;
_topId = topId;
_headerId = headerId;
_majorVersion = majorVersion;
_minorVersion = minorVersion;
}As we can see, constructor's parameters follow in the order majorVersion, minorVersion; whereas when calling the constructor they are passed in this order: minorVersion, majorVersion. Seems like a typo. In case it was made deliberately (what if?) — I think it would require an additional comment.
Issue 38
internal ObjectManager(
ISurrogateSelector selector,
StreamingContext context,
bool checkSecurity,
bool isCrossAppDomain)
{
_objects = new ObjectHolder[DefaultInitialSize];
_selector = selector;
_context = context;
_isCrossAppDomain = isCrossAppDomain;
}PVS-Studio warning: V3117 Constructor parameter 'checkSecurity' is not used. ObjectManager.cs 33
The checkSecurity parameter of the constructor isn't used in any way. There are no comments on it. I guess it's left for compatibility, but anyway, in the context of recent security conversations, it looks interesting.
Issue 39
Here's the code that seemed unusual to me. The pattern looks one and the same in all three detected cases and is located in methods with equal names and variables names. Consequently:
- either I'm not enlightened enough to get the purpose of such duplication;
- or the error was spread by the copy-paste method.
The code itself:
private void EnlargeArray()
{
int newLength = _values.Length * 2;
if (newLength < 0)
{
if (newLength == int.MaxValue)
{
throw new SerializationException(SR.Serialization_TooManyElements);
}
newLength = int.MaxValue;
}
FixupHolder[] temp = new FixupHolder[newLength];
Array.Copy(_values, 0, temp, 0, _count);
_values = temp;
}PVS-Studio warnings:
- V3022 Expression 'newLength == int.MaxValue' is always false. ObjectManager.cs 1423
- V3022 Expression 'newLength == int.MaxValue' is always false. ObjectManager.cs 1511
- V3022 Expression 'newLength == int.MaxValue' is always false. ObjectManager.cs 1558
What is different in other methods is the type of the temp array elements (not FixupHolder, but long or object). So I still have suspicions of copy-paste…
Issue 40
Code from the System.Data.Odbc project.
public string UnquoteIdentifier(....)
{
....
if (!string.IsNullOrEmpty(quotePrefix) || quotePrefix != " ")
{ .... }
....
}PVS-Studio warning: V3022 Expression '!string.IsNullOrEmpty(quotePrefix) || quotePrefix != " "' is always true. OdbcCommandBuilder.cs 338
The analyzer assumes that the given expression always has the true value. It is really so. It even doesn't matter what value is actually in quotePrefix — the condition itself is written incorrectly. Let's get to the bottom of this.
We have the || operator, so the expression value will be true, if the left or right (or both) operand will have the true value. It's all clear with the left one. The right one will be evaluated only in case if the left one has the false value. This means, if the expression is composed in the way that the value of the right operand is always true when the value of the left one is false, the result of the entire expression will permanently be true.
From the code above we know that if the right operand is evaluated, the value of the expression string.IsNullOrEmpty(quotePrefix) — true, so one of these statements is true:
- quotePrefix == null;
- quotePrefix.Length == 0.
If one of these statements is true, the expression quotePrefix != " " will also be true, which we wanted to prove. Meaning that the value of the entire expression is always true, regardless of the quotePrefix contents.
Issue 41
Going back to constructors with unused parameters:
private sealed class PendingGetConnection
{
public PendingGetConnection(
long dueTime,
DbConnection owner,
TaskCompletionSource<DbConnectionInternal> completion,
DbConnectionOptions userOptions)
{
DueTime = dueTime;
Owner = owner;
Completion = completion;
}
public long DueTime { get; private set; }
public DbConnection Owner { get; private set; }
public TaskCompletionSource<DbConnectionInternal>
Completion { get; private set; }
public DbConnectionOptions UserOptions { get; private set; }
}PVS-Studio warning: V3117 Constructor parameter 'userOptions' is not used. DbConnectionPool.cs 26
We can see from the analyzer warnings and the code, that only one constructor's parameter isn't used — userOptions, and others are used for initializing same-name properties. It looks like a developer forgot to initialize one of the properties.
Issue 42
There's suspicious code, that we've come across 2 times. The pattern is the same.
private DataTable ExecuteCommand(....)
{
....
foreach (DataRow row in schemaTable.Rows)
{
resultTable.Columns
.Add(row["ColumnName"] as string,
(Type)row["DataType"] as Type);
}
....
}PVS-Studio warnings:
- V3051 An excessive type cast. The object is already of the 'Type' type. DbMetaDataFactory.cs 176
- V3051 An excessive type cast. The object is already of the 'Type' type. OdbcMetaDataFactory.cs 1109
The expression (Type)row[«DataType»] as Type looks suspicious. First, explicit casting will be performed, after that — casting via the as operator. If the value row[«DataType»] — null, it will successfully 'pass' through both castings and will do as an argument to the Add method. If row[«DataType»] returns the value, which cannot be casted to the Type type, an exception of the InvalidCastException type will be generated right during the explicit cast. In the end, why do we need two castings here? The question is open.
Issue 43
Let's look at the suspicious fragment from System.Runtime.InteropServices.RuntimeInformation.
public static string FrameworkDescription
{
get
{
if (s_frameworkDescription == null)
{
string versionString = (string)AppContext.GetData("FX_PRODUCT_VERSION");
if (versionString == null)
{
....
versionString
= typeof(object).Assembly
.GetCustomAttribute<
AssemblyInformationalVersionAttribute>()
?.InformationalVersion;
....
int plusIndex = versionString.IndexOf('+');
....
}
....
}
....
}
}PVS-Studio warning: V3105 The 'versionString' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. RuntimeInformation.cs 29
The analyzer warns about a possible exception of the NullReferenceException type when calling the IndexOf method for the versionString variable. When receiving the value for a variable, code authors use the '?.' operator to avoid a NullReferenceException exception when accessing the InfromationalVersion property. The trick is that if the call of GetCustomAttribute<...> returns null, an exception will still be generated, but below — when calling the IndexOf method, as versionString will have the null value.
Issue 44
Let's address the System.ComponentModel.Composition project and look through several warnings. Two warnings were issued for the following code:
public static bool CanSpecialize(....)
{
....
object[] genericParameterConstraints = ....;
GenericParameterAttributes[] genericParameterAttributes = ....;
// if no constraints and attributes been specifed, anything can be created
if ((genericParameterConstraints == null) &&
(genericParameterAttributes == null))
{
return true;
}
if ((genericParameterConstraints != null) &&
(genericParameterConstraints.Length != partArity))
{
return false;
}
if ((genericParameterAttributes != null) &&
(genericParameterAttributes.Length != partArity))
{
return false;
}
for (int i = 0; i < partArity; i++)
{
if (!GenericServices.CanSpecialize(
specialization[i],
(genericParameterConstraints[i] as Type[]).
CreateTypeSpecializations(specialization),
genericParameterAttributes[i]))
{
return false;
}
}
return true;
}PVS-Studio warnings:
- V3125 The 'genericParameterConstraints' object was used after it was verified against null. Check lines: 603, 589. GenericSpecializationPartCreationInfo.cs 603
- V3125 The 'genericParameterAttributes' object was used after it was verified against null. Check lines: 604, 594. GenericSpecializationPartCreationInfo.cs 604
In code there are checks genericParameterAttributes != null and genericParameterConstraints != null. Therefore, null — acceptable values for these variables, we'll take it into account. If both variables have the null value, we'll exit the method, no questions. What if one of two variables mentioned above is null, but in doing so we don't exit the method? If such case is possible and execution gets to traversing the loop, we'll get an exception of the NullReferenceException type.
Issue 45
Next we'll move to another interesting warning from this project. And though, let's do something different — first we'll use the class again, and then look at the code. Next, we'll add reference to the same-name NuGet package of the last available prerelease version in the project (I installed the package of the version 4.6.0-preview6.19303.8). Let's write simple code, for example, such as:
LazyMemberInfo lazyMemberInfo = new LazyMemberInfo();
var eq = lazyMemberInfo.Equals(null);
Console.WriteLine(eq);The Equals method isn't commented, I didn't find this method description for .NET Core at docs.microsoft.com, only for .NET Framework. If we look at it ("LazyMemberInfo.Equals(Object) Method") — we won't see anything special whether it returns true or false, there is no information on generated exceptions. We'll execute the code and see:
We can get a little twisted and write the following code and also get interesting output:
LazyMemberInfo lazyMemberInfo = new LazyMemberInfo();
var eq = lazyMemberInfo.Equals(typeof(String));
Console.WriteLine(eq);The result of the code execution.
Interestingly, these both exceptions are generated in the same expression. Let's look insidethe Equals method.
public override bool Equals(object obj)
{
LazyMemberInfo that = (LazyMemberInfo)obj;
// Difefrent member types mean different members
if (_memberType != that._memberType)
{
return false;
}
// if any of the lazy memebers create accessors in a delay-loaded fashion,
// we simply compare the creators
if ((_accessorsCreator != null) || (that._accessorsCreator != null))
{
return object.Equals(_accessorsCreator, that._accessorsCreator);
}
// we are dealing with explicitly passed accessors in both cases
if(_accessors == null || that._accessors == null)
{
throw new Exception(SR.Diagnostic_InternalExceptionMessage);
}
return _accessors.SequenceEqual(that._accessors);
}PVS-Studio warning: V3115 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. LazyMemberInfo.cs 116
Actually in this case the analyzer screwed up a bit, as it issued a warning for the that._memberType expression. However, exceptions occur earlier when executing the expression (LazyMemberInfo)obj. We've already made a note of it.
I think it's all clear with InvalidCastException. Why is NullReferenceException generated? The fact is that LazyMemberInfo is a struct, therefore, it gets unboxed. The null value unboxing, in turns, leads to occurrence of an exception of the NullReferenceException type. Also there is a couple of typos in comments — authors should probably fix them. An explicit exception throwing is still on the authors hands.
Issue 46
By the way, I came across a similar case in System.Drawing.Common in the TriState structure.
public override bool Equals(object o)
{
TriState state = (TriState)o;
return _value == state._value;
}PVS-Studio warning: V3115 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. TriState.cs 53
The problems are the same as in the case described above.
Issue 47
Let's consider several fragments from System.Text.Json.
Remember I wrote that ToString mustn't return null? Time to solidify this knowledge.
public override string ToString()
{
switch (TokenType)
{
case JsonTokenType.None:
case JsonTokenType.Null:
return string.Empty;
case JsonTokenType.True:
return bool.TrueString;
case JsonTokenType.False:
return bool.FalseString;
case JsonTokenType.Number:
case JsonTokenType.StartArray:
case JsonTokenType.StartObject:
{
// null parent should have hit the None case
Debug.Assert(_parent != null);
return _parent.GetRawValueAsString(_idx);
}
case JsonTokenType.String:
return GetString();
case JsonTokenType.Comment:
case JsonTokenType.EndArray:
case JsonTokenType.EndObject:
default:
Debug.Fail($"No handler for {nameof(JsonTokenType)}.{TokenType}");
return string.Empty;
}
}At first sight, this method doesn't return null, but the analyzer argues the converse.
PVS-Studio warning: V3108 It is not recommended to return 'null' from 'ToSting()' method. JsonElement.cs 1460
The analyzer points to the line with calling the GetString() method. Let's have a look at it.
public string GetString()
{
CheckValidInstance();
return _parent.GetString(_idx, JsonTokenType.String);
}Let's go deeper in the overloaded version of the GetString method:
internal string GetString(int index, JsonTokenType expectedType)
{
....
if (tokenType == JsonTokenType.Null)
{
return null;
}
....
}Right after we see the condition, whose execution will result in the null value — both from this method and ToString which we initially considered.
Issue 48
Another interesting fragment:
internal JsonPropertyInfo CreatePolymorphicProperty(....)
{
JsonPropertyInfo runtimeProperty
= CreateProperty(property.DeclaredPropertyType,
runtimePropertyType,
property.ImplementedPropertyType,
property?.PropertyInfo,
Type,
options);
property.CopyRuntimeSettingsTo(runtimeProperty);
return runtimeProperty;
}PVS-Studio warning: V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'property' object JsonClassInfo.AddProperty.cs 179
When calling the CreateProperty method, properties are referred several times through the variable property: property.DeclaredPropertyType, property.ImplementedPropertyType, property?.PropertyInfo. As you can see, in one case code authors use the '?.' operator. If it's not out of place here and property can have the null value, this operator won't be of any help, as an exception of the NullReferenceException type will be generated with direct access.
Issue 49
The following suspicious fragments were found in the System.Security.Cryptography.Xml project. They are paired up, the same as it has been several times with other warnings. Again, the code looks like copy-paste, compare these yourself.
The first fragment:
public void Write(StringBuilder strBuilder,
DocPosition docPos,
AncestralNamespaceContextManager anc)
{
docPos = DocPosition.BeforeRootElement;
foreach (XmlNode childNode in ChildNodes)
{
if (childNode.NodeType == XmlNodeType.Element)
{
CanonicalizationDispatcher.Write(
childNode, strBuilder, DocPosition.InRootElement, anc);
docPos = DocPosition.AfterRootElement;
}
else
{
CanonicalizationDispatcher.Write(childNode, strBuilder, docPos, anc);
}
}
}The second fragment.
public void WriteHash(HashAlgorithm hash,
DocPosition docPos,
AncestralNamespaceContextManager anc)
{
docPos = DocPosition.BeforeRootElement;
foreach (XmlNode childNode in ChildNodes)
{
if (childNode.NodeType == XmlNodeType.Element)
{
CanonicalizationDispatcher.WriteHash(
childNode, hash, DocPosition.InRootElement, anc);
docPos = DocPosition.AfterRootElement;
}
else
{
CanonicalizationDispatcher.WriteHash(childNode, hash, docPos, anc);
}
}
}PVS-Studio warnings:
- V3061 Parameter 'docPos' is always rewritten in method body before being used. CanonicalXmlDocument.cs 37
- V3061 Parameter 'docPos' is always rewritten in method body before being used. CanonicalXmlDocument.cs 54
In both methods the docPos parameter is overwritten before its value is used. Therefore, the value, used as a method argument, is simply ignored.
Issue 50
Let's consider several warnings on the code of the System.Data.SqlClient project.
private bool IsBOMNeeded(MetaType type, object value)
{
if (type.NullableType == TdsEnums.SQLXMLTYPE)
{
Type currentType = value.GetType();
if (currentType == typeof(SqlString))
{
if (!((SqlString)value).IsNull && ((((SqlString)value).Value).Length > 0))
{
if ((((SqlString)value).Value[0] & 0xff) != 0xff)
return true;
}
}
else if ((currentType == typeof(string)) && (((String)value).Length > 0))
{
if ((value != null) && (((string)value)[0] & 0xff) != 0xff)
return true;
}
else if (currentType == typeof(SqlXml))
{
if (!((SqlXml)value).IsNull)
return true;
}
else if (currentType == typeof(XmlDataFeed))
{
return true; // Values will eventually converted to unicode string here
}
}
return false;
}PVS-Studio warning: V3095 The 'value' object was used before it was verified against null. Check lines: 8696, 8708. TdsParser.cs 8696
The analyzer was confused by the check value != null in one of the conditions. It seems like it was lost there during refactoring, as value gets dereferenced many times. If value can have the null value — things are bad.
Issue 51
The next error is from tests, but it seemed interesting to me, so I decided to cite it.
protected virtual TDSMessageCollection CreateQueryResponse(....)
{
....
if (....)
{
....
}
else if ( lowerBatchText.Contains("name")
&& lowerBatchText.Contains("state")
&& lowerBatchText.Contains("databases")
&& lowerBatchText.Contains("db_name"))
// SELECT [name], [state] FROM [sys].[databases] WHERE [name] = db_name()
{
// Delegate to current database response
responseMessage = _PrepareDatabaseResponse(session);
}
....
}PVS-Studio warning: V3053 An excessive expression. Examine the substrings 'name' and 'db_name'. QueryEngine.cs 151
The fact is that in this case the combination of subexpressions lowerBatchText.Contains(«name») and lowerBatchText.Contains(«db_name») is redundant. Indeed, if the checked string contains the substring «db_name», it will contain the «name» substring as well. If the string doesn't contain «name», it won't contain «db_name» either. As a result, it turns out that the check lowerBatchText.Contains(«name») is redundant. Unless it can reduce the number of evaluated expressions, if the checked string doesn't contain «name».
Issue 52
A suspicious fragment from the code of the System.Net.Requests project.
protected override PipelineInstruction PipelineCallback(
PipelineEntry entry, ResponseDescription response, ....)
{
if (NetEventSource.IsEnabled)
NetEventSource.Info(this,
$"Command:{entry?.Command} Description:{response?.StatusDescription}");
// null response is not expected
if (response == null)
return PipelineInstruction.Abort;
....
if (entry.Command == "OPTS utf8 on\r\n")
....
....
}PVS-Studio warning: V3125 The 'entry' object was used after it was verified against null. Check lines: 270, 227. FtpControlStream.cs 270
When composing an interpolated string, such expressions as entry?.Command and response?.Description are used. The '?.' operator is used instead of the '.' operator not to get an exception of the NullReferenceException type in case if any of the corresponding parameters has the null value. In this case, this technique works. Further, as we can see from the code, a possible null value for response gets split off (exit from the method if response == null), whereas there's nothing similar for entry. As a result, if entry — null further along the code when evaluating entry.Command (with the usage of '.', not '?.'), an exception will be generated.
At this point, a fairly detailed code review is waiting for us, so I suggest that you have another break — chill out, make some tea or coffee. After that I'll be right here to continue.
Are you back? Then let's keep going. :)
Issue 53
Now let's find something interesting in the System.Collections.Immutable project. This time we'll have some experiments with the System.Collections.Immutable.ImmutableArray<T> struct. The methods IStructuralEquatable.Equals and IStructuralComparable.CompareTo are of special interest for us.
Let's start with the IStructuralEquatable.Equals method. The code is given below, I suggest that you try to get what's wrong yourself:
bool IStructuralEquatable.Equals(object other, IEqualityComparer comparer)
{
var self = this;
Array otherArray = other as Array;
if (otherArray == null)
{
var theirs = other as IImmutableArray;
if (theirs != null)
{
otherArray = theirs.Array;
if (self.array == null && otherArray == null)
{
return true;
}
else if (self.array == null)
{
return false;
}
}
}
IStructuralEquatable ours = self.array;
return ours.Equals(otherArray, comparer);
}Did you manage? If yes — my congrats. :)
PVS-Studio warning: V3125 The 'ours' object was used after it was verified against null. Check lines: 1212, 1204. ImmutableArray_1.cs 1212
The analyzer was confused by the call of the instance Equals method through the ours variable, located in the last return expression, as it suggests that an exception of the NullReferenceException type might occur here. Why does the analyzer suggest so? To make it easier to explain, I'm giving a simplified code fragment of the same method below.
bool IStructuralEquatable.Equals(object other, IEqualityComparer comparer)
{
....
if (....)
{
....
if (....)
{
....
if (self.array == null && otherArray == null)
{
....
}
else if (self.array == null)
{
....
}
}
}
IStructuralEquatable ours = self.array;
return ours.Equals(otherArray, comparer);
}In the last expressions, we can see, that the value of the ours variable comes from self.array. The check self.array == null is performed several times above. Which means, ours, the same as self.array, can have the null value. At least in theory. Is this state reachable in practice? Let's try to find out. To do this, once again I cite the body of the method with set key points.
bool IStructuralEquatable.Equals(object other, IEqualityComparer comparer)
{
var self = this; // <= 1
Array otherArray = other as Array;
if (otherArray == null) // <= 2
{
var theirs = other as IImmutableArray;
if (theirs != null) // <= 3
{
otherArray = theirs.Array;
if (self.array == null && otherArray == null)
{
return true;
}
else if (self.array == null) // <= 4
{
return false;
}
}
IStructuralEquatable ours = self.array; // <= 5
return ours.Equals(otherArray, comparer);
}Key point 1. self.array == this.array (due to self = this). Therefore, before calling the method, we need to get the condition this.array == null.
Key point 2. We can ignore this if, which will be the simplest way to get what we want. To ignore this if, we only need the other variable to be of the Array type or a derived one, and not to contain the null value. This way, after using the as operator, a non-null reference will be written in otherArray and we'll ignore the first if statement.
Key point 3. This point requires a more complex approach. We definitely need to exit on the second if statement (the one with the conditional expression theirs != null). If it doesn't happen and then branch starts to execute, most certainly we won't get the needed point 5 under the condition self.array == null due to the key point 4. To avoid entering the if statement of the key point 3, one of these conditions has to be met:
- the other value has to be null;
- the actual other type mustn't implement the IImmutableArray interface.
Key point 5. If we get to this point with the value self.array == null, it means that we've reached our aim, and an exception of the NullReferenceException type will be generated.
We get the following datasets that will lead us to the needed point.
First: this.array — null.
Second — one of the following ones:
- other — null;
- other has the Array type or one derived from it;
- other doesn't have the Array type or a derived from it and in doing so, doesn't implement the IImmutableArray interface.
array is the field, declared in the following way:
internal T[] array;As ImmutableArray<T> is a structure, it has a default constructor (without arguments) that will result in the array field taking value by default, which is null. And that's what we need.
Let's not forget that we were investigating an explicit implementation of the interface method, therefore, casting has to be done before the call.
Now we have the game in hands to reach the exception occurrence in three ways. We add reference to the debugging library version, write the code, execute and see what happens.
Code fragment 1.
var comparer = EqualityComparer<String>.Default;
ImmutableArray<String> immutableArray = new ImmutableArray<string>();
((IStructuralEquatable)immutableArray).Equals(null, comparer);Code fragment 2.
var comparer = EqualityComparer<String>.Default;
ImmutableArray<String> immutableArray = new ImmutableArray<string>();
((IStructuralEquatable)immutableArray).Equals(new string[] { }, comparer);Code fragment 3.
var comparer = EqualityComparer<String>.Default;
ImmutableArray<String> immutableArray = new ImmutableArray<string>();
((IStructuralEquatable)immutableArray).Equals(typeof(Object), comparer);The execution result of all three code fragments will be the same, only achieved by different input entry data, and execution paths.
Issue 54
If you didn't forget, we have another method that we need to discredit. :) But this time we won't cover it in such detail. Moreover, we already know some information from the previous example.
int IStructuralComparable.CompareTo(object other, IComparer comparer)
{
var self = this;
Array otherArray = other as Array;
if (otherArray == null)
{
var theirs = other as IImmutableArray;
if (theirs != null)
{
otherArray = theirs.Array;
if (self.array == null && otherArray == null)
{
return 0;
}
else if (self.array == null ^ otherArray == null)
{
throw new ArgumentException(
SR.ArrayInitializedStateNotEqual, nameof(other));
}
}
}
if (otherArray != null)
{
IStructuralComparable ours = self.array;
return ours.CompareTo(otherArray, comparer); // <=
}
throw new ArgumentException(SR.ArrayLengthsNotEqual, nameof(other));
}PVS-Studio warning: V3125 The 'ours' object was used after it was verified against null. Check lines: 1265, 1251. ImmutableArray_1.cs 1265
As you can see, the case is very similar to the previous example.
Let's write the following code:
Object other = ....;
var comparer = Comparer<String>.Default;
ImmutableArray<String> immutableArray = new ImmutableArray<string>();
((IStructuralComparable)immutableArray).CompareTo(other, comparer);We'll try to find some entry data to reach the point, where exception of the NullReferenceException type might occur:
Value: other — new String[]{ };
Result:
Thus, we again managed to figure out such data, with which an exception occurs in the method.
Issue 55
In the System.Net.HttpListener project I stumbled upon several both suspicious and very similar places. Once again, I can't shake the feeling about copy-paste, taking place here. Since the pattern is the same, we'll look at one code example. I'll cite analyzer warnings for the rest cases.
public override IAsyncResult BeginRead(byte[] buffer, ....)
{
if (NetEventSource.IsEnabled)
{
NetEventSource.Enter(this);
NetEventSource.Info(this,
"buffer.Length:" + buffer.Length +
" size:" + size +
" offset:" + offset);
}
if (buffer == null)
{
throw new ArgumentNullException(nameof(buffer));
}
....
}PVS-Studio warning: V3095 The 'buffer' object was used before it was verified against null. Check lines: 51, 53. HttpRequestStream.cs 51
Generation of an exception of the ArgumentNullException type under the condition buffer == null obviously suggests that null is an unacceptable value for this variable. However, if the value of the NetEventSource.IsEnabled expression is true and buffer — null, when evaluating the buffer.Length expression, an exception of the NullReferenceException type will be generated. As we can see, we won't even reach the buffer == null check in this case.
PVS-Studio warnings issued for other methods with the pattern:
- V3095 The 'buffer' object was used before it was verified against null. Check lines: 49, 51. HttpResponseStream.cs 49
- V3095 The 'buffer' object was used before it was verified against null. Check lines: 74, 75. HttpResponseStream.cs 74
Issue 56
A similar code snippet was in the System.Transactions.Local project.
internal override void EnterState(InternalTransaction tx)
{
if (tx._outcomeSource._isoLevel == IsolationLevel.Snapshot)
{
throw TransactionException.CreateInvalidOperationException(
TraceSourceType.TraceSourceLtm,
SR.CannotPromoteSnapshot,
null,
tx == null ? Guid.Empty : tx.DistributedTxId);
}
....
}PVS-Studio warning: V3095 The 'tx' object was used before it was verified against null. Check lines: 3282, 3285. TransactionState.cs 3282
Under a certain condition, an author wants to throw an exception of the InvalidOperationException type. When calling the method for creating an exception object, code authors use the tx parameter, check it for null to avoid an exception of the NullReferenceException type when evaluating the tx.DistributedTxId expression. It's ironic that the check won't be of help, as when evaluating the condition of the if statement, instance fields are accessed via the tx variable — tx._outcomeSource._isoLevel.
Issue 57
Code from the System.Runtime.Caching project.
internal void SetLimit(int cacheMemoryLimitMegabytes)
{
long cacheMemoryLimit = cacheMemoryLimitMegabytes;
cacheMemoryLimit = cacheMemoryLimit << MEGABYTE_SHIFT;
_memoryLimit = 0;
// never override what the user specifies as the limit;
// only call AutoPrivateBytesLimit when the user does not specify one.
if (cacheMemoryLimit == 0 && _memoryLimit == 0)
{
// Zero means we impose a limit
_memoryLimit = EffectiveProcessMemoryLimit;
}
else if (cacheMemoryLimit != 0 && _memoryLimit != 0)
{
// Take the min of "cache memory limit" and
// the host's "process memory limit".
_memoryLimit = Math.Min(_memoryLimit, cacheMemoryLimit);
}
else if (cacheMemoryLimit != 0)
{
// _memoryLimit is 0, but "cache memory limit"
// is non-zero, so use it as the limit
_memoryLimit = cacheMemoryLimit;
}
....
}PVS-Studio warning: V3022 Expression 'cacheMemoryLimit != 0 && _memoryLimit != 0' is always false. CacheMemoryMonitor.cs 250
If you look closely at the code, you'll notice that one of the expressions — cacheMemoryLimit != 0 && _memoryLimit != 0 will always be false. Since _memoryLimit has the 0 value (is set before the if statement), the right operand of the && operator is false. Therefore, the result of the entire expression is false.
Issue 58
I cite a suspicious code fragment from the System.Diagnostics.TraceSource project below.
public override object Pop()
{
StackNode n = _stack.Value;
if (n == null)
{
base.Pop();
}
_stack.Value = n.Prev;
return n.Value;
}PVS-Studio warning: V3125 The 'n' object was used after it was verified against null. Check lines: 115, 111. CorrelationManager.cs 115
In fact, it is an interesting case. Due to the check n == null, I assume, that null is an expected value for this local variable. If so, an exception of the NullReferenceException type will be generated when accessing the instance property — n.Prev. If in this case n can never be null, base.Pop() will never be called.
Issue 59
An interesting code fragment from the System.Drawing.Primitives project. Again, I suggest that you try to find the problem yourself. Here's the code:
public static string ToHtml(Color c)
{
string colorString = string.Empty;
if (c.IsEmpty)
return colorString;
if (ColorUtil.IsSystemColor(c))
{
switch (c.ToKnownColor())
{
case KnownColor.ActiveBorder:
colorString = "activeborder";
break;
case KnownColor.GradientActiveCaption:
case KnownColor.ActiveCaption:
colorString = "activecaption";
break;
case KnownColor.AppWorkspace:
colorString = "appworkspace";
break;
case KnownColor.Desktop:
colorString = "background";
break;
case KnownColor.Control:
colorString = "buttonface";
break;
case KnownColor.ControlLight:
colorString = "buttonface";
break;
case KnownColor.ControlDark:
colorString = "buttonshadow";
break;
case KnownColor.ControlText:
colorString = "buttontext";
break;
case KnownColor.ActiveCaptionText:
colorString = "captiontext";
break;
case KnownColor.GrayText:
colorString = "graytext";
break;
case KnownColor.HotTrack:
case KnownColor.Highlight:
colorString = "highlight";
break;
case KnownColor.MenuHighlight:
case KnownColor.HighlightText:
colorString = "highlighttext";
break;
case KnownColor.InactiveBorder:
colorString = "inactiveborder";
break;
case KnownColor.GradientInactiveCaption:
case KnownColor.InactiveCaption:
colorString = "inactivecaption";
break;
case KnownColor.InactiveCaptionText:
colorString = "inactivecaptiontext";
break;
case KnownColor.Info:
colorString = "infobackground";
break;
case KnownColor.InfoText:
colorString = "infotext";
break;
case KnownColor.MenuBar:
case KnownColor.Menu:
colorString = "menu";
break;
case KnownColor.MenuText:
colorString = "menutext";
break;
case KnownColor.ScrollBar:
colorString = "scrollbar";
break;
case KnownColor.ControlDarkDark:
colorString = "threeddarkshadow";
break;
case KnownColor.ControlLightLight:
colorString = "buttonhighlight";
break;
case KnownColor.Window:
colorString = "window";
break;
case KnownColor.WindowFrame:
colorString = "windowframe";
break;
case KnownColor.WindowText:
colorString = "windowtext";
break;
}
}
else if (c.IsNamedColor)
{
if (c == Color.LightGray)
{
// special case due to mismatch between Html and enum spelling
colorString = "LightGrey";
}
else
{
colorString = c.Name;
}
}
else
{
colorString = "#" + c.R.ToString("X2", null) +
c.G.ToString("X2", null) +
c.B.ToString("X2", null);
}
return colorString;
}Okay, okay, just kidding… Or did you still find something? Anyway, let's reduce the code to clearly state the issue.
Here is the short code version:
switch (c.ToKnownColor())
{
....
case KnownColor.Control:
colorString = "buttonface";
break;
case KnownColor.ControlLight:
colorString = "buttonface";
break;
....
}PVS-Studio warning: V3139 Two or more case-branches perform the same actions. ColorTranslator.cs 302
I can't say for sure, but I think it's an error. In other cases, when a developer wanted to return the same value for several enumerators he used several case(s), following each other. And it's easy enough to make a mistake with copy-paste here, I think.
Let's dig a little deeper. To get the «buttonface» value from the analyzed ToHtml method, you can pass one of the following values to it (expected):
- SystemColors.Control;
- SystemColors.ControlLight.
If we check ARGB values for each of these colors, we'll see the following:
- SystemColors.Control — (255, 240, 240, 240);
- SystemColors.ControlLight — (255, 227, 227, 227).
If we call the inverse conversion method FromHtml on the received value («buttonface»), we'll get the color Control (255, 240, 240, 240). Can we get the ControlLight color from FromHtml? Yes. This method contains the table of colors, which is the basis for composing colors (in this case). The table's initializer has the following line:
s_htmlSysColorTable["threedhighlight"]
= ColorUtil.FromKnownColor(KnownColor.ControlLight);Accordingly, FromHtml returns the ControlLight (255, 227, 227, 227) color for the «threedhighlight» value. I think that's exactly what should have been used in case KnownColor.ControlLight.
Issue 60
We'll check out a couple of interesting warnings from the System.Text.RegularExpressions project.
internal virtual string TextposDescription()
{
var sb = new StringBuilder();
int remaining;
sb.Append(runtextpos);
if (sb.Length < 8)
sb.Append(' ', 8 - sb.Length);
if (runtextpos > runtextbeg)
sb.Append(RegexCharClass.CharDescription(runtext[runtextpos - 1]));
else
sb.Append('^');
sb.Append('>');
remaining = runtextend - runtextpos;
for (int i = runtextpos; i < runtextend; i++)
{
sb.Append(RegexCharClass.CharDescription(runtext[i]));
}
if (sb.Length >= 64)
{
sb.Length = 61;
sb.Append("...");
}
else
{
sb.Append('$');
}
return sb.ToString();
}PVS-Studio warning: V3137 The 'remaining' variable is assigned but is not used by the end of the function. RegexRunner.cs 612
A value is written in the local remaining variable, but it's not longer used in the method. Perhaps, some code, using it, was removed, but the variable itself was forgotten. Or there is a crucial error and this variable has to somehow be used.
Issue 61
public void AddRange(char first, char last)
{
_rangelist.Add(new SingleRange(first, last));
if (_canonical && _rangelist.Count > 0 &&
first <= _rangelist[_rangelist.Count - 1].Last)
{
_canonical = false;
}
}PVS-Studio warning: V3063 A part of conditional expression is always true if it is evaluated: _rangelist.Count > 0. RegexCharClass.cs 523
The analyzer rightly noted, that a part of the expression _rangelist.Count > 0 will always be true, if this code is executed. Even if this list (which _rangelist points at), was empty, after adding the element _rangelist.Add(....) it wouldn't be the same.
Issue 62
Let's look at the warnings of the V3128 diagnostic rule in the projects System.Drawing.Common and System.Transactions.Local.
private class ArrayEnumerator : IEnumerator
{
private object[] _array;
private object _item;
private int _index;
private int _startIndex;
private int _endIndex;
public ArrayEnumerator(object[] array, int startIndex, int count)
{
_array = array;
_startIndex = startIndex;
_endIndex = _index + count;
_index = _startIndex;
}
....
}PVS-Studio warning: V3128 The '_index' field is used before it is initialized in constructor. PrinterSettings.Windows.cs 1679
When initializing the _endIndex field, another _index field is used, which has a standard value default(int), (that is 0) at the moment of its usage. The _index field is initialized below. In case if it's not an error — the _index variable should have been omitted in this expression not to be confusing.
Issue 63
internal class TransactionTable
{
....
private int _timerInterval;
....
internal TransactionTable()
{
// Create a timer that is initially disabled by specifing
// an Infinite time to the first interval
_timer = new Timer(new TimerCallback(ThreadTimer),
null,
Timeout.Infinite,
_timerInterval);
....
// Store the timer interval
_timerInterval = 1 << TransactionTable.timerInternalExponent;
....
}
}PVS-Studio warning: V3128 The '_timerInterval' field is used before it is initialized in constructor. TransactionTable.cs 151
The case is similar to the one above. First the value of the _timerInterval field is used (while it's still default(int)) to initialize _timer. Only after that the _timerInterval field itself will be initialized.
Issue 64
Next warnings were issued by the diagnostic rule, which is still in development. There's no documentation or final message, but we've already found a couple of interesting fragments with its help. Again these fragments look like copy-paste, so we'll consider only one code fragment.
private bool ProcessNotifyConnection(....)
{
....
WeakReference reference = (WeakReference)(
LdapConnection.s_handleTable[referralFromConnection]);
if ( reference != null
&& reference.IsAlive
&& null != ((LdapConnection)reference.Target)._ldapHandle)
{ .... }
....
}PVS-Studio warning (stub): VXXXX TODO_MESSAGE. LdapSessionOptions.cs 974
The trick is that after checking reference.IsAlive, garbage might be collected and the object, which WeakReference points to, will be garbage collected. In this case, Target will return the null value. As a result, when accessing the instance field _ldapHandle, an exception of the NullReferenceException type will occur. Microsoft itself warns about this trap with the check IsAlive. A quote from docs.microsoft.com — "WeakReference.IsAlive Property": Because an object could potentially be reclaimed for garbage collection immediately after the IsAlive property returns true, using this property is not recommended unless you are testing only for a false return value.
Summary on Analysis
Are these all errors and interesting places, found during the analysis? Of course, not! When looking through the analysis results, I was thoroughly checking out the warnings. As their number increased and it became clear there were enough of them for an article, I was scrolling through the results, trying to select only the ones that seemed to me the most interesting. When I got to the last ones (the largest logs), I was only able to look though the warnings until the sight caught on something unusual. So if you dig around, I'm sure you can find much more interesting places.
For example, I ignored almost all V3022 and V3063 warnings. So to speak, if I came across such code:
String str = null;
if (str == null)
....I would omit it, as there were many other interesting places that I wanted to describe. There were warnings on unsafe locking using the lock statement with locking by this and so on — V3090; unsafe event calls — V3083; objects, which types implement IDisposable, but for which Dispose / Close isn't called — V3072 and similar diagnostics and much more.
I also didn't note problems, written in tests. At least, I tried, but could accidentally take some. Except for a couple of places that I found interesting enough to draw attention to them. But the testing code can also contain errors, due to which the tests will work incorrectly.
Generally, there are still many things to investigate — but I didn't have the intention to mark all found issues.
The quality of the code seemed uneven to me. Some projects were perfectly clean, others contained suspicious places. Perhaps we might expect clean projects, especially when it comes to the most commonly used library classes.
To sum up, we can say, that the code is of quite high-quality, as its amount was considerable. But, as this article suggests, there were some dark corners.
By the way, a project of this size is also a good test for the analyzer. I managed to find a number of false / weird warnings that I selected to study and correct. So as a result of the analysis, I managed to find the points, where we have to work on the PVS-Studio itself.
Conclusion
If you got to this place by reading the whole article — let me shake your hand! I hope that I was able to show you interesting errors and demonstrate the benefit of static analysis. If you have learned something new for yourself, that will let you write better code — I will be doubly pleased.
Anyway, some help by the static analysis won't hurt, so suggest that you try PVS-Studio on your project and see what interesting places can be found with its usage. If you have any questions or you just want to share interesting found fragments — don't hesitate to write at support@viva64.com. :)
Best regards!
P.S. For .NET Core libraries developers
Thank you so much for what you do! Good job! Hopefully this article will help you make the code a bit better. Remember, that I haven't written all suspicious places and you'd better check the project yourself using the analyzer. This way, you'll be able to investigate all warnings in details. Moreover, it'll be more convenient to work with it, rather than with simple text log / list of errors (I wrote about this in more details here).