Checking Orleans with the PVS-Studio analyzer

Orleans is a cross-platform framework for creating scalable cloud applications. This software is developed by Microsoft, and PVS-Studio often checks its projects. Let's see how many suspicious places our analyzer can find this time.

Introduction

Orleans scales from an on-premises server to cloud-based distributed applications. The project's main feature is a programming model that simplifies the development of concurrent distributed systems.

The project code is almost entirely written in C#. You can find it in the repository on GitHub. We checked the code with the PVS-Studio analyzer. As mentioned above, the Orleans project was developed by Microsoft, which makes it interesting for analysis. We have quite a lot of articles about checking Microsoft open-source projects, I encourage you to read them.

As a result of analysis, we got 229 warnings — 38 with High level of certainty, 106 with Medium level, and 85 with Low level. In this article, I'll describe the most interesting ones.

Non-obvious initialization

Issue 1

public abstract class SystemTarget : ....
{
  ....
  internal SystemTarget(SystemTargetGrainId grainId, 
                        SiloAddress silo,
                        bool lowPriority,
                        ILoggerFactory loggerFactory)
  {
    this.id = grainId;
    this.Silo = silo;
    this.ActivationAddress = GrainAddress.GetAddress(this.Silo,
                                                     this.id.GrainId, 
                                                     this.ActivationId); // <=

    this.IsLowPriority = lowPriority;
    this.ActivationId = ActivationId                                     // <=
                        .GetDeterministic(grainId.GrainId);
    this.timerLogger = loggerFactory.CreateLogger<GrainTimer>();
    this.logger = loggerFactory.CreateLogger(this.GetType());
  }
  ....
}

PVS-Studio's warning: V3128 The 'ActivationId' property is used before it is initialized in constructor. SystemTarget.cs 83

The analyzer detects that one of the properties in the constructor is used before initialization. The this.ActivationAddress property is assigned the value which was obtained as a result of the GrainAddress.GetAddress method's execution. this.ActivationId is passed as one of the parameters to this method. Well, it looks like a correct operation. Except one thing. The this.ActivationId property is initialized after it's used. Perhaps the developer confused the initialization order of the properties mentioned above.

The same then and else

Issue 2

public virtual async Task ConfirmOneAndCancelOne(bool useTwoSteps = false,
                                                 bool reverseOrder = false)
{
  ....
  if (useTwoSteps)
  {
    if (reverseOrder)                                                 // <=
    {
      etag = await stateStorage.Store(etag, metadata, 
                                      emptyPendingStates, 1, null);

      _ = await stateStorage.Store(etag, metadata,
                                         emptyPendingStates, null, 1);
    }
    else
    {
      etag = await stateStorage.Store(etag, metadata,
                                      emptyPendingStates, 1, null);

      _ = await stateStorage.Store(etag, metadata,
                                   emptyPendingStates, null, 1);
    }
  }
  else
  {
    _ = await stateStorage.Store(etag, metadata,
                                 emptyPendingStates, 1, 1);
  }
  ....
}

PVS-Studio's warning: V3004 The 'then' statement is equivalent to the 'else' statement. TransactionalStateStorageTestRunner.cs 327

The analyzer warns that the then and else branches of the conditional if operator are the same. Indeed, it is very strange — the same actions are performed regardless of the value of the reverseOrder argument. Most likely, the code is not completed. Or it's just a typo.

If the developer intended to make these two actions the same, then I think this fragment needs an explanatory comment.

Ambiguous for

Issue 3

private class BatchOperation
{
  private readonly List<TableTransactionAction> batchOperation;
  ....

  public async Task Flush()
  {
    if (batchOperation.Count > 0)
    {
      try
      {
        ....
        batchOperation.Clear();                              // <=
        keyIndex = -1;

        if (logger.IsEnabled(LogLevel.Trace))
        {
          for (int i = 0; i < batchOperation.Count; i++)     // <=
          {
            logger.LogTrace(....)
          }
        }
      }
      catch (Exception ex)
      {
        ....
      }
    }
  }
}

PVS-Studio's warning: V3116 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. AzureTableTransactionalStateStorage.cs 345

Look at the for loop. It should help output some debugging information, but it won't — the batchOperation collection is cleared before this loop. It's better to delete elements from the list after the loop.

Issue 4

public static MethodInfo GetMethodInfoOrDefault(....)
{
  foreach (var method in interfaceType.GetMethods(  BindingFlags.Public 
                                                  | BindingFlags.NonPublic 
                                                  | BindingFlags.Instance))
  {
    ....
    var parameters = method.GetParameters();
    if (parameters.Length != parameterTypes.Length) 
    {
      continue;
    }

    for (int i = 0; i < parameters.Length; i++)
    {
      if (!parameters[0].ParameterType.Equals(parameterTypes[i]))  // <=
      {
        continue;
      }
    }

    return method;
  }
  ....
}

PVS-Studio's warning: V3102 Suspicious access to element of 'parameters' object by a constant index inside a loop. OrleansGeneratedCodeHelper.cs 267

The analyzer was triggered by a loop in which an array element is accessed via the constant index. Look at the if (parameters.Length != parameterTypes.Length) condition. If it is true, the continue statement is triggered. Therefore, the collections should be of the same size to execute the subsequent code. This was most likely made to further compare pairs of corresponding elements of these collections. However, in the for body, the first element is always taken from the parameters collection.

We must say that there's another ambiguous point. Using for is pointless since no actions are performed there except for skipping to a new iteration of this loop. Perhaps the developer expected to move to the next iteration of the external loop, but something went wrong.

This situation can be fixed by adding a flag to move to a new iteration of foreach and changing the index for parameters to i. The code will look like this:

public static MethodInfo GetMethodInfoOrDefault(....)
{
  foreach (var method in interfaceType.GetMethods(  BindingFlags.Public 
                                                  | BindingFlags.NonPublic 
                                                  | BindingFlags.Instance))
  {
    ....
    bool flag = false;

    for (int i = 0; i < parameters.Length; i++)
    {
      if (!parameters[i].ParameterType.Equals(parameterTypes[i]))
      {
        flag = true;
        break;
      }
    }

    if(flag)
      continue;

    return method;
  }
  ....
}

Issues with while

Issue 5

public async ValueTask<ConnectionContext> AcceptAsync(....)
{
  if (await _acceptQueue.Reader.WaitToReadAsync(....))
  {
    while (_acceptQueue.Reader.TryRead(out var item))
    {
      var remoteConnectionContext = item.Connection;
      var localConnectionContext = ....

      item.ConnectionAcceptedTcs.TrySetResult(true);

      return localConnectionContext;                      // <=
    }
  }

  return null;
}

PVS-Studio's warning: V3020 An unconditional 'return' within a loop. InMemoryTransportListenerFactory.cs 117

Now look at the while loop. The loop's body uses the return operator which will be executed on the first iteration. Perhaps the developer meant that the code inside the loop should work only once. If so, why not use if? This will make the code more understandable. It's also possible that this loop is necessary here. In this case, the return operator must be executed depending on some condition.

Issue 6

public static TService UnwrapService<TService>(object caller, TService service)
{
  while (   service is IServiceHolder<TService> 
         && caller is TService callerService)
  {
    return callerService;
  }
  ....
}

PVS-Studio's warning: V3020 An unconditional 'return' within a loop. OrleansGeneratedCodeHelper.cs 99

This issue is similar to the previous one. The return operator is used in the while body. As already mentioned in this article, using while like this is pointless — the loop will have only one iteration. Perhaps there should be some condition for using the return operator.

Potential dereference of the null reference

Issue 7

private int CheckLocalHealthCheckParticipants(DateTime now,
                                              List<string> complaints)
{
  var score = 0;
  foreach (var participant in _healthCheckParticipants)
  {
    try
    {
      if (!participant.CheckHealth(_lastHealthCheckTime, out var reason))  // <=
      {
        _log.LogWarning(...., participant?.GetType().ToString(), reason);  // <=
        complaints?.Add($".... {participant?.GetType().ToString()} ...."); // <=
        ++score;
      }
    }
    catch (Exception exception)
    {
      _log.LogError(exception, ...., participant?.GetType().ToString());   // <=
      Complaints?.Add($".... {participant?.GetType().ToString()} ....");   // <=
      ++score;
    }
  }

  _lastHealthCheckTime = now;
  return score;
}

PVS-Studio's warning: V3095 The 'participant' object was used before it was verified against null. Check lines: 282, 284. LocalSiloHealthMonitor.cs 282

The analyzer detected that the participant variable was used before it was checked for null. It's strange that this variable is accessed without any check:

if (!participant.CheckHealth(_lastHealthCheckTime, out var reason))

All subsequent accesses to the same variable (4 accesses, actually) are checked. Apparently, the developer expected that participant can be null. Note that CheckHealth is not an extension method. If we call such method from a null variable, then NullReferenceException will be thrown.

Although a potentially dangerous code fragment is in the try block, it's unlikely that the developer wanted to catch exceptions of this type. This conclusion can be made based on the number of null checks in this block.

Issue 8

public Silo(ILocalSiloDetails siloDetails, IServiceProvider services)
{
  ....
  foreach (ILifecycleParticipant<ISiloLifecycle> participant
             in namedLifecycleParticipantCollection?.GetServices(this.Services)
                                                   ?.Select(....))
  {
    participant?.Participate(this.siloLifecycle);
  }
  ....
}

PVS-Studio's warning: V3153 Enumerating the result of null-conditional access operator can lead to NullReferenceException. Silo.cs 180

Look at the collection for which iteration will be performed in foreach. This collection is a result of calling the GetServices and Select methods. The calls are made using the '?.' operator. Most likely, the developer expected that null could be obtained as a result of accessing namedLifecycleParticipantCollection or when calling the GetServices method.

In this case, namedLifecycleParticipantCollection?.GetServices(....)?.Select(....) will also be null. An attempt to iterate the collection with null in foreach will lead to NullReferenceException. Unfortunately, the null conditional operator here is useless. If you want a detailed explanation of this problem, you can read this article.

To avoid such a situation, use the '??' operator. In this case, if '?.' returns null, the exception won't be thrown.

The correct version of the loop looks like this:

foreach (ILifecycleParticipant<ISiloLifecycle> participant
             in namedLifecycleParticipantCollection?.GetServices(this.Services)
                                                   ?.Select(....)
                ?? Enumerable.Empty<ILifecycleParticipant<ISiloLifecycle>>)

Issue 9

public void FailMessage(Message msg, string reason)
{
  if (msg != null && msg.IsPing())                          // <=
  {
    this.Log.LogWarning("Failed ping message {Message}", msg);
  }

  MessagingStatisticsGroup.OnFailedSentMessage(msg);
  if (msg.Direction == Message.Directions.Request)          // <=
  {
    if (this.Log.IsEnabled(LogLevel.Debug)) ....;

    this.messageCenter.SendRejection(....);
  }
  else
  {
    this.MessagingTrace.OnSiloDropSendingMessage(....);
  }
}

PVS-Studio's warning: V3125 The 'msg' object was used after it was verified against null. Check lines: 275, 269. SiloConnection.cs 275

Potential dereference of the null reference. Again. In this example, before the msg variable is accessed for the first time, the variable is checked for null. After that, the variable is passed as an argument to the MessagingStatisticsGroup.OnFailedSentMessage method, where it's checked again.

internal static void OnFailedSentMessage(Message msg)
{
  if (msg == null || !msg.HasDirection) return;
  ....
}

However, there's no check in the second if statement of the FailMessage method. As mentioned above, dereferencing of the null reference will lead to NullReferenceException.

We often see such errors when we check open-source projects. You can see examples here.

Issue 10

private async Task ReadTableAndStartTimers(IRingRange range,
                                           int rangeSerialNumberCopy)
{
  ....
  try
  {
    ....
    ReminderTableData table = await reminderTable.ReadRows(....);
    ....
    if (null == table && reminderTable is MockReminderTable) return;  // <=
    var remindersNotInTable = ....
    if (logger.IsEnabled(LogLevel.Debug)) 
      logger.Debug(...., table.Reminders.Count, ....);                // <=
    ....
  }
  catch (Exception exc)
  {
    ....
  }
}

PVS-Studio's warning: V3125 The 'table' object was used after it was verified against null. Check lines: 306, 303. LocalReminderService.cs 306

This warning is similar to the previous one. Here the table variable is checked for null and after that it is accessed without any check. As in the previous example, if table is null, accessing its property will result in an exception thrown.

Suspicious shifts

Issue 11, 12

public static void WriteField<TBufferWriter>
                   (ref Writer<TBufferWriter> writer,
                    uint fieldIdDelta,
                    Type expectedType,
                    long value) where TBufferWriter : IBufferWriter<byte>
{
  ReferenceCodec.MarkValueField(writer.Session);
  if (value <= int.MaxValue && value >= int.MinValue)             // <=
  {
    if (value > 1 << 20 || -value > 1 << 20)
    {
      writer.WriteFieldHeader(fieldIdDelta,
                              expectedType,
                              CodecFieldType,
                              WireType.Fixed32);
      writer.WriteInt32((int)value);
    }
    else
    {
      writer.WriteFieldHeader(fieldIdDelta,
                              expectedType,
                              CodecFieldType,
                              WireType.VarInt);
      writer.WriteVarInt64(value);
    }
  }
  else if (value > 1 << 41 || -value > 1 << 41)                   // <=
  {
    writer.WriteFieldHeader(fieldIdDelta,
                            expectedType,
                            CodecFieldType,
                            WireType.Fixed64);
    writer.WriteInt64(value);
  }
  else
  {
    writer.WriteFieldHeader(fieldIdDelta,
                            expectedType,
                            CodecFieldType,
                            WireType.VarInt);
    writer.WriteVarInt64(value);
  }
}

Here PVS-Studio issues two warnings at once:

V3134 Shift by 41 bits is greater than the size of 'Int32' type of expression '1'. IntegerCodec.cs 611
V3022 Expression 'value > 1 << 41 || -value > 1 << 41' is always true. Probably the '&&' operator should be used here. IntegerCodec.cs 611

Let's inspect the first warning. In the if (value > 1 << 41 || -value > 1 << 41) condition, 1 is shifted bitwise. After that the result is compared with the value variable. The problem is that 1 has the Int32 type, which size is 32 bits. So, a shift of 41 bits is equivalent to the shift of 9. A shift of more bits than the size of the left operand of the '>>' operator looks strange.

In the condition, a comparison is made with the value variable. It has the long type, which is an alias of the Int64 type. Also, in the then block of this condition, the WriteInt64 method is called. This method takes a variable of the Int64 type as an argument. The points mentioned above make us doubt whether the implementation of the shift was correct.

To understand the second warning, we need to inspect one more condition — if (value <= int.MaxValue && value>= int.MinValue). In the else block of this condition, value will not be in the Int32 type range. Hence the if (value > 1 << 41 || -value > 1 << 41) condition will always be true.

Most likely, the developer believed that 1, in regards to which the shift is made in the if (value > 1 << 41 || -value > 1 << 41) condition, is of the Int64 type, but it is not.

For correct implementation, the L suffix should be used. After making this fix, the condition will look like this:

if (value > 1L << 41 || -value > 1L << 41)

Incorrect message

Issue 13

public Exception DeserializeException<TInput>(....)
{
  if (!_typeConverter.TryParse(typeName, out var type))
  {
    ....
  }
  else if (typeof(Exception).IsAssignableFrom(type))
  {
    ....
  }
  else
  {
    throw new NotSupportedException("Type {type} is not supported");
  }
}

PVS-Studio's warning: V3138 String literal contains potential interpolated expression. Consider inspecting: type. ExceptionCodec.cs 367

The analyzer detected a string that most likely contains an interpolated expression, but the '$' symbol was not used. Look at the last else block. It creates an object of the NotSupportedException type. A string is passed to the constructor of this object. I doubt that the developer wanted to send messages like "Type {type} is not supported". Most likely, the value of the type variable should be substituted instead of the "{type}" substring. The code will look like this:

throw new NotSupportedException($"Type {type} is not supported");

Conclusion

Summing up, we can say that the warnings were quite diverse. The article presents both errors and minor mistakes in the code. Anyway, it's better to fix them all.

A third of the warnings described in this article is about the potential dereference of the null reference. This is not surprising – such warnings were issued the most. Perhaps the developers of Orleans should investigate this case.

You can check your code with the analyzer too. Just download it here. You can try it for free, some help with the code never hurts :).

Thank you and see you soon!

Blog