# Analysis of suspicious code fragments in MassTransit

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 :\)\.

![1061_MassTransit/image1.png](https://import.viva64.com/docx/blog/1061_MassTransit/image1.png)

## 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](https://github.com/MassTransit/MassTransit/tree/v8.0.15) code\. 

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

## Suspicious condition

**Issue 1**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'string\.IsNullOrEmpty\(binPath\)' is always true\. AssemblyFinder\.cs 23;
* [V3142](https://pvs-studio.com/en/docs/warnings/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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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:

```cpp
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:

```cpp
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**

```cpp
[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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/pvs-studio/try-free/)\. Enjoy the usage\!