# SARIF SDK and its errors
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](https://www.oasis-open.org/committees/tc_home.php?wg_abbrev=sarif) website\. The source code of SARIF SDK can be downloaded from the repository on [GiHub](https://github.com/microsoft/sarif-sdk)\. The project's homepage is available by [link](http://sarifweb.azurewebsites.net/)\.
## About the project
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\.
## Errors
[V3070](https://pvs-studio.com/en/docs/warnings/v3070/) \[CWE\-457\] Uninitialized variable 'Binary' is used when initializing the 'Default' variable\. MimeType\.cs 90
```cpp
public static class MimeType
{
....
/// The MIME type to use when no better MIME type is known.
public static readonly string Default = Binary;
....
/// The MIME type for binaries.
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](https://pvs-studio.com/en/docs/warnings/v3061/) Parameter 'logicalLocationToIndexMap' is always rewritten in method body before being used\. PrereleaseCompatibilityTransformer\.cs 1963
```cpp
private static JArray ConvertLogicalLocationsDictionaryToArray(
....
Dictionary logicalLocationToIndexMap,
....)
{
....
logicalLocationToIndexMap =
new Dictionary(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:
```cpp
private static bool ApplyChangesFromTC25ThroughTC30(....)
{
....
Dictionary logicalLocationToIndexMap = null;
....
logicalLocationToIndexMap =
new Dictionary(LogicalLocation.ValueComparer);
run["logicalLocations"] =
ConvertLogicalLocationsDictionaryToArray(
....,
logicalLocationToIndexMap,
....);
}
```
Weird and suspicious code\.
[V3008](https://pvs-studio.com/en/docs/warnings/v3008/) \[CWE\-563\] The 'run\.Tool' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 116, 114\. ExportRulesMetadataCommandBase\.cs 116
```cpp
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](https://pvs-studio.com/en/docs/warnings/v3042/) \[CWE\-476\] Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the 'loc' object WhereComparer\.cs 152
```cpp
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](https://pvs-studio.com/en/docs/warnings/v3042/) \[CWE\-476\] Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the 'formatString' object InsertOptionalDataVisitor\.cs 194
```cpp
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](https://pvs-studio.com/en/docs/warnings/v3042/) \[CWE\-476\] Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the 'messageText' object FortifyFprConverter\.cs 1210
[V3042](https://pvs-studio.com/en/docs/warnings/v3042/) \[CWE\-476\] Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the 'messageText' object FortifyFprConverter\.cs 1216
```cpp
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](https://pvs-studio.com/en/docs/warnings/v3080/) \[CWE\-476\] Possible null dereference\. Consider inspecting 'fileDataVersionOne\.Uri'\. SarifCurrentToVersionOneVisitor\.cs 1030
```cpp
private IDictionary
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:
```cpp
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](https://pvs-studio.com/en/docs/warnings/v3080/) \[CWE\-476\] Possible null dereference\. Consider inspecting '\_jsonTextWriter'\. SarifLogger\.cs 242
```cpp
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](https://pvs-studio.com/en/docs/warnings/v3083/) \[CWE\-367\] Unsafe invocation of event 'RuleRead', NullReferenceException is possible\. Consider assigning event to a local variable before invoking it\. FxCopConverter\.cs 897
```cpp
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:
```cpp
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\.
* V3083 \[CWE\-367\] Unsafe invocation of event 'ResultRead', NullReferenceException is possible\. Consider assigning event to a local variable before invoking it\. FxCopConverter\.cs 813
[V3095](https://pvs-studio.com/en/docs/warnings/v3095/) \[CWE\-476\] The 'v1Location' object was used before it was verified against null\. Check lines: 333, 335\. SarifVersionOneToCurrentVisitor\.cs 333
```cpp
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](https://pvs-studio.com/en/docs/warnings/v3125/) \[CWE\-476\] The 'v1StackFrame' object was used after it was verified against null\. Check lines: 1182, 1171\. SarifVersionOneToCurrentVisitor\.cs 1182
```cpp
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:
```cpp
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\.
## Conclusion
As you see, the article isn't long but I hope you enjoyed this light reading :\) Just in case, that you can always [download](https://pvs-studio.com/en/pvs-studio/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](https://pvs-studio.com/en/blog/posts/)\. See you\!
To learn more about new blog posts, you are welcome to subscribe to the following channels:
* Twitter: [@Code\_Analysis](https://twitter.com/Code_Analysis)
* Telegram: [pvsstudio\_en](https://t.me/pvsstudio_en)
* Good old RSS: [viva64\-blog\-en](http://feeds.feedburner.com/viva64-blog-en)