Никита Паневин

04 Июл 2022

Теги:

#CSharp

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

04 Июл 2022

Автор: Никита Паневин

Введение
Неочевидная инициализация
Одинаковые then и else
Неоднозначный for
Проблемы с while
Потенциальное разыменование нулевой ссылки
Подозрительные сдвиги
Некорректное сообщение
Заключение

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

Введение

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

Код проекта практически полностью написан на языке C# и доступен в репозитории на GitHub. Для проверки был использован анализатор PVS-Studio. Как уже было сказано, проект Orleans разработан компанией Microsoft, что делает его интересным для анализа. У нас достаточно много статей о проверке открытых проектов этой компании, рекомендую с ними ознакомиться.

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

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

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

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 The 'then' statement is equivalent to the 'else' statement. TransactionalStateStorageTestRunner.cs 327

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

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

Неоднозначный 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: 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

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 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. Код будет выглядеть следующим образом:

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

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 An unconditional 'return' within a loop. InMemoryTransportListenerFactory.cs 117

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

Issue 6

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

Предупреждение PVS-Studio: V3020 An unconditional 'return' within a loop. OrleansGeneratedCodeHelper.cs 99

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

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

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: V3095 The 'participant' object was used before it was verified against null. Check lines: 282, 284. LocalSiloHealthMonitor.cs 282

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

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

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

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

Issue 8

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

Предупреждение PVS-Studio: 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-условный оператор в данном случае не спасает. За подробным объяснением этой проблемы рекомендую обратиться к статье.

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

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

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: V3125 The 'msg' object was used after it was verified against null. Check lines: 275, 269. SiloConnection.cs 275

И снова потенциальное разыменование нулевой ссылки. В данном примере перед первым обращением к переменной msg производится проверка на null. После этого она передаётся в качестве аргумента в метод MessagingStatisticsGroup.OnFailedSentMessage, где также присутствует проверка:

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

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

Подобные ошибки часто встречаются при проверке проектов. Ознакомиться с ними можно здесь.

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

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

Для начала разберём первое предупреждение. В условии 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. После внесения этой правки условие будет выглядеть следующим образом:

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

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

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: V3138 String literal contains potential interpolated expression. Consider inspecting: type. ExceptionCodec.cs 367

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

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

Заключение

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

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

Рекомендую вам проверить свой код с помощью анализатора, который можно скачать здесь. Попробовать его можно бесплатно, лишним явно не будет :).

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

#CSharp