Для получения триального ключа
заполните форму ниже
Team License (базовая версия)
Enterprise License (расширенная версия)
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Запросите информацию о ценах
Новая лицензия
Продление лицензии
--Выберите валюту--
USD
EUR
GBP
RUB
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Бесплатная лицензия PVS-Studio для специалистов Microsoft MVP
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Для получения лицензии для вашего открытого
проекта заполните, пожалуйста, эту форму
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Мне интересно попробовать плагин на:
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Ваше сообщение отправлено.

Мы ответим вам на


Если вы так и не получили ответ, пожалуйста, проверьте папку
Spam/Junk и нажмите на письме кнопку "Не спам".
Так Вы не пропустите ответы от нашей команды.

>
>
>
Azure SDK for .NET: история о непростом…

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

06 Дек 2019

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

0692_Azure_SDK_for_NET_ru/image1.png

О проекте

Эту статью я пишу как бы вдогонку к своей предыдущей статье, которая также была о проекте, связанном с 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 в деле. Удачи в борьбе с багами!

Популярные статьи по теме
Главный вопрос программирования, рефакторинга и всего такого

Дата: 14 Апр 2016

Автор: Андрей Карпов

Вы угадали, ответ - "42". Здесь приводится 42 рекомендации по программированию, которые помогут избежать множества ошибок, сэкономить время и нервы. Автором рекомендаций выступает Андрей Карпов - тех…
Статический анализ как часть процесса разработки Unreal Engine

Дата: 27 Июн 2017

Автор: Андрей Карпов

Проект Unreal Engine развивается - добавляется новый код и изменятся уже написанный. Неизбежное следствие развития проекта - появление в коде новых ошибок, которые желательно выявлять как можно раньш…
Эффект последней строки

Дата: 31 Май 2014

Автор: Андрей Карпов

Я изучил множество ошибок, возникающих в результате копирования кода. И утверждаю, что чаще всего ошибки допускают в последнем фрагменте однотипного кода. Ранее я не встречал в книгах описания этого …
Любите статический анализ кода!

Дата: 16 Окт 2017

Автор: Андрей Карпов

Я в шоке от возможностей статического анализа кода, хотя сам участвую в разработке инструмента PVS-Studio. На днях я был искренне удивлён тому, что анализатор оказался умнее и внимательнее меня. Рабо…
PVS-Studio для Java

Дата: 17 Янв 2019

Автор: Андрей Карпов

В седьмой версии статического анализатора PVS-Studio мы добавили поддержку языка Java. Пришло время немного рассказать, как мы начинали делать поддержку языка Java, что у нас получилось и какие дальн…
Характеристики анализатора PVS-Studio на примере EFL Core Libraries, 10-15% ложных срабатываний

Дата: 31 Июл 2017

Автор: Андрей Карпов

После большой статьи про проверку операционной системы Tizen мне было задано много вопросов о проценте ложных срабатываний и о плотности ошибок (сколько ошибок PVS-Studio выявляет на 1000 строк кода)…
Бесплатный PVS-Studio для тех, кто развивает открытые проекты

Дата: 22 Дек 2018

Автор: Андрей Карпов

В канун празднования нового 2019 года команда PVS-Studio решила сделать приятный подарок всем контрибьюторам open-source проектов, хостящихся на GitHub, GitLab или Bitbucket. Им предоставляется возмо…
Технологии, используемые в анализаторе кода PVS-Studio для поиска ошибок и потенциальных уязвимостей

Дата: 21 Ноя 2018

Автор: Андрей Карпов

Краткое описание технологий, используемых в инструменте PVS-Studio, которые позволяют эффективно обнаруживать большое количество паттернов ошибок и потенциальных уязвимостей. Статья описывает реализа…
PVS-Studio ROI

Дата: 30 Янв 2019

Автор: Андрей Карпов

Время от времени нам задают вопрос, какую пользу в денежном эквиваленте получит компания от использования анализатора PVS-Studio. Мы решили оформить ответ в виде статьи и привести таблицы, которые по…
Как PVS-Studio оказался внимательнее, чем три с половиной программиста

Дата: 22 Окт 2018

Автор: Андрей Карпов

PVS-Studio, как и другие статические анализаторы кода, часто выдаёт ложные срабатывания. Но не стоит спешить считать странные срабатывания ложными. Это короткая история о том, как PVS-Studio вновь ок…

Комментарии (0)

Следующие комментарии
Этот сайт использует куки и другие технологии, чтобы предоставить вам более персонализированный опыт. Продолжая просмотр страниц нашего веб-сайта, вы принимаете условия использования этих файлов. Если вы не хотите, чтобы ваши данные обрабатывались, пожалуйста, покиньте данный сайт. Подробнее →
Принять