# 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\.

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

## 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](https://github.com/dotnet/orleans)\. 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](https://pvs-studio.com/en/blog/inspections/#Microsoft) 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**

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

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

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

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

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

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

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

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

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

```cpp
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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/blog/posts/csharp/0832/)\.

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:

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

**Issue 9**

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

```cpp
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](https://pvs-studio.com/en/blog/examples/v3125/)\. 

**Issue 10**

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

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3134/) Shift by 41 bits is greater than the size of 'Int32' type of expression '1'\. IntegerCodec\.cs 611
* [V3022](https://pvs-studio.com/en/docs/warnings/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:

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

## Incorrect message

**Issue 13**

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

```cpp
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](https://pvs-studio.com/en/pvs-studio/try-free/)\. You can try it for free, some help with the code never hurts :\)\.

Thank you and see you soon\!