# SARIF SDK и его ошибки
Сегодня у нас на тесте очередной качественный проект Microsoft, в котором мы всё же попытаемся героически поискать ошибки при помощи PVS\-Studio\. SARIF – аббревиатура от "Static Analysis Results Interchange Format", представляет собой стандарт \(формат файла\), предназначенный для взаимодействия и обмена результатами работы статических анализаторов с другими инструментами: IDE, комплексными инструментами проверки и анализа кода \(например, SonarQube\), системами непрерывной интеграции и т\.п\. SARIF SDK, соответственно, содержит инструментарий разработчика \.NET для поддержки SARIF, а также вспомогательные файлы\.
SARIF возник в Microsoft и теперь является стандартом, разрабатываемым в рамках OASIS \(некоммерческий консорциум, который занимается открытыми стандартами\)\. SARIF предназначен для передачи не только результатов работы анализатора, но и метаданных об инструменте, а также данных о том, как он был запущен, временных метках и так далее\. Более подробно со стандартом можно ознакомиться на сайте [OASIS](https://www.oasis-open.org/committees/tc_home.php?wg_abbrev=sarif)\. Исходный код SARIF SDK можно скачать с репозитория на [GiHub](https://github.com/microsoft/sarif-sdk)\. Домашняя страница проекта доступна по [ссылке](http://sarifweb.azurewebsites.net/)\.
## О проекте
Проект SARIF SDK оказался маленьким: 799 файлов \.cs \(примерно 98 000 непустых строк кода\)\. В составе проекта содержатся тесты, которые я всегда исключаю из проверки\. Таким образом, интересующая нас часть кода составила 642 файла \.cs \(примерно 79 000 непустых строк кода\)\. Это, конечно, мало\. Зато проверка и анализ прошли легко и быстро, между делом, что я и попытался отразить на картинке в начале статьи\. Тем не менее, кое\-какие интересные ошибки найти удалось\. Давайте на них посмотрим\.
## Ошибки
[V3070](https://pvs-studio.ru/ru/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";
....
}
```
Поле _Default_ инициализируют значением другого поля, которое еще не получило значения\. В результате, _Default_ получит значение по умолчанию для типа _string_ – null\. Вероятно, ошибку не заметили, потому что поле _Default_ нигде не используют\. Но всё может измениться, и тогда разработчик столкнётся с непредвиденным результатом или падением программы\.
[V3061](https://pvs-studio.ru/ru/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);
....
}
```
Параметр _logicalLocationToIndexMap_ никак не используют, записывая в него другое значение\. Любопытно, но старое значение \- точно такой же пустой словарь, который создают в вызывающем методе:
```cpp
private static bool ApplyChangesFromTC25ThroughTC30(....)
{
....
Dictionary logicalLocationToIndexMap = null;
....
logicalLocationToIndexMap =
new Dictionary(LogicalLocation.ValueComparer);
run["logicalLocations"] =
ConvertLogicalLocationsDictionaryToArray(
....,
logicalLocationToIndexMap,
....);
}
```
Странный и подозрительный код\.
[V3008](https://pvs-studio.ru/ru/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(....); // <=
....
}
```
Свойству _run\.Tool_ дважды присваивают значение\. Как при создании объекта _Tool_, так и при записи в свойство _Tool_, никакой дополнительной работы не производится\. Поэтому повторное присваивание выглядит подозрительно\.
[V3042](https://pvs-studio.ru/ru/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;
}
```
В случае, если значение переменной _loc_ окажется равным _null_, будет произведена попытка возврата значения из правой части оператора ??, что приведет к доступу по нулевой ссылке\.
[V3042](https://pvs-studio.ru/ru/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;
....
}
```
В двух параллельных ветках условного оператора ?: используют небезопасный и безопасный варианты доступа по потенциально нулевой ссылке _formatString_\.
[V3042](https://pvs-studio.ru/ru/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.ru/ru/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(....);
}
....
}
```
Здесь анализатор выдал сразу два предупреждения о возможном доступе по нулевой ссылке _messageText_\. Выглядит тривиально, но ошибка от этого не перестаёт быть ошибкой\.
[V3080](https://pvs-studio.ru/ru/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))
{
....
}
....
}
```
Анализатор заподозрил, что возможен _NullReferenceException_ при работе со ссылкой _fileDataVersionOne\.Uri_\. Посмотрим, откуда приходит эта переменная, и прав ли анализатор\. Для этого изучим тело метода _CreateFileDataVersionOne_:
```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; }
....
}
```
Действительно, при создании объекта класса _FileDataVersionOne_ свойство _Uri_ может получить значение _null_\. Хороший пример совместной работы механизмов анализа потока данных и межпроцедурного анализа\.
[V3080](https://pvs-studio.ru/ru/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(); } // <=
}
....
}
```
Здесь допущена опечатка\. Очевидно, что в условии второго блока _if_ должно быть _\_jsonTextWriter \!\= null_\. Опасность данного кода в том, что он, скорее всего, не падает, потому что _\_jsonTextWriter_ как раз не равен _null_\. Но при этом и поток остается открытым\.
[V3083](https://pvs-studio.ru/ru/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(....);
}
....
}
```
С событием работают небезопасно\. Некритичная ошибка, которую легко исправить, например, следуя подсказке Visual Studio\. Вот такую замену предлагает IDE:
```cpp
private void ReadRule(....)
{
....
RuleRead?.Invoke(....);
....
}
```
Работы на несколько секунд, зато анализатор больше не ругается, а IDE не подсвечивает код\. Ещё одна подобная ошибка:
* 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.ru/ru/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)
{
....
}
....
}
```
Автор кода предполагал, что ссылка _v1Location_ может быть нулевой, поэтому добавил соответствующую проверку\. При этом в коде чуть выше он почему\-то работает с данной ссылкой безо всяких проверок\. Невнимательность при рефакторинге? Трудно сказать\.
[V3125](https://pvs-studio.ru/ru/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;
}
```
Традиционно – обратная ситуация\. Сначала ссылку _v1StackFrame_ проверяют на равенство _null_, а затем о проверке забыли\. Но здесь есть один важный нюанс: переменные _v1StackFrame_ и _stackFrame_ логически связаны\. Смотрите, если _v1StackFrame_ будет \= _null_, то объект _StackFrame_ не будет создан, а _stackFrame_ так и останется \= _null_\. При этом программа упадет на вызове _stackFrame\.Location_, так как никаких проверок тут нет\. То есть до опасного использования _v1StackFrame_, на которое указал анализатор, дело даже не дойдет\. Этот код работоспособен только в случае передачи в метод _CreateStackFrame_ ненулевых значений _v1StackFrame_\. Я заподозрил, что в вызывающем коде это как\-то контролируется\. Вызовы _CreateStackFrame_ выглядят так:
```cpp
Frames = v1Stack.Frames?.Select(CreateStackFrame).ToList()
```
_CreateStackFrame_ используют как селектор\. Проверки передаваемых ссылок на равенство _null_ здесь нет\. Возможно, где\-то при заполнении коллекции _Frames_ это \(запись нулевых ссылок\) контролируется, но так далеко копать я не стал\. Вывод и так очевиден \- код требует внимания авторов\.
## Заключение
Статья получилась небольшая, зато никто не успел заскучать :\) Напоминаю, что вы всегда можете [скачать](https://pvs-studio.ru/ru/pvs-studio/download/) наш анализатор, чтобы самостоятельно поискать ошибки в своих или чужих проектах\.
И напоследок, небольшой анонс: следующая моя статья будет про самые интересные ошибки, которые я и мои коллеги нашли в C\# проектах в 2019 году\. Следите за нашим [блогом](https://pvs-studio.ru/ru/blog/posts/)\. До встречи\!
Чтобы узнать о новых публикациях в блоге, вы можете подписаться на следующие каналы:
* Twitter: [@pvsstudio\_rus](https://twitter.com/pvsstudio_rus/)
* VK\.com: [Анализатор PVS\-Studio](https://vk.com/pvsstudio_rus)
* Telegram: [@pvsstudio\_rus](https://t.me/pvsstudio_rus)
* Старый добрый RSS: [viva64\-blog\-ru](http://feeds.feedburner.com/viva64-blog-ru)