Сергей Хренов

10 Дек 2019

Теги:

#CSharp

SARIF SDK и его ошибки

10 Дек 2019

Автор: Сергей Хренов

О проекте
Ошибки
Заключение

Сегодня у нас на тесте очередной качественный проект Microsoft, в котором мы всё же попытаемся героически поискать ошибки при помощи PVS-Studio. SARIF – аббревиатура от "Static Analysis Results Interchange Format", представляет собой стандарт (формат файла), предназначенный для взаимодействия и обмена результатами работы статических анализаторов с другими инструментами: IDE, комплексными инструментами проверки и анализа кода (например, SonarQube), системами непрерывной интеграции и т.п. SARIF SDK, соответственно, содержит инструментарий разработчика .NET для поддержки SARIF, а также вспомогательные файлы.

SARIF возник в Microsoft и теперь является стандартом, разрабатываемым в рамках OASIS (некоммерческий консорциум, который занимается открытыми стандартами). SARIF предназначен для передачи не только результатов работы анализатора, но и метаданных об инструменте, а также данных о том, как он был запущен, временных метках и так далее. Более подробно со стандартом можно ознакомиться на сайте OASIS. Исходный код SARIF SDK можно скачать с репозитория на GiHub. Домашняя страница проекта доступна по ссылке.

О проекте

Проект SARIF SDK оказался маленьким: 799 файлов .cs (примерно 98 000 непустых строк кода). В составе проекта содержатся тесты, которые я всегда исключаю из проверки. Таким образом, интересующая нас часть кода составила 642 файла .cs (примерно 79 000 непустых строк кода). Это, конечно, мало. Зато проверка и анализ прошли легко и быстро, между делом, что я и попытался отразить на картинке в начале статьи. Тем не менее, кое-какие интересные ошибки найти удалось. Давайте на них посмотрим.

Ошибки

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";
  ....
}

Поле Default инициализируют значением другого поля, которое еще не получило значения. В результате, Default получит значение по умолчанию для типа string – null. Вероятно, ошибку не заметили, потому что поле Default нигде не используют. Но всё может измениться, и тогда разработчик столкнётся с непредвиденным результатом или падением программы.

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);
  ....
}

Параметр logicalLocationToIndexMap никак не используют, записывая в него другое значение. Любопытно, но старое значение - точно такой же пустой словарь, который создают в вызывающем методе:

private static bool ApplyChangesFromTC25ThroughTC30(....)
{
  ....
  Dictionary<LogicalLocation, int> logicalLocationToIndexMap = null;
  ....
  logicalLocationToIndexMap =
    new Dictionary<LogicalLocation, int>(LogicalLocation.ValueComparer);

  run["logicalLocations"] =
    ConvertLogicalLocationsDictionaryToArray(
      ....,
      logicalLocationToIndexMap,
      ....);
}

Странный и подозрительный код.

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(....);  // <=
  ....
}

Свойству run.Tool дважды присваивают значение. Как при создании объекта Tool, так и при записи в свойство Tool, никакой дополнительной работы не производится. Поэтому повторное присваивание выглядит подозрительно.

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;
}

В случае, если значение переменной loc окажется равным null, будет произведена попытка возврата значения из правой части оператора ??, что приведет к доступу по нулевой ссылке.

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;
  ....
}

В двух параллельных ветках условного оператора ?: используют небезопасный и безопасный варианты доступа по потенциально нулевой ссылке formatString.

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(....);
  }
  ....
}

Здесь анализатор выдал сразу два предупреждения о возможном доступе по нулевой ссылке messageText. Выглядит тривиально, но ошибка от этого не перестаёт быть ошибкой.

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))
  {
    ....
  }
  ....
}

Анализатор заподозрил, что возможен NullReferenceException при работе со ссылкой fileDataVersionOne.Uri. Посмотрим, откуда приходит эта переменная, и прав ли анализатор. Для этого изучим тело метода CreateFileDataVersionOne:


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 [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(); }  // <=
  }
  ....
}

Здесь допущена опечатка. Очевидно, что в условии второго блока if должно быть _jsonTextWriter != null. Опасность данного кода в том, что он, скорее всего, не падает, потому что _jsonTextWriter как раз не равен null. Но при этом и поток остается открытым.

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(....);
  }
  ....
}

С событием работают небезопасно. Некритичная ошибка, которую легко исправить, например, следуя подсказке Visual Studio. Вот такую замену предлагает IDE:

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 [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)
  {
    ....
  }
  ....
}

Автор кода предполагал, что ссылка v1Location может быть нулевой, поэтому добавил соответствующую проверку. При этом в коде чуть выше он почему-то работает с данной ссылкой безо всяких проверок. Невнимательность при рефакторинге? Трудно сказать.

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;
}

Традиционно – обратная ситуация. Сначала ссылку v1StackFrame проверяют на равенство null, а затем о проверке забыли. Но здесь есть один важный нюанс: переменные v1StackFrame и stackFrame логически связаны. Смотрите, если v1StackFrame будет = null, то объект StackFrame не будет создан, а stackFrame так и останется = null. При этом программа упадет на вызове stackFrame.Location, так как никаких проверок тут нет. То есть до опасного использования v1StackFrame, на которое указал анализатор, дело даже не дойдет. Этот код работоспособен только в случае передачи в метод CreateStackFrame ненулевых значений v1StackFrame. Я заподозрил, что в вызывающем коде это как-то контролируется. Вызовы CreateStackFrame выглядят так:

Frames = v1Stack.Frames?.Select(CreateStackFrame).ToList()

CreateStackFrame используют как селектор. Проверки передаваемых ссылок на равенство null здесь нет. Возможно, где-то при заполнении коллекции Frames это (запись нулевых ссылок) контролируется, но так далеко копать я не стал. Вывод и так очевиден - код требует внимания авторов.

Заключение

Статья получилась небольшая, зато никто не успел заскучать :) Напоминаю, что вы всегда можете скачать наш анализатор, чтобы самостоятельно поискать ошибки в своих или чужих проектах.

И напоследок, небольшой анонс: следующая моя статья будет про самые интересные ошибки, которые я и мои коллеги нашли в C# проектах в 2019 году. Следите за нашим блогом. До встречи!

Чтобы узнать о новых публикациях в блоге, вы можете подписаться на следующие каналы: