Николай Миронов

06 Окт 2020

Теги:

#CSharp #Knowledge #Compiler

Nullable Reference не защищают, и вот доказательства

06 Окт 2020

Автор: Николай Миронов

Nullable Reference типы
Как были найдены доказательства
Пруфы тут
Попробуем уронить Roslyn
Это вводит в заблуждение
Вывод

Хотели ли вы когда-нибудь избавиться от проблемы с разыменованием нулевых ссылок? Если да, то использование Nullable Reference типов — это не ваш выбор. Интересно почему? Об этом сегодня и пойдёт речь.

0764_Nullable_Reference_And_Roslyn_ru/image1.png

Мы предупреждали, и это случилось. Около года назад мои коллеги написали статью, в которой предупреждали о том, что введение Nullable Reference типов не защитит от разыменований нулевых ссылок. Теперь у нас есть реальное подтверждение наших слов, которое было найдено в глубинах Roslyn.

Nullable Reference типы

Сама задумка добавить Nullable Reference (далее - NR) типы мне кажется интересной, так как проблема, связанная с разыменованием нулевых ссылок, актуальна и по сей день. Реализация же защиты от разыменований получилась крайне ненадежной. По задумке создателей принимать значение null могут только те переменные, тип которых помечен символом "?". Например, переменная типа string? говорит о том, что в ней может содержаться null, типа string - наоборот.

Однако нам все равно никто не запрещает передавать null в переменные non-nullable reference (далее - NNR) типов, ведь реализованы они не на уровне IL кода. За данное ограничение отвечает встроенный в компилятор статический анализатор. Поэтому данное нововведение скорее носит рекомендательный характер. Вот простой пример, показывающий, как это работает:

#nullable enable
object? nullable = null;
object nonNullable = nullable;
var deref = nonNullable.ToString();

Как мы видим, тип у nonNullable указан как NNR, но при этом мы спокойно можем передать туда null. Конечно, мы получим предупреждение о конвертации "Converting null literal or possible null value to non-nullable type.". Однако это можно обойти, добавив немного агрессии:

#nullable enable
object? nullable = null;
object nonNullable = nullable!; // <=
var deref = nonNullable.ToString();

Один восклицательный знак, и нет никаких предупреждений. Если кто-то из вас гурман, то доступен еще такой вариант:

#nullable enable
object nonNullable = null!;
var deref = nonNullable.ToString();

Ну и еще один пример. Создаем два простых консольных проекта. В первом пишем:

namespace NullableTests
{
    public static class Tester
    {
        public static string RetNull() => null;
    }
}

Во втором пишем:

#nullable enable 

namespace ConsoleApp1
{
    class Program
    {
        static void Main(string[] args)
        {
            string? nullOrNotNull = NullableTests.Tester.RetNull();
            System.Console.WriteLine(nullOrNotNull.Length);
        }
    }
}

Наводим на nullOrNotNull и видим вот такое сообщение:

0764_Nullable_Reference_And_Roslyn_ru/image2.png

Нам подсказывают, что строка здесь не может быть null. Однако мы то понимаем, что она здесь как раз будет null. Запускаем проект и получаем исключение:

0764_Nullable_Reference_And_Roslyn_ru/image3.png

Конечно, это всего лишь синтетические примеры, цель которых показать, что данное введение никак не гарантирует вам защиту от разыменования нулевой ссылки. Если вы посчитали, что синтетика — это скучно, и где вообще реальные примеры, то попрошу не беспокоиться, далее все это будет.

У NR типов есть еще одна проблема – непонятно, включены они или нет. Например, в решении имеется два проекта. Один размечен с помощью данного синтаксиса, а другой - нет. Зайдя в проект с NR типами, можно решить, что раз размечен один, то размечены все. Однако это будет не так. Получается, нужно каждый раз смотреть, а включен ли в проекте или файле nullable context. Иначе же можно по ошибке решить, что обычный reference тип – это NNR.

Как были найдены доказательства

При разработке новых диагностик в анализаторе PVS-Studio мы всегда тестируем их на нашей базе реальных проектов. Это помогает в различных аспектах. Например:

посмотреть "вживую" на качество полученных предупреждений;
избавиться от части ложных срабатываний;
найти интересные моменты в коде, о которых потом можно рассказать;
и так далее.

Одна из новых диагностик V3156 - нашла места, в которых из-за потенциального null могут возникать исключения. Формулировка диагностического правила звучит так: "The argument of the method is not expected to be null". Суть ее в том, что в метод, не ожидающий null, в качестве аргумента может передаваться значение null. Это может привести, например, к возникновению исключения или неправильному исполнению вызываемого метода. Подробнее о данном диагностическом правиле вы можете почитать здесь.

Пруфы тут

Вот мы и дошли до основной части данной статьи. Тут будут показаны реальные фрагменты кода из проекта Roslyn, на которые диагностика выдала предупреждения. Основной их смысл в том, что либо в NNR тип передается null, либо отсутствует проверка значения NR типа. Все это может привести к возникновению исключения.

Пример 1

private static Dictionary<object, SourceLabelSymbol>
BuildLabelsByValue(ImmutableArray<LabelSymbol> labels)
{
  ....
  object key;
  var constantValue = label.SwitchCaseLabelConstant;
  if ((object)constantValue != null && !constantValue.IsBad)
  {
    key = KeyForConstant(constantValue);
  }
  else if (labelKind == SyntaxKind.DefaultSwitchLabel)
  {
    key = s_defaultKey;
  }
  else
  {
    key = label.IdentifierNodeOrToken.AsNode();
  }

  if (!map.ContainsKey(key))                // <=
  {
    map.Add(key, label);
  } 
  ....
}

V3156 The first argument of the 'ContainsKey' method is not expected to be null. Potential null value: key. SwitchBinder.cs 121

Сообщение гласит, что key является потенциальным null. Давайте посмотрим, где эта переменная может получить такое значение. Проверим сначала метод KeyForConstant:

protected static object KeyForConstant(ConstantValue constantValue)
{
  Debug.Assert((object)constantValue != null);
  return constantValue.IsNull ? s_nullKey : constantValue.Value;
}
private static readonly object s_nullKey = new object();

Так как s_nullKey не являетcя null, смотрим, что возвращает constantValue.Value:

public object? Value
{
  get
  {
    switch (this.Discriminator)
    {
      case ConstantValueTypeDiscriminator.Bad: return null;  // <=
      case ConstantValueTypeDiscriminator.Null: return null; // <=
      case ConstantValueTypeDiscriminator.SByte: return Boxes.Box(SByteValue);
      case ConstantValueTypeDiscriminator.Byte: return Boxes.Box(ByteValue);
      case ConstantValueTypeDiscriminator.Int16: return Boxes.Box(Int16Value);
      ....
      default: throw ExceptionUtilities.UnexpectedValue(this.Discriminator);
    }
  }
}

Здесь есть два нулевых литерала, но в данном случае мы не зайдем ни в один case с ними. Это происходит из-за проверок IsBad и IsNull. Однако я бы хотел обратить ваше внимание на возвращаемый тип данного свойства. Он является NR типом, но при этом метод KeyForConstant уже возвращает NNR тип. Получается, что в общем случае вернуть null метод KeyForConstant может.

Другой источник, который может вернуть null, - метод AsNode:

public SyntaxNode? AsNode()
{
  if (_token != null)
  {
    return null;
  }

  return _nodeOrParent;
}

Снова прошу обратить внимание на возвращаемый тип метода — это NR тип. Получается, когда мы говорим, что из метода может вернуться null, то это ни на что не влияет. Интересно то, что компилятор здесь не ругается на преобразование из NR в NNR:

0764_Nullable_Reference_And_Roslyn_ru/image4.png

Пример 2

private SyntaxNode CopyAnnotationsTo(SyntaxNode sourceTreeRoot, 
                                     SyntaxNode destTreeRoot)
{  
  var nodeOrTokenMap = new Dictionary<SyntaxNodeOrToken, 
                                      SyntaxNodeOrToken>();
  ....
  if (sourceTreeNodeOrTokenEnumerator.Current.IsNode)
  {
    var oldNode = destTreeNodeOrTokenEnumerator.Current.AsNode();
    var newNode = sourceTreeNodeOrTokenEnumerator.Current.AsNode()
                                       .CopyAnnotationsTo(oldNode);
        
    nodeOrTokenMap.Add(oldNode, newNode); // <=
  }
  ....
}

V3156 The first argument of the 'Add' method is not expected to be null. Potential null value: oldNode. SyntaxAnnotationTests.cs 439

Еще один пример с функцией AsNode, которая была описана выше. Только в этот раз oldNode будет иметь NR тип. В то время как описанная выше key имела NNR тип.

Кстати, не могу не поделиться с вами интересным наблюдением. Как я уже описал выше, при разработке диагностики мы проверяем ее на разных проектах. При проверке срабатываний данного правила был замечен любопытный момент. Около 70% всех срабатываний было выдано на методы класса Dictionary. При этом большая их часть пришлась на метод TryGetValue. Возможно, это происходит из-за того, что подсознательно мы не ожидаем исключений от метода, который содержит слово try. Поэтому проверьте свой код на этот паттерн, вдруг у вас найдется что-то подобное.

Пример 3

private static SymbolTreeInfo TryReadSymbolTreeInfo(
    ObjectReader reader,
    Checksum checksum,
    Func<string, ImmutableArray<Node>, 
    Task<SpellChecker>> createSpellCheckerTask)
{
  ....
  var typeName = reader.ReadString();
  var valueCount = reader.ReadInt32();

  for (var j = 0; j < valueCount; j++)
  {
    var containerName = reader.ReadString();
    var name = reader.ReadString();

    simpleTypeNameToExtensionMethodMap.Add(typeName, // <=
                            new ExtensionMethodInfo(containerName, name)); 
  }
  ....
}

V3156 The first argument of the 'Add' method is passed as an argument to the 'TryGetValue' method and is not expected to be null. Potential null value: typeName. SymbolTreeInfo_Serialization.cs 255

Анализатор говорит, что проблема заключается в typeName. Давайте сначала убедимся, что этот аргумент действительно является потенциальным null. Смотрим на ReadString:

public string ReadString() => ReadStringValue();

Так, смотрим ReadStringValue:


private string ReadStringValue()
{
  var kind = (EncodingKind)_reader.ReadByte();
  return kind == EncodingKind.Null ? null : ReadStringValue(kind);
}

Отлично, теперь освежим память, посмотрев, куда же передавалась наша переменная:

simpleTypeNameToExtensionMethodMap.Add(typeName, // <=
                              new ExtensionMethodInfo(containerName,
                                                      name));

Думаю, самое время зайти внутрь метода Add:

public bool Add(K k, V v)
{
  ValueSet updated;

  if (_dictionary.TryGetValue(k, out ValueSet set)) // <=
  {
    ....
  }
  ....
}

Действительно, если в метод Add в качестве первого аргумента передать null, то мы получим исключение ArgumentNullException.

Кстати, интересно, что если в Visual Studio навести курсор на typeName, то мы увидим, что тип у него string?:

0764_Nullable_Reference_And_Roslyn_ru/image5.png

При этом возвращаемый тип метода - просто string:

0764_Nullable_Reference_And_Roslyn_ru/image6.png

При этом, если далее создать переменную NNR типа и ей присвоить typeName, то никакой ошибки не будет выведено.

Попробуем уронить Roslyn

Не злобы для, а забавы ради предлагаю попробовать воспроизвести один из показанных примеров.

0764_Nullable_Reference_And_Roslyn_ru/image7.png

Тест 1

Возьмем пример, описанный под номером 3:

private static SymbolTreeInfo TryReadSymbolTreeInfo(
    ObjectReader reader,
    Checksum checksum,
    Func<string, ImmutableArray<Node>, 
    Task<SpellChecker>> createSpellCheckerTask)
{
  ....
  var typeName = reader.ReadString();
  var valueCount = reader.ReadInt32();

  for (var j = 0; j < valueCount; j++)
  {
    var containerName = reader.ReadString();
    var name = reader.ReadString();

    simpleTypeNameToExtensionMethodMap.Add(typeName, // <=
                            new ExtensionMethodInfo(containerName, name)); 
  }
  ....
}

Для того, чтобы его воспроизвести, потребуется вызвать метод TryReadSymbolTreeInfo, но он является private. Хорошо, что в классе с ним есть метод ReadSymbolTreeInfo_ForTestingPurposesOnly, который уже является internal:

internal static SymbolTreeInfo ReadSymbolTreeInfo_ForTestingPurposesOnly(
    ObjectReader reader, 
    Checksum checksum)
{
  return TryReadSymbolTreeInfo(reader, checksum,
          (names, nodes) => Task.FromResult(
            new SpellChecker(checksum, 
                             nodes.Select(n => new StringSlice(names, 
                                                               n.NameSpan)))));
}

Очень приятно, что нам прям предлагают протестировать метод TryReadSymbolTreeInfo. Поэтому давайте рядышком создадим свой класс и напишем следующий код:

public class CheckNNR
{
  public static void Start()
  {
    using var stream = new MemoryStream();
    using var writer = new BinaryWriter(stream);
    writer.Write((byte)170);
    writer.Write((byte)9);
    writer.Write((byte)0);
    writer.Write(0);
    writer.Write(0);
    writer.Write(1);
    writer.Write((byte)0);
    writer.Write(1);
    writer.Write((byte)0);
    writer.Write((byte)0);
    stream.Position = 0;

    using var reader = ObjectReader.TryGetReader(stream);
    var checksum = Checksum.Create("val");

    SymbolTreeInfo.ReadSymbolTreeInfo_ForTestingPurposesOnly(reader, checksum);
  }
}

Теперь собираем Roslyn, создаем простое консольное приложение, подключаем все необходимые dll-файлы и пишем вот такой код:

static void Main(string[] args)
{
  CheckNNR.Start();
}

Запускаем, доходим до необходимого места и видим:

0764_Nullable_Reference_And_Roslyn_ru/image8.png

Далее заходим в метод Add и получаем ожидаемое исключение:

0764_Nullable_Reference_And_Roslyn_ru/image9.png

Напомню, что метод ReadString возвращает NNR тип, который по задумке не может содержать null. Данный пример лишний раз подтверждает актуальность диагностических правил PVS-Studio для поиска разыменования нулевых ссылок.

Тест 2

Ну и раз мы уже начали воспроизводить примеры, то почему бы не воспроизвести еще один. Этот пример не будет связан с NR типами. Однако его нашла все та же диагностика V3156, и мне захотелось о нем рассказать. Вот код:

public SyntaxToken GenerateUniqueName(SemanticModel semanticModel, 
                                      SyntaxNode location, 
                                      SyntaxNode containerOpt, 
                                      string baseName, 
                                      CancellationToken cancellationToken)
{
  return GenerateUniqueName(semanticModel, 
                            location, 
                            containerOpt, 
                            baseName, 
                            filter: null, 
                            usedNames: null,    // <=
                            cancellationToken);
}

V3156 The sixth argument of the 'GenerateUniqueName' method is passed as an argument to the 'Concat' method and is not expected to be null. Potential null value: null. AbstractSemanticFactsService.cs 24

Скажу честно: делая данную диагностику, я не особо ожидал срабатываний на прямой null. Ведь достаточно странно отправлять null в метод, который из-за этого выбросит исключение. Хотя я видел места, когда это было обосновано (например, с классом Expression), но сейчас не об этом.

Поэтому я был очень заинтригован, когда увидел данное предупреждение. Давайте же посмотрим, что происходит в методе GenerateUniqueName.

public SyntaxToken GenerateUniqueName(SemanticModel semanticModel,
                                      SyntaxNode location, 
                                      SyntaxNode containerOpt,
                                      string baseName, 
                                      Func<ISymbol, bool> filter,
                                      IEnumerable<string> usedNames, 
                                      CancellationToken cancellationToken)
{
  var container = containerOpt ?? location
                       .AncestorsAndSelf()
                       .FirstOrDefault(a => SyntaxFacts.IsExecutableBlock(a) 
                                         || SyntaxFacts.IsMethodBody(a));

  var candidates = GetCollidableSymbols(semanticModel, 
                                        location, 
                                        container, 
                                        cancellationToken);

  var filteredCandidates = filter != null ? candidates.Where(filter) 
                                          : candidates;

  return GenerateUniqueName(baseName, 
                            filteredCandidates.Select(s => s.Name)
                                              .Concat(usedNames));     // <=
}

Мы видим, что из метода только один выход, исключения не выбрасываются, да и goto нет. Иными словами, ничего не мешает передать usedNames в метод Concat и получить исключение ArgumentNullException.

Но это все слова, давайте же сделаем это. Для этого смотрим, откуда можно вызвать данный метод. Сам метод находится в классе AbstractSemanticFactsService. Класс является абстрактным, поэтому для удобства возьмем класс CSharpSemanticFactsService, который от него наследуется. В файле этого класса создадим свой, который и будет вызывать метод GenerateUniqueName. Выглядит это следующим образом:

public class DropRoslyn
{
  private const string ProgramText = 
    @"using System;
    using System.Collections.Generic;
    using System.Text
    namespace HelloWorld
    {
      class Program
      {
        static void Main(string[] args)
        {
          Console.WriteLine(""Hello, World!"");
        }
      }
    }";
  
  public void Drop()
  {
    var tree = CSharpSyntaxTree.ParseText(ProgramText);
    var instance = CSharpSemanticFactsService.Instance;
    var compilation = CSharpCompilation
                      .Create("Hello World")
                      .AddReferences(MetadataReference
                                     .CreateFromFile(typeof(string)
                                                     .Assembly
                                                     .Location))
                      .AddSyntaxTrees(tree);
    
    var semanticModel = compilation.GetSemanticModel(tree);
    var syntaxNode1 = tree.GetRoot();
    var syntaxNode2 = tree.GetRoot();
    
    var baseName = "baseName";
    var cancellationToken = new CancellationToken();
    
    instance.GenerateUniqueName(semanticModel, 
                                syntaxNode1, 
                                syntaxNode2, 
                                baseName, 
                                cancellationToken);
  }
}

Теперь собираем Roslyn, создаем простое консольное приложение, подключаем все необходимые dll-файлы и пишем вот такой код:

class Program
{
  static void Main(string[] args)
  {
    DropRoslyn dropRoslyn = new DropRoslyn();
    dropRoslyn.Drop();
  }
}

Запускаем приложение и получаем следующее:

0764_Nullable_Reference_And_Roslyn_ru/image10.png

Это вводит в заблуждение

Допустим мы соглашаемся с nullable концепцией. Получается, если мы видим NR тип, то считаем, что в нем может находиться потенциальный null. Однако иногда можно увидеть ситуации, когда компилятор нам говорит обратное. Поэтому здесь будет рассмотрено несколько случаев, когда использование данной концепции не является интуитивно понятным.

Случай 1

internal override IEnumerable<SyntaxToken>? TryGetActiveTokens(SyntaxNode node)
{
  ....
  var bodyTokens = SyntaxUtilities
                   .TryGetMethodDeclarationBody(node)
                   ?.DescendantTokens();

  if (node.IsKind(SyntaxKind.ConstructorDeclaration, 
                  out ConstructorDeclarationSyntax? ctor))
  {
    if (ctor.Initializer != null)
    {
      bodyTokens = ctor.Initializer
                       .DescendantTokens()
                       .Concat(bodyTokens); // <=
    }
  }
  return bodyTokens;
}

V3156 The first argument of the 'Concat' method is not expected to be null. Potential null value: bodyTokens. CSharpEditAndContinueAnalyzer.cs 219

Сразу смотрим, почему bodyTokens является потенциальным null и видим null-условный оператор:

var bodyTokens = SyntaxUtilities
                 .TryGetMethodDeclarationBody(node)
                 ?.DescendantTokens();              // <=

Если зайти в метод TryGetMethodDeclarationBody, то мы увидим, что он может вернуть null. Однако он относительно большой, поэтому я оставляю ссылку на него, если вы захотите лично убедиться в этом. С bodyTokens все понятно, но я хочу обратить внимание на аргумент ctor:

if (node.IsKind(SyntaxKind.ConstructorDeclaration, 
                out ConstructorDeclarationSyntax? ctor))

Как мы видим, его тип задан как NR. При этом строчкой ниже происходит разыменование:

if (ctor.Initializer != null)

Такое сочетание немного настораживает. Впрочем, вы скажете, что, скорее всего, если IsKind возвращает true, то ctor точно не равен null. Так оно и есть:

public static bool IsKind<TNode>(
    [NotNullWhen(returnValue: true)] this SyntaxNode? node, // <=
    SyntaxKind kind,
    [NotNullWhen(returnValue: true)] out TNode? result)     // <=
    where TNode : SyntaxNode 
{
  if (node.IsKind(kind))
  {
    result = (TNode)node;
    return true;
  }

  result = null;
  return false;
}

Тут используются специальные атрибуты, которые указывают, при каком выходном значении параметры не будут равны null. В этом же мы можем убедиться, посмотрев на логику метода IsKind. Получается, что внутри условия тип у ctor должен быть NNR. Компилятор это понимает и говорит, что ctor внутри условия будет не null. Однако, чтобы это понять нам, мы должны зайти в метод IsKind и там заметить атрибут. Иначе же это выглядит как разыменование NR переменной без проверки на null. Можно попробовать добавить немного наглядности следующим образом:

if (node.IsKind(SyntaxKind.ConstructorDeclaration, 
                out ConstructorDeclarationSyntax? ctor))
{
    if (ctor!.Initializer != null) // <=
    {
      ....
    }
}

Случай 2

public TextSpan GetReferenceEditSpan(InlineRenameLocation location, 
                                     string triggerText, 
                                     CancellationToken cancellationToken)
{
  var searchName = this.RenameSymbol.Name;
  if (_isRenamingAttributePrefix)
  {
    searchName = GetWithoutAttributeSuffix(this.RenameSymbol.Name);
  }

  var index = triggerText.LastIndexOf(searchName,            // <=
                                      StringComparison.Ordinal);
  ....
}

V3156 The first argument of the 'LastIndexOf' method is not expected to be null. Potential null value: searchName. AbstractEditorInlineRenameService.SymbolRenameInfo.cs 126

Нас интересует переменная searchName. null может быть записан в неё после вызова метода GetWithoutAttributeSuffix, но не все так просто. Давайте посмотрим, что же в нем происходит:

private string GetWithoutAttributeSuffix(string value)
    => value.GetWithoutAttributeSuffix(isCaseSensitive:
                _document.GetRequiredLanguageService<ISyntaxFactsService>()
                         .IsCaseSensitive)!;

Давайте зайдем глубже:

internal static string? GetWithoutAttributeSuffix(
            this string name,
            bool isCaseSensitive)
{
  return TryGetWithoutAttributeSuffix(name, isCaseSensitive, out var result) 
         ? result : null;
}

Получается, метод TryGetWithoutAttributeSuffix, вернет либо result, либо null. Да и метод возвращает NR тип. Однако, вернувшись на шаг назад, мы заметим, что тип метода неожиданно поменялся на NNR. Происходит это из-за притаившегося знака "!":

_document.GetRequiredLanguageService<ISyntaxFactsService>()
         .IsCaseSensitive)!; // <=

Кстати, заметить его в Visual Studio довольно сложно:

0764_Nullable_Reference_And_Roslyn_ru/image11.png

Поставив его, разработчик утверждает нам, что метод никогда не вернет null. Хотя, смотря на предыдущие примеры и зайдя в метод TryGetWithoutAttributeSuffix, лично я не могу быть в этом уверен:

internal static bool TryGetWithoutAttributeSuffix(
            this string name,
            bool isCaseSensitive,
            [NotNullWhen(returnValue: true)] out string? result)
{
  if (name.HasAttributeSuffix(isCaseSensitive))
  {
    result = name.Substring(0, name.Length - AttributeSuffix.Length);
    return true;
  }

  result = null;
  return false;
}

Вывод

В заключение я хочу сказать, что попытка избавить нас от лишних проверок на null, – это отличная идея. Однако NR типы носят скорее рекомендательный характер, ведь нам никто строго не запрещает передать null в NNR тип. Именно поэтому сохраняют актуальность соответствующие правила PVS-Studio. Например, такие как V3080 или V3156.

Всего вам доброго и спасибо за внимание.

#CSharp #Knowledge #Compiler