Analysis of suspicious code fragments in MassTransit

Aug 09 2023

Author: Nikita Panevin

Introduction
Suspicious condition
Possible negative index access
Unused parameter
Conclusion

MassTransit is an open-source distributed application framework for .NET. In this article, we'll discuss some issues in its source code. A static analyzer will help us to identify them. Enjoy :).

Introduction

The project's code quality is crucial because MassTransit offers a public API. Unexpected behavior of the framework can damage not only the framework but also the applications that use it. This is one of the reasons why we decided to check MassTransit. In this article, we look at the MassTransit v8.0.15 code.

So, let's dive in and examine the most curious code snippets.

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

The PVS-Studio warning: V3001 There are identical sub-expressions '!context.ReceiveContext.IsDelivered' to the left and to the right of the '&&' operator. OutboxMessagePipe.cs 57

Look at the if statement. The value of the same property is checked, and that looks weird. Obviously, this check makes no sense. However, if we look at the properties that can be accessed from the context.ReceiveContext object, we will find IsFaulted. This may be the property that should be checked 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);
  });
}

For this code, 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 there is a condition that is always true and that unreachable code has been detected. Well, let's find out how come.

An empty string is assigned to the binPat variable. Immediately after the assignment, the value of the variable is checked to see if it is null or empty. The result of this check is always true. This way, the execution of the method always ends with this condition, since the value is returned in the then block.

It is difficult to say if this is an error or if this behavior has been implemented intentionally. In any case, the code looks suspicious because much of the method logic 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)
  {
    ....
  }

  ....
}

The PVS-Studio warning: V3022 Expression 'reader.TokenType == JsonTokenType.Null' is always false. DoubleActivity_Specs.cs 55

Let's look at the first condition of the Read method. The value of reader.TokenType is checked. If it is not equal to JsonTokenType.StartObject, an exception is thrown. Thus, reader.TokenType will have the value of JsonTokenType.StartObject after the condition is met and before the next call to the Read method. It turns out that the execution flow will never enter the then block of the second if. The Read call may have been skipped after the first if.

Possible negative index access

Issue 5

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

The PVS-Studio warning: V3106 Possible negative index value. The value of 'paramCount' index could reach -1. ExpressionCompiler.cs 555

Take a look at the ParamCount variable. Its value is the size of the array passed as an argument, reduced by 1. Then this value is used as an index. However, checking before use does not guarantee that the index is not negative. If a negative value index is accessed, an exception is thrown.

The developers may have assumed that closurePlusParamTypes always contains at least one element. Maybe, at this point, that's really the case. But there is no guarantee that an empty collection will not be passed to this method 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;
    ....
  }
  ....
}

The PVS-Studio warning: V3106 Possible negative index value. The value of 'index' index could reach -1. ExpressionCompiler.cs 1977

The analyzer reports a potential negative index access. The index variable has a value that is used as an index. The value is received as a result of the GetLabelOrInvokeIndex method execution. Now let's move on to the definition of this method:

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 one of the options for the return value is -1. This is not very good, as it raises an exception.

It is worth noting that there is a check to ensure that the return value of GetLabelOrInvokeIndex is not equal to -1 in some other places throughout the code. Here is one of the cases:

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

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

  ....
}

PVS-Studio warning: V3117 Constructor parameter 'initialize' is not used. BusHostInfo.cs 17

The parameter of one of the BusHostInfo class constructor overloads has not been used. Actually, this class has a public constructor without parameters, which does nothing at all. As a result, if we use a constructor with a parameter, initialization is performed regardless of the parameter's value. But if we use a constructor that does not receive parameters, then there is no initialization. This behavior is odd.

The parameter may have affected the code logic before but stopped after refactoring. Developers may have deliberately chosen not to delete unused parameters in order to prevent errors for users. Errors can occur because the BusHostInfo class is public, just like the constructor we are discussing. It means that the constructor can be accessed by users directly. If we change the method signature (remove the parameter), then errors will occur in the places where this constructor is called because the necessary overload is missing. In the event that my hunch is right, I'd appreciate a code comment on this.

Conclusion

The project source code turned out to be pretty clean. The clean code is mostly the result of the developers' efforts; however, it should be highlighted that the development team employed another static analyzer. This is clear due to the comments that were used to suppress its warnings. Even though the code was previously checked, the PVS-Studio analyzer was able to find several suspicious code fragments. Perhaps the development team used an analyzer before, but now they don't.

Anyway, I hope this article will help developers find and fix bugs to enhance the quality of the project's code :).

If you want to check the project's source code, try using PVS-Studio for free. Enjoy the usage!

#CSharp