MassTransit – Open Source distributed application platform for .NET. In this article, we will talk about the problem areas in the project code. A static analyzer will help us find such places. Happy reading :).

Introduction

Since MassTransit provides a public API, the quality of the code for such a project is important. Unexpected behavior of this product can negatively affect not only the product itself, but also the programs that use it. This is one of the reasons we decided to check out MassTransit. The source code corresponding to version v8.0.15 was taken for analysis.

Let’s not bore you, let’s start analyzing the most interesting code fragments.

Suspicious condition

Issue 1

public async Task Send(OutboxConsumeContext<TMessage> context)
{
  ....

  if (   !context.ReceiveContext.IsDelivered       // <=
      && !context.ReceiveContext.IsDelivered)      // <=
  {
    await context.NotifyConsumed(context,
                                 timer.Elapsed,
                                 _options.ConsumerType)
                 .ConfigureAwait(false);
  }
  ....
}

Warning PVS-Studio: V3001 There are identical sub-expressions ‘! context.ReceiveContext. OutboxMessagePipe.cs 57

Pay attention to the condition if. It looks strange, because the value of the same quality is checked. Obviously, such a check makes no sense. However, if you look at the properties that can be accessed in the object context.ReceiveContextit can be found IsFaulted. Perhaps he should appear in one of the cases.

Issue 2, 3

public static IEnumerable<Assembly> FindAssemblies(....)
{
  var assemblyPath = AppDomain.CurrentDomain.BaseDirectory;
  var binPath = string.Empty;                                           // <=

  if (string.IsNullOrEmpty(binPath))                                    // <=
    return FindAssemblies(assemblyPath,loadFailure, includeExeFiles, filter);

  if (Path.IsPathRooted(binPath))
    return FindAssemblies(binPath, loadFailure, includeExeFiles, filter);

  string[] binPaths = binPath.Split(';');
  return binPaths.SelectMany(bin =>
  {
      var path = Path.Combine(assemblyPath, bin);
      return FindAssemblies(path, loadFailure, includeExeFiles, filter);
  });
}

Here PVS-Studio issues two warnings at once:

• V3022 Expression ‘string.IsNullOrEmpty(binPath)’ is always true. AssemblyFinder.cs 23;

• V3142 Unreachable code detected. It is possible that an error is present. AssemblyFinder.cs 26.

The analyzer reports that an always-true condition and an unreachable code have been detected in this code. Let’s find out how it happened.

In variable binPat an empty string is provided. Immediately after assignment, it is checked that the value of this variable is equal null or an empty string. The result of such a check is always true. It turns out that the execution of the method always ends on this condition, because the block returns a value.

It’s hard to tell if this is a bug or if this behavior is implemented on purpose. In any case, the code looks suspicious because a good part of the logic of the method is lost.

Issue 4

public override Point? Read(ref Utf8JsonReader reader,
                            Type typeToConvert,
                            JsonSerializerOptions options)
{
  if (reader.TokenType != JsonTokenType.StartObject) // <=
    throw new JsonException("....");

  var originalDepth = reader.CurrentDepth;

  if (reader.TokenType == JsonTokenType.Null)        // <=
  {
    reader.Read();
    return null;
  }

  reader.Read();

  var x = double.NaN;
  var y = double.NaN;

  while (reader.TokenType == JsonTokenType.PropertyName)
  {
    ....
  }

  ....
}

Warning PVS-Studio: V3022 Expression ‘reader.TokenType == JsonTokenType.Null’ is always false. DoubleActivity_Specs.cs 55

Consider the first condition of the method Read. It checks the value reader.TokenType. If it doesn’t match JsonTokenType.StartObject, then an exception will be thrown. It follows that after the condition and before the next method call Read, reader.TokenType will matter JsonTokenType.StartObject. It turns out that there will be a second one in the block if the execution thread will never enter. Maybe after the first one if a call was missed Read.

A potential negative index appeal

Issue 5

private static void
        ReturnClosureTypeToParamTypesToPool(Type[] closurePlusParamTypes)
{
  var paramCount = closurePlusParamTypes.Length - 1;                  // <=
  if (paramCount != 0 && paramCount < 8)
    Interlocked.Exchange(ref _closureTypePlusParamTypesPool[paramCount],
                         closurePlusParamTypes);
}

Warning PVS-Studio: V3106 Possible negative index value. the value of “paramCount” index could reach -1. ExpressionCompiler.cs 555

Note the variable paramCount. Its value is the size of the array passed as an argument, reduced by 1. This value is then used as an index. However, checking before use does not guarantee that the index is not negative. An exception will be thrown if an index with a negative value is accessed.

Maybe the developers are sure that closurePlusParamTypes always contains at least one element. Maybe now it really is. But there is no guarantee that this method will not be passed an empty collection in the future.

Issue 6

public static bool TryEmit(....)
{
  ....
  if ((parent & ParentFlags.InlinedLambdaInvoke) != 0)
  {
    var index = closure.GetLabelOrInvokeIndex(gt.Target); // <=
    var invokeIndex = closure.Labels
                             .Items[index]                // <=
                             .InlinedLambdaInvokeIndex;
    ....
  }
  ....
}

Warning PVS-Studio: V3106 Possible negative index value. The value of ‘index’ index could reach -1. ExpressionCompiler.cs 1977

The analyzer indicates a potential reversal of the negative index. Changeable index contains a value used as an index. It will be obtained as a result of the execution of the method GetLabelOrInvokeIndex. Consider the definition of this:

public short GetLabelOrInvokeIndex(object labelTarget)
{
  var count = Labels.Count;
  var items = Labels.Items;
  for (short i = 0; i < count; ++i)
    if (items[i].Target == labelTarget)
        return i;
  return -1;
}

Here we see that the return value option is -1. It is easy to guess that such a scenario is not entirely desirable, since an exception will be thrown.

It is worth noting that when working with the return value GetLabelOrInvokeIndex in a number of other places there is a check that it is -1. One of them:

public short AddInlinedLambdaInvoke(InvocationExpression e)
{
  var index = GetLabelOrInvokeIndex(e);

  if (index == -1)                                         // <=
  {
    ref var label = ref Labels.PushSlot();
    label.Target = e;
    index = (short)(Labels.Count - 1);
  }

  return index;
}

An unused parameter

Issue 7

[Serializable]
public class BusHostInfo : HostInfo
{
  public BusHostInfo()
  {
  }

  public BusHostInfo(bool initialize)
  {
    FrameworkVersion = Environment.Version.ToString();
    OperatingSystemVersion = Environment.OSVersion.ToString();
    var entryAssembly =   System.Reflection.Assembly.GetEntryAssembly()
                       ?? System.Reflection.Assembly.GetCallingAssembly();

    MachineName = Environment.MachineName;

    MassTransitVersion = typeof(HostInfo).GetTypeInfo()
                                         .Assembly
                                         .GetName()
                                         .Version
                                         ?.ToString();

    try
    {
      using var currentProcess = Process.GetCurrentProcess();
      ProcessId = currentProcess.Id;
      ProcessName = currentProcess.ProcessName;
      if ("dotnet".Equals(ProcessName, StringComparison.OrdinalIgnoreCase))
        ProcessName = GetUsefulProcessName(ProcessName);
    }
    catch (PlatformNotSupportedException)
    {
      ProcessId = 0;
      ProcessName = GetUsefulProcessName("UWP");
    }

    var assemblyName = entryAssembly.GetName();
    Assembly = assemblyName.Name;
    AssemblyVersion = assemblyName.Version?.ToString() ?? "Unknown";
  }

  ....
}

Warning PVS-Studio: V3117 Constructor parameter ‘initialize’ is not used. BusHostInfo.cs 17

A parameter of one of the overloads of the class constructor BusHostInfo was not used. This class has a public constructor with no parameters that does nothing at all. The result is the following situation: if you use a constructor with a parameter, initialization is performed regardless of its value. If you use a constructor that does not set parameters, then there is no initialization. This behavior looks very strange.

Maybe the parameter had an effect on the work logic before, but it stopped after the refactoring. The developer may have intended not to remove an unused option to avoid user errors. Errors could arise because the class BusHostInfo public, as is the parsed constructor. This means that it is available directly to users. If you change the signature of the method (remove the parameter), errors will occur at the call points of this constructor due to the lack of the necessary load. If this assumption is correct, I would like to see a comment explaining this point.

Conclusion

It is safe to say that the project code turned out to be clean. To a large extent, this is the merit of the developers, but it is worth considering that they already used a different static analyzer. This became clear based on the comments to suppress his triggers. Even under this condition, it was possible to find a number of suspicious places. Perhaps the analyzer was used earlier, but now it has stopped.

Anyway, hope this article helps developers to find and fix bugs to improve the quality and cleanliness of the project code :).

If you have a desire to check the source code of the project of interest, you can try PVS-Studio for free. Happy using!

If you would like to share this article with an English-speaking audience, please use the translation link: Nikita Panevin. Analysis of suspicious code fragments in MassTransit.