# Проверка проекта Orleans статическим анализатором PVS\-Studio

Orleans – кроссплатформенный фреймворк для создания масштабируемых облачных приложений\. Это ПО разработано компанией Microsoft, проекты которой не раз проверялись анализатором PVS\-Studio\. Посмотрим, сколько подозрительных мест удастся найти в этот раз\.

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

## Введение

Orleans масштабируется от локального сервера до распределённых приложений в облаке\. Основной особенностью проекта является модель программирования, упрощающая разработку параллельных распределённых систем\.

Код проекта практически полностью написан на языке C\# и доступен в [репозитории на GitHub](https://github.com/dotnet/orleans)\. Для проверки был использован анализатор PVS\-Studio\. Как уже было сказано, проект Orleans разработан компанией Microsoft, что делает его интересным для анализа\. У нас достаточно [много статей](https://pvs-studio.ru/ru/blog/inspections/#Microsoft) о проверке открытых проектов этой компании, рекомендую с ними ознакомиться\.

В результате анализа было получено 229 срабатываний, из которых 38 имели уровень High, 106 – Medium и 85 – Low\. В этой статье я расскажу о самых интересных из них\.

## Неочевидная инициализация

**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: [V3128](https://pvs-studio.ru/ru/docs/warnings/v3128/) The 'ActivationId' property is used before it is initialized in constructor\. SystemTarget\.cs 83

Анализатор указывает на то, что в конструкторе одно из свойств используется до его инициализации\. Свойству _this\.ActivationAddress_ присваивается значение, полученное в результате выполнения _GrainAddress\.GetAddress_\. Этому методу в качестве одного из аргументов передаётся _this\.ActivationId_\. В целом вполне корректная операция, если бы не одно "но"\. Свойство _this\.ActivationId_ будет проинициализировано уже после использования\. Скорее всего, разработчик перепутал порядок инициализации вышеупомянутых свойств\.

## Одинаковые then и 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: [V3004](https://pvs-studio.ru/ru/docs/warnings/v3004/) The 'then' statement is equivalent to the 'else' statement\. TransactionalStateStorageTestRunner\.cs 327

Сообщение анализатора обращает наше внимание на то, что ветки then и else условного оператора _if_ совпадают\. Действительно, очень странно, что независимо от значения аргумента _reverseOrder_ выполняются одни и те же действия\. Скорее всего, этот код не дописан или содержит опечатку\.

Если задумано, что действия должны быть одинаковы, то этому коду явно не хватает поясняющего комментария\.

## Неоднозначный 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: [V3116](https://pvs-studio.ru/ru/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

Обратите внимание на цикл _for_\. С его помощью должна выводиться некоторая отладочная информация, но этого не произойдёт, ведь перед ним _batchOperation_ очищается\. Скорее всего, удалять элементы из списка следует уже после цикла\.

**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: [V3102](https://pvs-studio.ru/ru/docs/warnings/v3102/) Suspicious access to element of 'parameters' object by a constant index inside a loop\. OrleansGeneratedCodeHelper\.cs 267

Анализатор обнаружил цикл, в котором происходит обращение по константному индексу\. Обратите внимание на условие _if \(parameters\.Length \!\= parameterTypes\.Length\)_\. Если оно истинно, срабатывает оператор перехода _continue_\. Следовательно для выполнения последующего кода коллекции должны иметь одинаковый размер\. Подобная "отсечка", скорее всего, сделана для того, чтобы далее сравнить пары соответствующих элементов этих коллекций\. Однако в теле _for_ из коллекции _parameters_ всегда берётся первый элемент\.

Стоит отметить, что есть ещё один неоднозначный момент\. Использование _for_ не имеет смысла, так как в нём не производится никаких действий кроме перехода к новой итерации этого цикла\. Вероятно, ожидался переход к следующей итерации внешнего цикла, но что\-то пошло не так\. 

Исправить сложившуюся ситуацию можно, добавив флаг для перехода на новую итерацию _foreach_ и изменив индекс для _parameters_ на _i_\. Код будет выглядеть следующим образом:

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

## Проблемы с 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: [V3020](https://pvs-studio.ru/ru/docs/warnings/v3020/) An unconditional 'return' within a loop\. InMemoryTransportListenerFactory\.cs 117

Обратите внимание на цикл _while\._ В его теле использован оператор _return_, который выполнится на первой итерации\. Возможно, разработчик подразумевал, что код внутри цикла должен отработать один раз\. Тогда возникает вопрос: почему бы не использовать _if_? Ведь таким образом код станет более понятным\. Также возможно, что цикл здесь действительно нужен\. В этом случае оператор _return_ должен выполняться в зависимости от какого\-то условия\.

**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: [V3020](https://pvs-studio.ru/ru/docs/warnings/v3020/) An unconditional 'return' within a loop\. OrleansGeneratedCodeHelper\.cs 99

Срабатывание схоже с предыдущим\. Здесь в теле _while_ использован оператор _return_\. Как уже было сказано, использование _while_ подобным образом не имеет смысла, ведь у цикла будет лишь одна итерация\. Возможно, тут должно быть некоторое условие для использования оператора _return_\. 

## Потенциальное разыменование нулевой ссылки

**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: [V3095](https://pvs-studio.ru/ru/docs/warnings/v3095/) The 'participant' object was used before it was verified against null\. Check lines: 282, 284\. LocalSiloHealthMonitor\.cs 282

Анализатор предупреждает о том, что переменная _participant_ использовалась перед тем, как была проверена на _null_\. Выглядит странно, что в условии производится обращение к данной переменной без проверки:

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

Все последующие обращения к этой же переменной \(коих 4\) производятся с проверкой\. Судя по всему, разработчик ожидал, что _participant_ может иметь значение _null_\. Стоит отметить, что _CheckHealth_ не является методом расширения\. При вызове такого метода у переменной, равной _null_, будет выброшено исключение типа _NullReferenceException_\.

Хотя потенциально опасный участок кода находится в блоке _try_, маловероятно, что разработчик хотел отлавливать исключения именно этого типа\. Такой вывод можно сделать, исходя из числа проверок на _null_ в этом блоке\.

**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: [V3153](https://pvs-studio.ru/ru/docs/warnings/v3153/) Enumerating the result of null\-conditional access operator can lead to NullReferenceException\. Silo\.cs 180

Обратите внимание на коллекцию, по которой будет производиться итерирование в _foreach_\. Она является результатом вызова методов _GetServices_ и _Select_\. Стоит отметить, что вызовы производятся с помощью оператора '?\.'\. Скорее всего разработчик ожидал, что в результате обращения к _namedLifecycleParticipantCollection_ или при вызове метода _GetServices_ может быть получен _null_\.

В таком случае значением _namedLifecycleParticipantCollection?\.GetServices\(\.\.\.\.\)?\.Select\(\.\.\.\.\)_ также будет _null_\. Попытка же обхода _null_ в _foreach_ приведёт к выбрасыванию исключения типа _NullReferenceException_\. Увы, null\-условный оператор в данном случае не спасает\. За подробным объяснением этой проблемы рекомендую обратиться к [статье](https://pvs-studio.ru/ru/blog/posts/csharp/0832/)\.

Чтобы избежать подобной ситуации, можно использовать оператор '??'\. В таком случае, если '?\.' вернёт _null_, исключение выброшено не будет\.

Исправленный вариант цикла выглядит так:

```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: [V3125](https://pvs-studio.ru/ru/docs/warnings/v3125/) The 'msg' object was used after it was verified against null\. Check lines: 275, 269\. SiloConnection\.cs 275

И снова потенциальное разыменование нулевой ссылки\. В данном примере перед первым обращением к переменной _msg_ производится проверка на _null_\. После этого она передаётся в качестве аргумента в метод _MessagingStatisticsGroup\.OnFailedSentMessage_, где также присутствует проверка:

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

А вот во втором условии метода _FailMessage_ проверки нет_\. _Как было сказано ранее, при разыменовывании нулевой ссылки будет выброшено исключение типа _NullReferenceException_\.

Подобные ошибки часто встречаются при проверке проектов\. Ознакомиться с ними можно [здесь](https://pvs-studio.ru/ru/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: [V3125](https://pvs-studio.ru/ru/docs/warnings/v3125/) The 'table' object was used after it was verified against null\. Check lines: 306, 303\. LocalReminderService\.cs 306

Срабатывание схоже с предыдущим_\._ Здесь переменная _table_ проверяется на _null_, после чего обращение к ней производится без проверки\. Как и в прошлом примере, если _table_ будет иметь значение _null_, обращение к её свойству приведёт к выбрасыванию исключения\.

## Подозрительные сдвиги

**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);
  }
}
```

Тут PVS\-Studio выдает сразу два предупреждения:

* [V3134](https://pvs-studio.ru/ru/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.ru/ru/docs/warnings/v3022/) Expression 'value \> 1 << 41 \|\| \-value \> 1 << 41' is always true\. Probably the '&&' operator should be used here\. IntegerCodec\.cs 611

Для начала разберём первое предупреждение\. В условии _if \(value \> 1 << 41 \|\| \-value \> 1 << 41\)_ производится побитовый сдвиг 1\. После этого полученный результат сравнивается с переменной _value\. _Проблема заключается в том, что 1 имеет тип _Int32_, размерность которого составляет 32 бита\. Соответственно, сдвиг на 41 бит эквивалентен сдвигу на 9\. Сдвиг на большее число бит, чем размер левого операнда оператора '\>\>', выглядит странно\.

В условии производится сравнение с переменной _value_\. Она имеет тип _long_,_ _псевдоним типа _Int64\. _Также в блоке then этого условия вызывается метод _WriteInt64_, принимающий в качестве аргумента переменную типа _Int64_\. Вышеперечисленные моменты заставляют сомневаться в корректности реализации сдвига\.

Чтобы разобраться во втором срабатывании, стоит рассмотреть ещё одно условие, а именно – _if \(value <\= int\.MaxValue && value \>\= int\.MinValue_\)\._ _В блоке else этого условия значение _value_ не будет входить в диапазон типа _Int32_\. Следовательно условие _if \(value \> 1 << 41 \|\| \-value \> 1 << 41\)_ всегда будет истинным\. 

Скорее всего, разработчик полагал, что число 1, по отношению к которому производится сдвиг в условии _if \(value \> 1 << 41 \|\| \-value \> 1 << 41\)_, будет иметь тип _Int64_, но это не так\.

Для корректной реализации следует использовать суффикс _L_\. После внесения этой правки условие будет выглядеть следующим образом:

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

## Некорректное сообщение

**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: [V3138](https://pvs-studio.ru/ru/docs/warnings/v3138/) String literal contains potential interpolated expression\. Consider inspecting: type\. ExceptionCodec\.cs 367

Анализатор обнаружил строку, которая, скорее всего, содержит интерполированное выражение, но символ '$' не был использован\. Обратите внимание на последний блок else\. В нём создаётся объект типа _NotSupportedException, _в конструктор которого передаётся строка\. Маловероятно, что разработчик хотел передавать сообщения вида "Type \{type\} is not supported"\. Скорее всего, вместо подстроки "\{type\}" должно подставляться значение переменной _type_\. Корректный код будет выглядеть следующим образом: 

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

## Заключение 

Подводя итог, можно сказать, что срабатывания оказались достаточно разнообразными\. В статье представлены как те, которые можно отнести к ошибкам, так и незначительные погрешности в коде\. В любом случае стоит избавиться как от первых, так и от вторых\.

Треть описанных срабатываний была связана с потенциальным разыменованием нулевой ссылки\. Это неудивительно – именно таких предупреждений было выдано больше всего\. Возможно, разработчикам Orleans стоит уделить этому аспекту больше внимания\.

Рекомендую вам проверить свой код с помощью анализатора, который можно скачать [здесь](https://pvs-studio.ru/ru/pvs-studio/try-free/)\. Попробовать его можно бесплатно, лишним явно не будет :\)\.

Спасибо за внимание, до новых встреч\!