Когда мы решили поискать ошибки в проекте Azure SDK для .
NET, мы были приятно удивлены его размером.
«Три с половиной миллиона строк кода», — сказали мы, изучая статистику проекта.
Вот сколько всего можно там найти.
Но, увы и ах.
Проект оказался секретным.
В чем особенность проекта и как он тестировался — читайте в этой статье.
о проекте
Я пишу эту статью в продолжение моей предыдущей статьи, которая также была посвящена проекту, связанному с Microsoft Azure: Azure PowerShell: «в основном безвреден» .Итак, в этот раз я рассчитывал на значительное количество разнообразных и интересных ошибок.
Ведь для статического анализа обычно важен размер проекта, особенно при разовых проверках всего проекта.
Да, на практике этого обычно не делают. А если и будут, то только на этапе внедрения анализатора.
При этом никто сразу не понимает огромного количества тревог (значительное количество предупреждений является нормой при работе анализатора в этом режиме), а просто откладывает их как технический долг, используя механизмы подавления сообщений и хранения их в специальных базы данных (массовое подавление).
Мы занимаемся разовыми проверками в исследовательских целях.
Поэтому крупные учебные проекты всегда предпочтительнее мелких.
Однако проект Azure SDK для .
NET сразу показал свою несостоятельность в качестве тестового стенда.
Даже его внушительные размеры не помогли, а скорее даже усложнили работу.
Причина указана в следующей статистике проекта:
- Файлы исходного кода .
cs (не включая тесты): 16 500.
- Решения Visual Studio (.
sln): 163
- Непустые строки кода: 3 462 000.
- Из них автоматически сгенерированных: около 3 300 000
- Репозиторий проекта доступен по адресу GitHub .
Проверка таких проектов статическим анализатором обычно трудоемка и бесполезна, так как там много рабочего, но нелогичного (на первый взгляд) и избыточного кода.
Это приводит к большому количеству ложных срабатываний.
Вишенкой на торте было большое количество решений Visual Studio (163), на которых была «размазана» эта масса кода.
Поэтому потребовались некоторые усилия, чтобы проверить оставшийся код (не сгенерированный автоматически).
Помогает то, что весь автоматически сгенерированный код находится в подпапках решения по относительному пути " \src\Generated».
Кроме того, каждый CS-файл такого кода содержит специальный комментарий в теге :
Для чистоты эксперимента я случайным образом протестировал около десяти случайно выбранных решений с автоматически сгенерированным кодом.// <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 для .
NET.
Microsoft.Azure.Management.Advisor
Это лишь одно из многих решений, содержащих автоматически сгенерированный код. Как я уже говорил выше, выборочно было опробовано около десятка таких решений.И везде сообщения были одинаковыми и, как и ожидалось, бесполезными.
Позвольте мне привести вам пару примеров таких триггеров.
В3022 Выражение «Учетные данные!= null» всегда истинно.
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);
}
}
Очевидно, что код избыточен, и проверка Учетные данные != ноль бесполезный.
Но код работает. И генерируется автоматически.
Поэтому никаких претензий.
В3022 Выражение «_queryParameters.Count > 0» всегда является ложным.
КонфигурацииOperations.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
В3001 Слева и справа от 'buffer.Length' имеются идентичные подвыражения 'buffer.Length'.
<' 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."); .
}
В условии допущена ошибка, вероятно, в результате использования копипаста.
Судя по тому, что следует в коде буфер скопировано в множество , проверка должна выглядеть так: if (array == null || array.Length < buffer.Length)
Но, как я всегда говорю, такие ошибки должен исправлять автор кода.
В3083 Возможен небезопасный вызов события _onChange, NullReferenceException. Рассмотрите возможность присвоения события локальной переменной перед ее вызовом.
ClientOptionsMonitor.cs 44 private event Action<TOptions, string> _onChange;
.
private void InvokeChanged(.
) { .
if (_onChange != null)
{
_onChange.Invoke(options, name);
}
}
Не критично, но ошибка.
Между проверкой события на равенство нулевой и с его вызовом им удастся отписаться от события.
Тогда переменная _по изменению получит значение нулевой , и будет выброшено исключение.
Код необходимо переписать более безопасно.
Например, вот так: private void InvokeChanged(.
) { .
_onChange?.
Invoke(options, name);
}
Azure.Messaging.EventHubs
В3080 Возможное нулевое разыменование.
Рассмотрите возможность проверки 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);
}
Давайте проследим, что происходит со значением переменной событиесвойствозначение в данном фрагменте кода.
В начале метода присваивается переменная нулевой .
Далее, в одном из условий первого выключатель , переменная инициализируется, после чего метод завершается.
Второй блок выключатель содержит множество условий, в каждом из которых переменная также получает новое значение.
Но в блоке по умолчанию переменная событиесвойствозначение они просто используются без какой-либо проверки, что является ошибкой, так как на данный момент переменная равна нулевой .
В3066 Возможный неправильный порядок аргументов, передаваемых в конструктор EventHubConsumer: «partitionId» и «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 .
В связи с тем, что первый параметр транспортПотребительский расположен в той же строке, что и имя класса, при беглом взгляде на код может показаться, что параметр идентификатор раздела находится на третьем месте, а не на четвёртом (моих комментариев с порядковыми номерами параметров нет в исходном коде).
Это всего лишь предположение, но я бы изменил форматирование кода объявления конструктора примерно на такое: internal EventHubConsumer
(
TransportEventHubConsumer transportConsumer,
string eventHubName,
string consumerGroup,
string partitionId,
EventPosition eventPosition,
EventHubConsumerOptions consumerOptions,
EventHubRetryPolicy retryPolicy)
{
.
}
Azure.Хранилище
В3112 Ненормальность среди подобных сравнений.Возможно, внутри выражения «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;
}
Ошибка, допущенная по невнимательности.
Обнаружить такую ошибку при проверке кода довольно сложно.
Правильный вариант проверки: .
ContentEncoding == other.ContentEncoding && ContentLanguage == other.ContentLanguage && .
В3112 Ненормальность среди подобных сравнений.
Возможно, внутри выражения «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
В3053 Чрезмерное выражение.Проверьте подстроки «IList» и «List».
PropertyData.cs 157 В3053 Чрезмерное выражение.
Проверьте подстроки List и 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"); // <=
}
Анализатор выдал два предупреждения о бессмысленных или ошибочных проверках.
В первом случае поиск подстроки «Список» после поиска «IList» выглядит избыточным.
Действительно, условие: this.Type.Contains("IList") || this.Type.Contains("List")
можно заменить на это: this.Type.Contains("List")
Во втором случае поиск подстроки «IReadOnlyList» бессмысленен, так как предварительно ищется более короткая подстрока «List».
Также есть вероятность, что в самих подстроках поиска есть ошибки и там должно быть что-то еще.
В любом случае правильный вариант исправления условия с учетом обоих комментариев может предложить только автор кода.
В3095 Объект «httpRequest.Content.Headers» использовался до того, как он был проверен на значение null. Проверьте строки: 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 используются без каких-либо проверок, но позже в коде доступ к этой переменной осуществляется с помощью оператора условного доступа.
В3125 Объект «omPropertyData» использовался после проверки его соответствия нулю.
Проверьте строки: 156, 148. CodeGenerationUtilities.cs 156 private static string GetProtocolCollectionToObjectModelCollectionString(
.
, PropertyData omPropertyData, .
) { if (IsMappedEnumPair(omPropertyData?.
GenericTypeParameter, .
)) { .
} if (IsTypeComplex(omPropertyData.GenericTypeParameter)) .
}
Противоположная ситуация.
Один блок кода содержит безопасную опцию доступа к потенциально нулевой ссылке.
омPropertyData .
Далее в коде они работают с одной и той же ссылкой без каких-либо проверок.
В3146 Возможное нулевое разыменование «значения».
«FirstOrDefault» может возвращать нулевое значение по умолчанию.
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(); .
} .
}
В результате метода ФёрстОрдефолт если поиск не удался, будет возвращено значение по умолчанию для типа.
нить , то есть нулевой .
Значение будет присвоено переменной ценить , который далее используется в коде методом Заменять без всяких проверок.
Код следует сделать более безопасным.
Например, вот так: foreach (string canonicalHeader in customHeaders)
{
string value = httpRequest.Headers.
GetValues(canonicalHeader).
FirstOrDefault(); value = value?.
Replace('\n', ' ').
Replace('\r', ' ').
TrimStart(); .
}
Microsoft.Azure.ServiceBus
В3121 Перечисление BlocksUsing было объявлено с атрибутом Flags, но не устанавливает никаких инициализаторов для переопределения значений по умолчанию.
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, } .
} .
}
Перечисление объявлено с атрибутом Флаги .
При этом значения констант оставляются по умолчанию ( Мониторэнтер = 0 , МониторОжидание = 1 , РучноеResetEvent = 2 и так далее).
Это может привести к тому, что при попытке использовать комбинации флаги, например, вторая и третья константы МониторОжидание (=1) | РуководствоResetEvent (=2) , результатом будет не уникальное значение, а константа со значением по умолчанию 3 ( AutoResetEvent ).
Это может стать неожиданностью для вызывающего кода.
Если передача БлокиИспользование Если вы действительно планируете использовать его для указания комбинаций флагов (битового поля), вам следует присвоить константам значения, равные степеням двойки: [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,
}
В3125 Объект «сессия» использовался после проверки его соответствия нулю.
Проверьте строки: 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()); } .
}
Обратите внимание на работу с переменной сессия в блоке ловить .
Вызов метода Аборт осуществляется безопасно через оператор условного доступа.
Но затем они делают небезопасный вызов метода GetInnerException .
В этом случае вместо исключения ожидаемого типа может быть выброшено исключение.
NullReferenceException .
Код необходимо исправить.
Метод AmqpExceptionHelper.GetClientException поддерживает передачу значений нулевой для параметра внутреннееисключение : public static Exception GetClientException(
Exception exception,
string referenceId = null,
Exception innerException = null,
bool connectionError = false)
{
.
}
Поэтому достаточно использовать оператор условного доступа при вызове сеанс.
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 для .
NET. Да, это требует дополнительных усилий, но тем приятнее будет результат. А чтобы вам не приходилось прилагать чрезмерных усилий, мы рекомендуем использовать статический анализ и на рабочих местах разработчиков при написании нового кода.
Это наиболее эффективный подход. Скачайте и попробуйте PVS-Studio в бизнесе.
Удачи в борьбе с ошибками! 
Если вы хотите поделиться этой статьей с англоязычной аудиторией, воспользуйтесь ссылкой для перевода: Сергей Хренов.
Azure SDK для .
NET: история о сложном поиске ошибок .
Теги: #Разработка для Windows #microsoft #Microsoft Azure #C++ #.
NET #Visual Studio #azure #pvs-studio #windows form
-
Хватит Крутить (Восклицательный Знак)
19 Oct, 24 -
Профилирование Приложений Python
19 Oct, 24 -
Несколько Слов О Великих И Могучих.
19 Oct, 24
