Сергей Хренов

06 Дек 2019

Теги:

#CSharp

Azure SDK for .NET: история о непростом поиске ошибок

06 Дек 2019

Автор: Сергей Хренов

О проекте
Microsoft.Azure.Management.Advisor
Azure.Core
Azure.Messaging.EventHubs
Azure.Storage
Microsoft.Azure.Batch
Microsoft.Azure.ServiceBus
Заключение

Когда мы решили поискать ошибки в проекте Azure SDK for .NET, то были приятно удивлены его размером. "Три с половиной миллиона строк кода", - приговаривали мы, изучая статистику проекта. Это сколько же там всего можно найти. Но, увы и ах. Проект оказался с секретом. Какова же особенность проекта и как прошла его проверка - читайте в этой статье.

О проекте

Эту статью я пишу как бы вдогонку к своей предыдущей статье, которая также была о проекте, связанном с Microsoft Azure: Azure PowerShell: "в основном безвреден". Итак, на этот раз я рассчитывал на солидное число разнообразных и интересных ошибок. Ведь для статического анализа обычно важен размер проекта, особенно при разовых проверках проекта целиком. Да, на практике так обычно не делают. А если и делают, то только на этапе внедрения анализатора. При этом никто не разбирается сразу в огромном числе срабатываний (значительное число предупреждений – это норма при запуске анализатора в таком режиме), а просто откладывают их в технический долг при помощи механизмов подавления сообщений и их хранения в специальных базах (mass suppression). Мы же занимаемся именно разовыми проверками в исследовательских целях. Поэтому большие проекты для изучения всегда предпочтительнее небольших.

Однако проект Azure SDK for .NET сразу показал свою несостоятельность в качестве тестового стенда. Даже его впечатляющий размер не помог, а, скорее, даже осложнил работу. Причина указана в приведенной далее статистике проекта:

Файлов исходного кода .cs (не включая тесты): 16 500
Решений Visual Studio (.sln): 163
Непустых строк кода: 3 462 000
Из них автогенерированных: около 3 300 000
Репозиторий проекта доступен на GitHub.

Примерно 95% кода сгенерировано автоматически, значительная часть этого кода многократно повторяется. Проверять такие проекты статическим анализатором, как правило, трудоёмко и бесполезно, так как там наблюдается много работающего, но нелогичного (на первый взгляд) и избыточного кода. Это приводит к большому числу ложных срабатываний.

В качестве вишенки на торте выступило большое число решений Visual Studio (163), по которым "размазана" эта масса кода. Так что для проверки оставшегося кода (не автогенерированного) пришлось приложить некоторые усилия. Подспорьем стало то, что весь автоматически сгенерированный код находится в подпапках решений по относительному пути "<Папка решения>\src\Generated". Также, каждый файл .cs такого кода содержит специальный комментарий в тэге <auto-generated>:

// <auto-generated>
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License. See License.txt in the project root for
// license information.
//
// Code generated by Microsoft (R) AutoRest Code Generator.
// Changes may cause incorrect behavior and will be lost if the code is
// regenerated.
// </auto-generated>

Для чистоты эксперимента я выборочно проверил около десяти случайно выбранных решений с автогенерированным кодом. О результатах будет ниже.

Итак, несмотря на маленький объем оставшегося "честного" кода, мне все же удалось найти там некоторое количество ошибок. В этот раз я не буду приводить срабатывания в порядке номеров диагностик PVS-Studio. Вместо этого я сгруппирую срабатывания по решениям, в которых они были обнаружены.

Давайте посмотрим, что мне удалось обнаружить в коде Azure SDK for .NET.

Microsoft.Azure.Management.Advisor

Это как раз одно из множества решений, содержащих автогенерированный код. Как я говорил выше, выборочно было проверено около десятка таких решений. И везде сообщения были однотипны и, что ожидаемо, бесполезны. Приведу пару примеров таких срабатываний.

V3022 Expression 'Credentials != null' is always true. AdvisorManagementClient.cs 204

// Code generated by Microsoft (R) AutoRest Code Generator.
....
public ServiceClientCredentials Credentials { get; private set; }
....
public AdvisorManagementClient(ServiceClientCredentials credentials,
  params DelegatingHandler[] handlers) : this(handlers)
{
  if (credentials == null)
  {
    throw new System.ArgumentNullException("credentials");
  }
  Credentials = credentials;
  if (Credentials != null)    // <=
  {
    Credentials.InitializeServiceClient(this);
  }
}

Очевидно, что код избыточен, а проверка Credentials != null бесполезна. Но код рабочий. И автогенерирован. Поэтому – никаких претензий.

V3022 Expression '_queryParameters.Count > 0' is always false. ConfigurationsOperations.cs 871

// Code generated by Microsoft (R) AutoRest Code Generator.
....
public async Task<AzureOperationResponse<IPage<ConfigData>>>
  ListBySubscriptionNextWithHttpMessagesAsync(....)
{
  ....
  List<string> _queryParameters = new List<string>();
  if (_queryParameters.Count > 0)
  {
    ....
  }
  ....
}

И снова, казалось бы, лишенная логики конструкция. Зачем-то проверяют размер только что созданного пустого списка. На самом деле – все в порядке. Это сейчас проверка бессмысленна, но если в генераторе настроят создание списка, например, на базе другой коллекции, то проверка уже будет иметь смысл. Итак – снова к коду нет претензий, учитывая его происхождение, конечно.

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

Azure.Core

V3001 There are identical sub-expressions 'buffer.Length' to the left and to the right of the '<' operator. AzureBaseBuffersExtensions.cs 30

public static async Task WriteAsync(...., ReadOnlyMemory<byte> buffer, ....)
{
  byte[]? array = null;
  ....
  if (array == null || buffer.Length < buffer.Length)  // <=
  {
    if (array != null)
      ArrayPool<byte>.Shared.Return(array);
    array = ArrayPool<byte>.Shared.Rent(buffer.Length);
  }
  if (!buffer.TryCopyTo(array))
    throw new Exception("could not rent large enough buffer.");
  ....
}

Ошибка в условии допущена, вероятно, в результате применения copy-paste. Судя по тому, что далее в коде buffer копируют в array, проверка должна иметь вид:

if (array == null || array.Length < buffer.Length)

Но, как я всегда говорю, правкой таких ошибок должен заниматься автор кода.

V3083 Unsafe invocation of event '_onChange', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. ClientOptionsMonitor.cs 44

private event Action<TOptions, string> _onChange;
....
private void InvokeChanged(....)
{
  ....
  if (_onChange != null)
  {
    _onChange.Invoke(options, name);
  }
}

Некритичная, но ошибка. Между проверкой события на равенство null и его инвокацией от события могут успеть отписаться. Тогда переменная _onChange получит значение null, и будет выброшено исключение. Код необходимо переписать более безопасно. Например, так:

private void InvokeChanged(....)
{
  ....
  _onChange?.Invoke(options, name);
}

Azure.Messaging.EventHubs

V3080 Possible null dereference. Consider inspecting 'eventPropertyValue'. AmqpMessageConverter.cs 650

private static bool TryCreateEventPropertyForAmqpProperty(
  object amqpPropertyValue,
  out object eventPropertyValue)
{
  eventPropertyValue = null;
  ....
  switch (GetTypeIdentifier(amqpPropertyValue))
  {
    case AmqpProperty.Type.Byte:
    ....
    case AmqpProperty.Type.String:
      eventPropertyValue = amqpPropertyValue;
      return true;
    ....
  }
  ....
  switch (amqpPropertyValue)
  {
    case AmqpSymbol symbol:
      eventPropertyValue = ....;
      break;

    case byte[] array:
      eventPropertyValue = ....;
      break;

    case ArraySegment<byte> segment when segment.Count == segment.Array.Length:
      eventPropertyValue = ....;
      break;

    case ArraySegment<byte> segment:
      ....
      eventPropertyValue = ....;
      break;

    case DescribedType described when (described.Descriptor is AmqpSymbol):
      eventPropertyValue = ....;
      break;

    default:
      var exception = new SerializationException(
        string.Format(...., eventPropertyValue.GetType().FullName));  // <=
      ....
  }

  return (eventPropertyValue != null);
}

Давайте отследим, что происходит со значением переменной eventPropertyValue в приведенном фрагменте кода. В начале метода переменной присваивается null. Далее, в одном из условий первого switch, переменная инициализируется, после чего происходит выход из метода. Второй блок switch содержит массу условий, в каждом из которых переменная также получает некое новое значение. А вот в блоке default переменную eventPropertyValue просто используют без всякой проверки, что является ошибкой, так как в данный момент переменная равна null.

V3066 Possible incorrect order of arguments passed to 'EventHubConsumer' constructor: 'partitionId' and 'consumerGroup'. TrackOneEventHubClient.cs 394

public override EventHubConsumer CreateConsumer(....)
{
  return new EventHubConsumer
  (
    new TrackOneEventHubConsumer(....),
    TrackOneClient.EventHubName,
    partitionId,                  // <= 3
    consumerGroup,                // <= 4
    eventPosition,
    consumerOptions,
    initialRetryPolicy
  );
}

Анализатор заподозрил, что при вызове конструктора класса EventHubConsumer перепутан порядок третьего и четвертого аргументов. Посмотрим на объявление конструктора:

internal EventHubConsumer(TransportEventHubConsumer transportConsumer,
                          string eventHubName,
                          string consumerGroup,         // <= 3
                          string partitionId,           // <= 4
                          EventPosition eventPosition,
                          EventHubConsumerOptions consumerOptions,
                          EventHubRetryPolicy retryPolicy)
{
  ....
}

Аргументы и правда перепутаны местами. Я предполагаю как была допущена эта ошибка. Виновато, вероятно, неудачное форматирование кода. Взгляните еще раз на объявление конструктора EventHubConsumer. Из-за того, что первый параметр transportConsumer расположен на той же строке, что и имя класса, при беглом просмотре кода может показаться, что параметр partitionId находится на третьем месте, а не на четвертом (моих комментариев с порядковыми номерами параметров в оригинальном коде нет). Это только предположение, но я бы изменил форматирование кода объявления конструктора на такое:

internal EventHubConsumer
(
  TransportEventHubConsumer transportConsumer,
  string eventHubName,
  string consumerGroup,
  string partitionId,
  EventPosition eventPosition,
  EventHubConsumerOptions consumerOptions,
  EventHubRetryPolicy retryPolicy)
{
  ....
}

Azure.Storage

V3112 An abnormality within similar comparisons. It is possible that a typo is present inside the expression 'ContentLanguage == other.ContentEncoding'. BlobSasBuilder.cs 410

public struct BlobSasBuilder : IEquatable<BlobSasBuilder>
{
  ....
  public bool Equals(BlobSasBuilder other) =>
    BlobName == other.BlobName &&
    CacheControl == other.CacheControl &&
    BlobContainerName == other.BlobContainerName &&
    ContentDisposition == other.ContentDisposition &&
    ContentEncoding == other.ContentEncoding &&         // <=
    ContentLanguage == other.ContentEncoding &&         // <=
    ContentType == other.ContentType &&
    ExpiryTime == other.ExpiryTime &&
    Identifier == other.Identifier &&
    IPRange == other.IPRange &&
    Permissions == other.Permissions &&
    Protocol == other.Protocol &&
    StartTime == other.StartTime &&
    Version == other.Version;
}

Ошибка, допущенная по невнимательности. Найти подобную ошибку при code-review довольно сложно. Правильный вариант проверки:

    ....
    ContentEncoding == other.ContentEncoding &&
    ContentLanguage == other.ContentLanguage &&
    ....

V3112 An abnormality within similar comparisons. It is possible that a typo is present inside the expression 'ContentLanguage == other.ContentEncoding'. FileSasBuilder.cs 265

public struct FileSasBuilder : IEquatable<FileSasBuilder>
{
  ....
  public bool Equals(FileSasBuilder other)
    => CacheControl == other.CacheControl
    && ContentDisposition == other.ContentDisposition
    && ContentEncoding == other.ContentEncoding         // <=
    && ContentLanguage == other.ContentEncoding         // <=
    && ContentType == other.ContentType
    && ExpiryTime == other.ExpiryTime
    && FilePath == other.FilePath
    && Identifier == other.Identifier
    && IPRange == other.IPRange
    && Permissions == other.Permissions
    && Protocol == other.Protocol
    && ShareName == other.ShareName
    && StartTime == other.StartTime
    && Version == other.Version
    ;

Точно такая же ошибка в очень похожем фрагменте кода. Вероятно, код был скопирован и частично модифицирован. А вот ошибка осталась.

Microsoft.Azure.Batch

V3053 An excessive expression. Examine the substrings 'IList' and 'List'. PropertyData.cs 157

V3053 An excessive expression. Examine the substrings 'List' and 'IReadOnlyList'. PropertyData.cs 158

public class PropertyData
{
  ....
  public bool IsTypeCollection => this.Type.Contains("IList") ||
                                  this.Type.Contains("IEnumerable") ||
                                  this.Type.Contains("List") ||        // <=
                                  this.Type.Contains("IReadOnlyList"); // <=
}

Анализатор выдал два предупреждения о бессмысленных либо ошибочных проверках. В первом случае поиск подстроки "List" после поиска "IList" выглядит избыточно. Действительно, условие:

this.Type.Contains("IList") || this.Type.Contains("List")

можно заменить на такое:

this.Type.Contains("List")

Во втором случае поиск подстроки "IReadOnlyList" лишен смысла, так как ранее производят поиск более короткой подстроки "List".

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

V3095 The 'httpRequest.Content.Headers' object was used before it was verified against null. Check lines: 76, 79. BatchSharedKeyCredential.cs 76

public override Task ProcessHttpRequestAsync(
  HttpRequestMessage httpRequest, ....)
{
  ....
  signature.Append(httpRequest.Content != null
    && httpRequest.Content.Headers.Contains("Content-Language") ? .... :  
                                                                  ....;

  long? contentLength = httpRequest.Content?.Headers?.ContentLength;
  ....
}

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

V3125 The 'omPropertyData' object was used after it was verified against null. Check lines: 156, 148. CodeGenerationUtilities.cs 156

private static string GetProtocolCollectionToObjectModelCollectionString(
  ...., PropertyData omPropertyData, ....)
{
  if (IsMappedEnumPair(omPropertyData?.GenericTypeParameter, ....))
  {
    ....
  }

  if (IsTypeComplex(omPropertyData.GenericTypeParameter))
  ....
}

Обратная ситуация. Один блок кода содержит безопасный вариант доступа к потенциально нулевой ссылке omPropertyData. Далее в коде с этой же ссылкой работают без всяких проверок.

V3146 Possible null dereference of 'value'. The 'FirstOrDefault' can return default null value. BatchSharedKeyCredential.cs 127

public override Task
  ProcessHttpRequestAsync(HttpRequestMessage httpRequest, ....)
{
  ....
  foreach (string canonicalHeader in customHeaders)
  {
    string value = httpRequest.Headers.
                   GetValues(canonicalHeader).FirstOrDefault();
    value = value.Replace('\n', ' ').Replace('\r', ' ').TrimStart();
    ....
  }
  ....
}

В результате работы метода FirstOrDefault, если поиск потерпит неудачу, будет возвращено значение по умолчанию для типа string, то есть null. Значение будет присвоено переменной value, которая используется далее в коде с методом Replace безо всяких проверок. Код следует сделать более безопасным. Например, так:

foreach (string canonicalHeader in customHeaders)
{
  string value = httpRequest.Headers.
                 GetValues(canonicalHeader).FirstOrDefault();
  value = value?.Replace('\n', ' ').Replace('\r', ' ').TrimStart();
  ....
}

Microsoft.Azure.ServiceBus

V3121 An enumeration 'BlocksUsing' was declared with 'Flags' attribute, but does not set any initializers to override default values. Fx.cs 69

static class Fx
{
  ....
  public static class Tag
  {
    ....
    [Flags]
    public enum BlocksUsing
    {
      MonitorEnter,
      MonitorWait,
      ManualResetEvent,
      AutoResetEvent,
      AsyncResult,
      IAsyncResult,
      PInvoke,
      InputQueue,
      ThreadNeutralSemaphore,
      PrivatePrimitive,
      OtherInternalPrimitive,
      OtherFrameworkPrimitive,
      OtherInterop,
      Other,

      NonBlocking,
    }
    ....
  }
  ....
}

Перечисление объявлено с атрибутом Flags. При этом значения констант оставлены по умолчанию (MonitorEnter = 0, MonitorWait = 1, ManualResetEvent = 2 и так далее). Это может приводить к тому, что при попытке использования комбинации флагов, например, второй и третьей констант MonitorWait (=1) | ManualResetEvent (=2), будет получено не уникальное значение, а константа со значением 3 по умолчанию (AutoResetEvent). Это может стать неожиданностью для вызывающего кода. Если перечисление BlocksUsing действительно планируется использовать для задания комбинаций флагов (битовое поле), то следует дать константам значения, равные степеням двойки:

[Flags]
public enum BlocksUsing
{
  MonitorEnter = 1,
  MonitorWait = 2,
  ManualResetEvent = 4,
  AutoResetEvent = 8,
  AsyncResult = 16,
  IAsyncResult = 32,
  PInvoke = 64,
  InputQueue = 128,
  ThreadNeutralSemaphore = 256,
  PrivatePrimitive = 512,
  OtherInternalPrimitive = 1024,
  OtherFrameworkPrimitive = 2048,
  OtherInterop = 4096,
  Other = 8192,

  NonBlocking = 16384,
}

V3125 The 'session' object was used after it was verified against null. Check lines: 69, 68. AmqpLinkCreator.cs 69

public async Task<Tuple<AmqpObject, DateTime>> CreateAndOpenAmqpLinkAsync()
{
  ....
  AmqpSession session = null;
  try
  {
    // Create Session
    ....
  }
  catch (Exception exception)
  {
    ....
    session?.Abort();
    throw AmqpExceptionHelper.GetClientException(exception, null,
      session.GetInnerException(), amqpConnection.IsClosing());
  }
  ....
}

Обратите внимание на работу с переменной session в блоке catch. Вызов метода Abort производится безопасно посредством оператора условного доступа. Но далее производят небезопасный вызов метода GetInnerException. При этом вместо выброса исключения ожидаемого типа, может быть выброшено исключение NullReferenceException. Код необходимо исправить. Метод AmqpExceptionHelper.GetClientException поддерживает передачу значения null для параметра innerException:

public static Exception GetClientException(
  Exception exception, 
  string referenceId = null, 
  Exception innerException = null, 
  bool connectionError = false)
{
  ....
}

Поэтому достаточно использовать оператор условного доступа при вызове session.GetInnerException():

public async Task<Tuple<AmqpObject, DateTime>> CreateAndOpenAmqpLinkAsync()
{
  ....
  AmqpSession session = null;
  try
  {
    // Create Session
    ....
  }
  catch (Exception exception)
  {
    ....
    session?.Abort();
    throw AmqpExceptionHelper.GetClientException(exception, null,
      session?.GetInnerException(), amqpConnection.IsClosing());
  }
  ....
}

Заключение

Как видите, большой размер проекта не всегда гарантирует большое число ошибок. Но не нужно расслабляться - всегда что-то можно найти. Даже в таком непростом по структуре проекте, как Azure SDK for .NET. Да, для этого требуется приложить дополнительные усилия, но тем приятнее будет результат. А чтобы не приходилось прикладывать чрезмерных усилий, мы рекомендуем использовать статический анализ, причем на рабочих местах разработчиков при написании нового кода. Это максимально эффективный подход. Скачайте и попробуйте PVS-Studio в деле. Удачи в борьбе с багами!

#CSharp