Our website uses cookies to enhance your browsing experience.
Your message has been sent. We will email you at
If you do not see the email in your inbox, please check if it is filtered to one of the following folders:
Webinar: C++ semantics - 06.11
Today we have another high-quality Microsoft project to be checked, which we'll heroically delve into trying to find errors with PVS-Studio. SARIF, an acronym for Static Analysis Interchange Format, which is a standard (file format), designed to interact and share the results of static analyzers with other tools: IDEs, complex code verification and analysis tools (e.g. SonarQube), continuous integration systems, etc. SARIF SDK, respectively, contains .NET developer tools to support SARIF as well as additional files.
SARIF originated at Microsoft and is now a standard developed by OASIS (a non-profit consortium that deals with open standards). SARIF is meant to pass not only the results of the analyzer, but also metadata about the tool, as well as data on how it was launched, time tags, and so on. For more information, visit the OASIS website. The source code of SARIF SDK can be downloaded from the repository on GiHub. The project's homepage is available by link.
The SARIF SDK project turned out to be small: 799 .cs files (approximately 98,000 non-empty lines of code). The project contains tests that I always exclude from the check. Thus, the part of the code we were interested in was 642 .cs files (approximately 79,000 non-empty lines of code). It's certainly not enough. On the plus side, the check and analysis were easy and fast, between this and then, which I tried to reflect on the picture at the beginning. Nonetheless, I managed to track down some uncanny cases. Let's have a look at them.
V3070 [CWE-457] Uninitialized variable 'Binary' is used when initializing the 'Default' variable. MimeType.cs 90
public static class MimeType
{
....
/// <summary>The MIME type to use when no better MIME type is known.</summary>
public static readonly string Default = Binary;
....
/// <summary>The MIME type for binaries.</summary>
public static readonly string Binary = "application/octet-stream";
....
}The field is initialized by the value of another field, which hasn't received a value yet. As a result, Default will receive the null value by default for the string type. Most likely, the error remained unnoticed, as the Default field isn't used anywhere. But things can change, and then the developer will face an undue result or the program crash.
V3061 Parameter 'logicalLocationToIndexMap' is always rewritten in method body before being used. PrereleaseCompatibilityTransformer.cs 1963
private static JArray ConvertLogicalLocationsDictionaryToArray(
....
Dictionary<LogicalLocation, int> logicalLocationToIndexMap,
....)
{
....
logicalLocationToIndexMap =
new Dictionary<LogicalLocation, int>(LogicalLocation.ValueComparer);
....
}The code author doesn't use the logicalLocationToIndexMap parameter in any way, but writes a different value in it. Curiously enough, the previous value is exactly the same empty dictionary, created in the caller code:
private static bool ApplyChangesFromTC25ThroughTC30(....)
{
....
Dictionary<LogicalLocation, int> logicalLocationToIndexMap = null;
....
logicalLocationToIndexMap =
new Dictionary<LogicalLocation, int>(LogicalLocation.ValueComparer);
run["logicalLocations"] =
ConvertLogicalLocationsDictionaryToArray(
....,
logicalLocationToIndexMap,
....);
}Weird and suspicious code.
V3008 [CWE-563] The 'run.Tool' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 116, 114. ExportRulesMetadataCommandBase.cs 116
public partial class Run
{
....
public Tool Tool { get; set; }
....
}
public partial class Tool : ....
{
....
public Tool()
{
}
....
}
private void OutputSarifRulesMetada(....)
{
....
var run = new Run();
run.Tool = new Tool();
run.Tool = Tool.CreateFromAssemblyData(....); // <=
....
}The run.Tool property is assigned a value twice. Both when creating the Tool object and when writing a value in the Tool property, no additional work is required. Therefore, reassigning smells fishy.
V3042 [CWE-476] Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'loc' object WhereComparer.cs 152
private static Uri ArtifactUri(ArtifactLocation loc, Run run)
{
return loc?.Uri ?? loc.Resolve(run)?.Uri;
}If the value of the loc variable is null, an attempt will be made to return the value from the right part of the ?? operator, resulting in the access by null reference.
V3042 [CWE-476] Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'formatString' object InsertOptionalDataVisitor.cs 194
public override Message VisitMessage(Message node)
{
....
node.Text = node.Arguments?.Count > 0
? string.Format(...., formatString.Text, ....)
: formatString?.Text;
....
}Developers use unsecure and secure access variants by a potentially null formatString reference in two parallel branches of the conditional ?: operator.
V3042 [CWE-476] Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'messageText' object FortifyFprConverter.cs 1210
V3042 [CWE-476] Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'messageText' object FortifyFprConverter.cs 1216
private void AddMessagesToResult(Result result)
{
....
string messageText = (rule.ShortDescription ?? rule.FullDescription)?.Text;
....
if (....)
{
// Replace the token with an embedded hyperlink.
messageText = messageText.Replace(....);
}
else
{
// Replace the token with plain text.
messageText = messageText.Replace(....);
}
....
}Here the analyzer issued already two warnings about possible access by the null messageText reference. It looks rather nonthreatening, but it's still an error.
V3080 [CWE-476] Possible null dereference. Consider inspecting 'fileDataVersionOne.Uri'. SarifCurrentToVersionOneVisitor.cs 1030
private IDictionary<string, FileDataVersionOne>
CreateFileDataVersionOneDictionary()
{
....
FileDataVersionOne fileDataVersionOne = CreateFileDataVersionOne(v2File);
if (fileDataVersionOne.Uri.OriginalString.Equals(key))
{
....
}
....
}The analyzer suspected that NullReferenceException is possible when working with the fileDataVersionOne.Uri reference. Let's see where this variable comes from and find out if the analyzer is right. To do this, let's take a close look at the body of the CreateFileDataVersionOne method:
private FileDataVersionOne CreateFileDataVersionOne(Artifact v2FileData)
{
FileDataVersionOne fileData = null;
if (v2FileData != null)
{
....
fileData = new FileDataVersionOne
{
....
Uri = v2FileData.Location?.Uri,
....
};
....
}
return fileData;
}
public partial class FileDataVersionOne
{
....
public Uri Uri { get; set; }
....
}Indeed, when creating the object of the FileDataVersionOne class, the Uri property might receive the null value. This is a great example data flow analysis and interprocedural analysis mechanisms working together.
V3080 [CWE-476] Possible null dereference. Consider inspecting '_jsonTextWriter'. SarifLogger.cs 242
public virtual void Dispose()
{
....
if (_closeWriterOnDispose)
{
if (_textWriter != null) { _textWriter.Dispose(); }
if (_jsonTextWriter == null) { _jsonTextWriter.Close(); } // <=
}
....
}There's a typo in this fragment. It's clear that _jsonTextWriter != null has to be in the condition of the second block. This piece of code is jeopardizing as, most likely, it doesn't crash, due to _jsonTextWriter being nonnull. Besides, the stream remains open.
V3083 [CWE-367] Unsafe invocation of event 'RuleRead', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. FxCopConverter.cs 897
private void ReadRule(....)
{
....
if (RuleRead != null)
{
RuleRead(....);
}
....
}Events are handled unsafely. It's an uncritical bug that can be easily fixed, for example, by following the Visual Studio tip. Here's the replacement suggested by the IDE:
private void ReadRule(....)
{
....
RuleRead?.Invoke(....);
....
}It takes only a few seconds to fix it, but the analyzer will no longer complain about it and the IDE won't highlight the code. Another similar error.
V3095 [CWE-476] The 'v1Location' object was used before it was verified against null. Check lines: 333, 335. SarifVersionOneToCurrentVisitor.cs 333
internal Location CreateLocation(LocationVersionOne v1Location)
{
....
string key = v1Location.LogicalLocationKey ??
v1Location.FullyQualifiedLogicalName;
if (v1Location != null)
{
....
}
....
}The author thought that the v1Location reference may be null and added an appropriate check. Whereas above we can see that this reference is handled without any checks. Inattentive refactoring? Well, you never know.
V3125 [CWE-476] The 'v1StackFrame' object was used after it was verified against null. Check lines: 1182, 1171. SarifVersionOneToCurrentVisitor.cs 1182
internal StackFrame CreateStackFrame(StackFrameVersionOne v1StackFrame)
{
StackFrame stackFrame = null;
if (v1StackFrame != null)
{
stackFrame = new StackFrame
{
....
};
}
stackFrame.Location =
CreateLocation(v1StackFrame.FullyQualifiedLogicalName,
v1StackFrame.LogicalLocationKey,
....);
return stackFrame;
}As always, here comes a reverse case. First the v1StackFrame reference is checked for null, and then the check is gone astray. But this case has an important caveat: v1StackFrame and stackFrame variables are logically related. See, if v1StackFrame is null, the StackFrame object won't be created, whereas stackFrame will remain null. Followed by the program crash due to a call of stackFrame.Location, as there are no checks here. So it won't even come to the dangerous v1StackFrame usage, indicated by the analyzer. This code only works if you pass nonnull v1StackFrame values to the CreateStackFrame method. I suspected that the caller code somehow controls it. CreateStackFrame calls look like this:
Frames = v1Stack.Frames?.Select(CreateStackFrame).ToList()CreateStackFrame is used as a selector. Passed references aren't checked for null here. Perhaps, when filling the Frames collection it (writing of null references) is controlled, but I didn't go for digging too deep. The conclusion is already obvious - the code requires authors' attention.
As you see, the article isn't long but I hope you enjoyed this light reading :) Just in case, that you can always download our analyzer to search for errors in your or someone's projects yourself.
And finally, a small announcement: my next article will be about the most interesting errors that my colleagues and I found in projects in 2019. Follow our blog. See you!
To learn more about new blog posts, you are welcome to subscribe to the following channels:
0
0
0
0