PVS-Studio and protobuf-net: best warnings are one click away

Dec 21 2022

Author: Artem Rovenskii

What are we talking about?
Demonstration of work
Results

Let's delve into best practice of getting acquainted with PVS-Studio. We'll show you a quick start of working with the analyzer using protobuf-net as an example.

1019_BestWarnings_Protobuf_net/image1.png

What are we talking about?

Getting acquainted with a static analyzer can be hard, especially on large projects. The analyzer's report may contain hundreds or even thousands of warnings. Reviewing them all may take a long time. That's when the Best Warnings mechanism may come in handy. With just one click, you'll see the most important warnings — those that most likely indicate an error.

Is it really one click? Yes, it is! Just click on the 'Best' button next to the warning levels in the PVS-Studio window. Then you'll see the best warnings according to the analyzer.

This is how the PVS-Studio 7.22 window looks like in Visual Studio 2022:

1019_BestWarnings_Protobuf_net/image2.png

When activated, the Best Warnings window looks like this:

1019_BestWarnings_Protobuf_net/image3.png

Starting with PVS-Studio 7.22, this mechanism is implemented in other IDEs:

Rider;
CLion;
IntelliJ IDEA.

Demonstration of work

It's easier to show the mechanism's abilities on an Open Source project. I chose protobuf-net to demonstrate how Best Warnings works. In simple terms, protobuf-net is a serializer that converts data into the Protocol Buffers format. For the analysis, I chose the latest version (3.1.4).

We must note that we won't show all warnings we got after filtering with Best Warnings. We'll show only the most interesting and clearly incorrect code fragments. You can try this mechanism in full on your project. The only thing you need is to request a trial key here.

By the way, we created an issue on GitHub for all these code fragments. The project developer replied to it and confirmed that all these errors should be fixed.

Now, let's finally click the button and look at the errors.

1019_BestWarnings_Protobuf_net/image4.png

We need more checks!

public virtual string GetName(FileDescriptorProto definition)
{
  var ns = definition?.Options?.GetOptions()?.Namespace;
  if (!string.IsNullOrWhiteSpace(ns)) return ns;
  ns = definition.Options?.CsharpNamespace;
  if (string.IsNullOrWhiteSpace(ns)) ns = GetName(definition.Package); // <=

  if (string.IsNullOrEmpty(ns)) ns = definition?.DefaultPackage;       // <=

  return string.IsNullOrWhiteSpace(ns) ? null : ns;
}

V3095 The 'definition' object was used before it was verified against null. Check lines: 139, 141. NameNormalizer.cs 139

In this fragment, the definition parameter is dereferenced and only after that it's checked for null. Note that the error with the absence of check will show itself even earlier.

Let's inspect.

If definition is null, then null will be written to the ns variable. After that, ns is checked for null with the IsNullOrWhiteSpace method. If ns is null, then the method won't exit, and NullReferenceException will be thrown in the next line.

To be honest, such code formatting is strange and hard to read. Perhaps, this is why there's an error in the method.

null? Use it!

internal static MethodInfo GetGetMethod(....)
{
  if (property is null) return null;
  MethodInfo method = property.GetGetMethod(nonPublic);
  if (method is null && !nonPublic && allowInternal)
  {
    method = property.GetGetMethod(true);
    if (method is null && !(   method.IsAssembly           // <=
                            || method.IsFamilyOrAssembly)) 
    {
      method = null;
    }
  }
  return method;
}

V3080 Possible null dereference. Consider inspecting 'method'. Helpers.cs 74

In this fragment, an error is in the logic of the if statement's condition. It looks like the developer checks the value of the method local variable for null, and then accesses it. As a result, it may lead to NullReferenceException.

Let's look at the name of the method. The name contains the Get word. So, we can assume that there's a method with Set somewhere. The GetSetMethod really exists. Moreover, this method has the same error. By the way, the warning about this one is also in Best Warnings. Eh, copy-paste.

Check before use

public EnumMemberSerializer(Type enumType)
{
  if (!enumType.IsEnum)                                                   // <=
    ThrowHelper.ThrowInvalidOperationException("...." +
                                               enumType.NormalizeName()); 
  ExpectedType = enumType ??                                              // <= 
                 throw new ArgumentNullException(nameof(enumType)); 
  _tail = Type.GetTypeCode(enumType) switch
  {
    ....
  };
  if (_tail is null)
    ThrowHelper.ThrowInvalidOperationException("...." + 
                                               enumType.NormalizeName());
}

V3095 The 'enumType' object was used before it was verified against null. Check lines: 13, 14. EnumMemberSerializer.cs 13

Should we check the parameter for null? Maybe, but certainly not after it's already been used :). In this case, the developer wanted to throw an exception if enumType is null. The exception will be thrown — but not what the developer expected.

Results

Seems like getting acquainted with a static analyzer can be pleasant :). At the same time, you can find errors without viewing the analyzer's full report. It is very important for users to evaluate the product in terms of user experience. That's why we made the Best Warnings mechanism that filters warnings in one click.

Please remember: this mechanism is designed primarily for quick and easy introduction to the analyzer's capabilities. Best Warnings is not a substitute for working with a full report. Thank you for reading, we hope you'd enjoy PVS-Studio :).

#CSharp