Home .NET Nullable Referencedoes not protect, and here is the proof

Nullable Referencedoes not protect, and here is the proof

by admin

Nullable Referencedoes not protect, and here is the proof
Have you ever wanted to get rid of the nullreference dereferencing problem? If so, then using Nullable Reference types is not your choice.Wondering why? That’s what we’re going to talk about today.
We warned, and it happened. About a year ago, my colleagues wrote article in which they warned that the introduction of Nullable Reference types would not protect against nullreference dereferences. Now we have a real confirmation of our words, which was found in the depths of Roslyn.

Nullable Reference types

The very idea of adding Nullable Reference (hereinafter NR) types seem to me to be interesting, because the problem with nullreferences dereferencing is still relevant today. The implementation of protection against dereferencing turned out to be very unreliable. The idea of the creators was to take the value null only those variables whose type is marked with "?" symbol. For example, a variable of type string? tells you that it can contain null such as string – vice versa.
However, no one forbids us from transmitting null into variables non-nullable reference (hereinafter – NNR) types, because they are not implemented at the level of IL code. The built-in static analyzer in the compiler is responsible for this limitation. That is why this innovation is more of a recommendation character. Here is a simple example that shows how it works :

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

As we can see, the type at nonNullable is specified as NNR, but we can still pass there null Of course, we will get a conversion warning "Converting nullliteral or possible nullvalue to non-nullable type.". However, we can get around this by adding some aggression :

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

One exclamation point, and there are no warnings. If any of you are foodies, another option is available :

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

One more example. We create two simple console projects. In the first one we write :

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

In the second one we write :

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

Pointing at nullOrNotNull and you will see this message :
Nullable Referencedoes not protect, and here is the proof
We are prompted that the string here cannot be null But we understand that it will be exactly null Now we launch the project and get an exception :
Nullable Referencedoes not protect, and here is the proof
Of course, these are just synthetic examples, the purpose of which is to show that this introduction in no way guarantees you protection against nulllink dereferencing. If you thought synthetics were boring, and where are the real examples anyway, please don’t bother, it’s all coming up next.
NR types have another problem – it is not clear whether they are enabled or not. For example, the solution has two projects. One is marked up with this syntax and the other is not. Going into a project with NR types, you may think that if one is marked up, all of them are marked up. But that won’t be the case. It turns out that you have to check each time whether the project or file has a nullable context enabled. Otherwise you can mistakenly think that the usual reference type is NNR.

How the evidence was found

When developing new diagnostics in the PVS-Studio analyzer, we always test them on our base of real projects. This helps in various aspects. For example :

  • to look "live" at the quality of the warnings received;
  • get rid of some of the false positives;
  • find interesting points in the code, which you can then talk about;
  • and so on.

One of the new V3156 diagnostics – found places where, because of a potential null can result in exceptions. The diagnostic rule’s formulation sounds like this:"The argument of the method is not expected to be null". Its essence is that a method that is not expected to be null can take as its argument the value null This can lead, for example, to an exception or incorrect execution of the called method. You can read more about this diagnostic rule here

Proof here

So, we have come to the main part of this article. Here we will show you real code fragments from the Roslyn project for which the diagnostic generated warnings. Their main sense is that either the NNR type is passed null or there is no check of the NR type’s value. All this can lead to an exception.
Example 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
The message states that key is a potential null Let’s see where this variable can get such a value. Let’s first check the method 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();

Since s_nullKey is not null , see what it returns constantValue.Value :

public object? Value{get{switch (this.Discriminator){caseConstantValueTypeDiscriminator.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);}}}

There are two null literals here, but in this case we will not go into either case with them. This is because of the checks IsBad and IsNull However, I would like to draw your attention to the return type of this property. It is an NR type, but the method KeyForConstant already returns an NNR type. It turns out that in the general case to return null method KeyForConstant can.
Another source that can return null, – method AsNode :

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

Again, please note the return type of the method is NR type. It turns out that when we say that from a method can return null , it doesn’t affect anything. The interesting thing is that the compiler here doesn’t swear at the conversion from NR to NNR:
Nullable Referencedoes not protect, and here is the proof
Example 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
Another example with the function AsNode which was described above. Only this time oldNode will be of NR type. Whereas the above described key had an NNR type.
By the way, I cannot help sharing an interesting observation with you. As I have already described above, we test it on various projects when developing diagnostics. I noticed an interesting thing when checking responses of this rule. About 70% of all the warnings were generated for methods of the Dictionary Most of them were issued for the method TryGetValue This might be because subconsciously we don’t expect exceptions from a method that contains the word try So check your code for this pattern, in case you find something similar.
Example 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
The analyzer says the problem is typeName Let’s first make sure that this argument is really a potential null Looking at ReadString :

public string ReadString() => ReadStringValue();

So, look ReadStringValue :

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

Great, now let’s refresh our memory by looking at where our variable was passed to :

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

I think it’s time to go inside the method Add :

public bool Add(K k, V v){ValueSet updated;if (_dictionary.TryGetValue(k, out ValueSet set)) // <={....}....}

Indeed, if the method Add pass in the first argument null then we get an exception ArgumentNullException
By the way, it is interesting that if in Visual Studio hover the cursor over typeName , we see that its type is string? :
Nullable Referencedoes not protect, and here is the proof
The return type of the method is simply string :
Nullable Referencedoes not protect, and here is the proof
If you then create a variable of the NNR type and assign to it typeName typeName, no error will be shown.

Let’s try to drop Roslyn

Not for spite, but for fun, I suggest you try to reproduce one of the examples shown.
Nullable Referencedoes not protect, and here is the proof
Test 1
Let’s take the example described under number 3:

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

In order to reproduce it, you will need to call the TryReadSymbolTreeInfo but it is private The good thing about the class has a method with it ReadSymbolTreeInfo_ForTestingPurposesOnly which is already 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)))));}

It is very nice that we are directly invited to test the method TryReadSymbolTreeInfo So let’s create our own class and write the following code :

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

Now let’s assemble Roslyn , create a simple console application, connect all the necessary dll-files and write this code :

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

Run, get to the desired location, and see :
Nullable Referencedoes not protect, and here is the proof
Next, we enter the method Add and get the expected exception :
Nullable Referencedoes not protect, and here is the proof
Recall that the method ReadString returns the NNR type, which is not supposed to contain null This example once again confirms the relevance of PVS-Studio’s diagnostic rules for searching for nullreference dereferences.
Test 2
Now that we’ve already started reproducing examples, why not reproduce another one. This example will not be related to NR types. But the same V3156 diagnostic found it and I wanted to tell you about it. Here is the code :

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 nullvalue: null. AbstractSemanticFactsService.cs 24
I’ll be honest: doing this diagnostic, I wasn’t expecting much of a straightforward response null After all, it is strange enough to send null to a method that will throw an exception because of it. Although I’ve seen places where this has been justified (e.g., with the class Expression ), but that’s not the point right now.
So I was very intrigued when I saw this warning. Let’s take a look at what’s going on in the method 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)); // <=}

We see that there is only one way out of the method, no exceptions are thrown, and goto is not. In other words, nothing prevents you from passing usedNames to the method Concat and get an exception ArgumentNullException
But that’s all words, let’s do it. To do that, let’s look at where this method can be called from. The method itself is in the class AbstractSemanticFactsService The class is abstract, so for convenience we take the class CSharpSemanticFactsService which is inherited from it. In the file of this class, let’s create our own one, which will call the GenerateUniqueName It looks like this :

public class DropRoslyn{private const string ProgramText =@"using System;using System.Collections.Generic;using System.Textnamespace 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);}}

Now build Roslyn, create a simple console application, plug in all the necessary dll-files and write this code :

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

Run the application and get the following :
Nullable Referencedoes not protect, and here is the proof

This is misleading

Suppose we agree with the nullable concept. It turns out that if we see an NR type, we think that it may contain a potential null However, sometimes you can see situations where the compiler tells us otherwise. So here we will look at several cases where the use of this concept is not intuitive.
Case 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
Immediately see why bodyTokens is a potential null and we see null-conditional operator :

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

If you go into the method TryGetMethodDeclarationBody , we see that it can return null However, it is relatively large, so I leave the link to it if you want to see for yourself. With bodyTokens everything is clear, but I want to pay attention to the argument ctor :

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

As we see, its type is specified as NR. In this case the line below is dereferenced :

if (ctor.Initializer != null)

This combination is a bit alarming. However, you will say that it is likely that if IsKind returns true , then ctor is definitely not equal to null It is :

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

Special attributes are used here to specify at which output value the parameters will not equal null .We can also see this by looking at the logic of the IsKind It turns out that inside the condition the type at ctor must be NNR. The compiler understands this and says that ctor inside the condition will not be null However, to understand this we have to go into the method IsKind and notice the attribute there. Otherwise it looks like a dereferencing of the NR variable without checking for null You can try to add some clarity as follows :

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

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

V3156The first argument of the ‘LastIndexOf’ method is not expected to be null. Potential nullvalue: searchName. AbstractEditorInlineRenameService.SymbolRenameInfo.cs 126
We are interested in the variable searchName null can be written into it after calling the method GetWithoutAttributeSuffix but it’s not that simple. Let’s take a look at what’s going on in it :

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

Let’s go deeper :

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

It turns out that the method TryGetWithoutAttributeSuffix will return either result , or null Yes and the method returns NR type. However, going back one step, we notice that the method type has suddenly changed to NNR. This is because of the lurking sign "!":

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

By the way, it is quite difficult to notice it in Visual Studio :
Nullable Referencedoes not protect, and here is the proof
By putting it, the developer asserts to us that the method will never return null Although, looking at the previous examples and going into the method TryGetWithoutAttributeSuffix , I personally can’t be sure about it :

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

Output

In conclusion, I want to say that the attempt to save us from unnecessary checks on null, – is an excellent idea. However, NR types are more of a recommendation, because nobody strictly forbids us to pass null to the NNR type. That’s why the corresponding rules of PVS-Studio are still relevant. For example, such as V3080 or V3156
All the best to you and thank you for your attention.
Nullable Referencedoes not protect, and here is the proof

If you want to share this article with an English-speaking audience, please use the translation link : Nikolay Mironov. Nullable Reference will not protect you, and here is the proof

You may also like