Приветствую всех любителей покритиковать чужой код. :) Сегодня в нашей лаборатории новый материал для исследования - исходный код проекта AWS SDK для .NET. В своё время мы писали статью о проверке AWS SDK для C++. Тогда не нашлось ничего особо интересного. Посмотрим, чем нас порадует .NET версия AWS SDK. Хорошая возможность в очередной раз продемонстрировать возможности анализатора PVS-Studio, а также сделать мир немного совершеннее.
Amazon Web Services (AWS) SDK для .NET - это набор инструментов разработчика, предназначенный для создания приложений на основе .NET в инфраструктуре AWS и позволяющий значительно упростить процесс написания кода. SDK включает в себя наборы API .NET для различных сервисов AWS, таких как Amazon S3, Amazon EC2, DynamoDB и других. Исходный код SDK размещён на GitHub.
Как я уже говорил, в своё время мы писали статью о проверке AWS SDK для C++. Статья получилась небольшая - на 512 тысяч строк кода нашлась всего пара ошибок. В этот раз мы имеем дело с гораздо большим объёмом кода, включающим в себя около 34 тысяч cs-файлов, а общее число строк кода (без учета пустых) составляет впечатляющие 5 миллионов. Незначительная часть кода (200 тысяч строк в 664 cs-файлах) приходится на тесты, их я не рассматривал.
Если качество кода .NET версии SDK приблизительно такое же, как у C++ (две ошибки на 512 KLOC), то мы должны получить примерно в 10 раз большее число ошибок. Это, конечно, очень неточная методика подсчёта, не учитывающая языковые особенности и ещё множество других факторов, но не думаю, что читателю сейчас хочется углубляться в скучные рассуждения. Вместо этого предлагаю сразу перейти к результатам.
Проверка производилась при помощи PVS-Studio версии 6.27. Невероятно, но в коде AWS SDK для .NET анализатору удалось обнаружить около 40 ошибок, о которых стоило бы рассказать. Это говорит не только о высоком качестве кода SDK (примерно 4 ошибки на 512 KLOC), но и о сопоставимом качестве работы C# анализатора PVS-Studio в сравнении с С++. Отличный результат!
Авторы AWS SDK для .NET - большие молодцы. От проекта к проекту они демонстрируют потрясающее качество кода. Это может служить хорошим примером для других команд. Но, конечно, я не был бы разработчиком статического анализатора, если бы не вставил свои 5 копеек. :) Мы уже взаимодействуем с командой Lumberyard из Amazon по вопросам использования PVS-Studio. Но так как это очень большая компания с кучей подразделений по миру, то весьма вероятно, что команда AWS SDK для .NET вообще никогда не слышала о PVS-Studio. Во всяком случае, в коде SDK я не нашёл никаких признаков использования нашего анализатора, хотя это ни о чем и не говорит. Тем не менее, команда, как минимум, использует анализатор, встроенный в Visual Studio. Это хорошо, но проверки кода можно усилить :).
Как итог, мне все же удалось найти несколько ошибок в коде SDK и, наконец, пришло время ими поделиться.
Ошибка в логике
Предупреждение PVS-Studio: V3008 [CWE-563] The 'this.linker.s3.region' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 116, 114. AWSSDK.DynamoDBv2.Net45 S3Link.cs 116
public string Region
{
get
{
....
}
set
{
if (String.IsNullOrEmpty(value))
{
this.linker.s3.region = "us-east-1";
}
this.linker.s3.region = value;
}
}Анализатор предупреждает о повторном присваивании значения одной и той же переменной. Из кода становится понятно, что это связано с ошибкой, которая нарушает логику работы: значение переменной this.linker.s3.region всегда будет равно value, независимо от условия if (String.IsNullOrEmpty(value)). В теле блока if был пропущен return. Код необходимо исправить следующим образом:
public string Region
{
get
{
....
}
set
{
if (String.IsNullOrEmpty(value))
{
this.linker.s3.region = "us-east-1";
return;
}
this.linker.s3.region = value;
}
}Бесконечная рекурсия
Предупреждение PVS-Studio: V3110 [CWE-674] Possible infinite recursion inside 'OnFailure' property. AWSSDK.ElasticMapReduce.Net45 ResizeJobFlowStep.cs 171
OnFailure? onFailure = null;
public OnFailure? OnFailure
{
get { return this.OnFailure; } // <=
set { this.onFailure = value; }
}Классический пример опечатки, которая приводит к возникновению бесконечной рекурсии в методе доступа get свойства OnFailure. Вместо возврата значения приватного поля onFailure обращаются к свойству OnFailure. Исправленный вариант:
public OnFailure? OnFailure
{
get { return this.onFailure; }
set { this.onFailure = value; }
}Вы спросите: "Как это работало?" Пока - никак. Свойство нигде не используется, но это временное явление. В один прекрасный момент кто-то начнёт его использовать и обязательно получит непредвиденный результат. Для предотвращения таких опечаток рекомендуется не использовать идентификаторы, различающиеся только регистром первой буквы.
Ещё одно замечание к данной конструкции - использование идентификатора, полностью совпадающего с именем типа OnFailure. С точки зрения компилятора это вполне допустимо, но затрудняет восприятие кода человеком.
Другая подобная ошибка:
Предупреждение PVS-Studio: V3110 [CWE-674] Possible infinite recursion inside 'SSES3' property. AWSSDK.S3.Net45 InventoryEncryption.cs 37
private SSES3 sSES3;
public SSES3 SSES3
{
get { return this.SSES3; }
set { this.SSES3 = value; }
}Ситуация идентична описанной выше. Только здесь бесконечная рекурсия возникнет при обращении к свойству SSES3 как на чтение, так и на запись. Исправленный вариант:
public SSES3 SSES3
{
get { return this.sSES3; }
set { this.sSES3 = value; }
}Тест на внимательность
Далее следует задание от программиста, увлечённого использованием методики Copy-Paste. Посмотрите на то, как код выглядит в редакторе Visual Studio, и попробуйте найти ошибку (кликните на картинку для увеличения).
Предупреждение PVS-Studio: V3029 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 91, 95. AWSSDK.AppSync.Net45 CreateApiKeyResponseUnmarshaller.cs 91
Я сократил тело метода UnmarshallException, убрав всё лишнее. Теперь видно, что идентичные проверки следуют одна за другой:
public override AmazonServiceException UnmarshallException(....)
{
....
if (errorResponse.Code != null &&
errorResponse.Code.Equals("LimitExceededException"))
{
return new LimitExceededException(errorResponse.Message,
innerException, errorResponse.Type, errorResponse.Code,
errorResponse.RequestId, statusCode);
}
if (errorResponse.Code != null &&
errorResponse.Code.Equals("LimitExceededException"))
{
return new LimitExceededException(errorResponse.Message,
innerException, errorResponse.Type, errorResponse.Code,
errorResponse.RequestId, statusCode);
}
....
}Может показаться, что ошибка не грубая - просто лишняя проверка. Но часто такой паттерн может свидетельствовать о более серьезных проблемах в коде, когда не будет выполнена какая-то нужная проверка.
В коде есть ещё несколько подобных ошибок.
Предупреждения PVS-Studio:
Что ты такое?
Предупреждение PVS-Studio: V3062 An object 'attributeName' is used as an argument to its own method. Consider checking the first actual argument of the 'Contains' method. AWSSDK.MobileAnalytics.Net45 CustomEvent.cs 261
/// <summary>
/// Dictionary that stores attribute for this event only.
/// </summary>
private Dictionary<string,string> _attributes =
new Dictionary<string,string>();
/// <summary>
/// Gets the attribute.
/// </summary>
/// <param name="attributeName">Attribute name.</param>
/// <returns>The attribute. Return null of attribute doesn't
/// exist.</returns>
public string GetAttribute(string attributeName)
{
if(string.IsNullOrEmpty(attributeName))
{
throw new ArgumentNullException("attributeName");
}
string ret = null;
lock(_lock)
{
if(attributeName.Contains(attributeName)) // <=
ret = _attributes[attributeName];
}
return ret;
}Анализатор обнаружил ошибку в методе GetAttribute: строку проверяют на то, что она содержит саму себя. Из описания к методу следует, что если имя атрибута (ключ attributeName) будет найдено (в словаре _attributes), то следует вернуть значение атрибута, иначе - null. В действительности же, так как условие attributeName.Contains(attributeName) всегда истинно, производится попытка возврата значения по ключу, который может быть не найден в словаре. Тогда вместо возврата null будет выброшено исключение KeyNotFoundException.
Давайте попробуем исправить данный код. Чтобы лучше понять, как это сделать, следует взглянуть на другой метод:
/// <summary>
/// Determines whether this instance has attribute the specified
/// attributeName.
/// </summary>
/// <param name="attributeName">Attribute name.</param>
/// <returns>Return true if the event has the attribute, else
/// false.</returns>
public bool HasAttribute(string attributeName)
{
if(string.IsNullOrEmpty(attributeName))
{
throw new ArgumentNullException("attributeName");
}
bool ret = false;
lock(_lock)
{
ret = _attributes.ContainsKey(attributeName);
}
return ret;
}Данный метод проверяет, существует ли имя атрибута (ключ attributeName) в словаре _attributes. Снова вернёмся к методу GetAttribute и исправим ошибку:
public string GetAttribute(string attributeName)
{
if(string.IsNullOrEmpty(attributeName))
{
throw new ArgumentNullException("attributeName");
}
string ret = null;
lock(_lock)
{
if(_attributes.ContainsKey(attributeName))
ret = _attributes[attributeName];
}
return ret;
}Теперь метод делает именно то, что заявлено в описании.
И ещё одно небольшое замечание к данному фрагменту кода. Я заметил, что авторы используют lock при работе со словарем _attributes. Ясно, что это необходимо при многопоточном доступе, но конструкция lock достаточно медленна и громоздка. Вместо Dictionary в данном случае, возможно, удобнее было бы использовать потокобезопасный вариант словаря - ConcurrentDictionary. Тогда необходимость в lock отпадает. Хотя, возможно я не знаю о каких-то особенностях проекта.
Подозрительное поведение
Предупреждение PVS-Studio: V3063 [CWE-571] A part of conditional expression is always true if it is evaluated: string.IsNullOrEmpty(inferredIndexName). AWSSDK.DynamoDBv2.PCL ContextInternal.cs 802
private static string GetQueryIndexName(....)
{
....
string inferredIndexName = null;
if (string.IsNullOrEmpty(specifiedIndexName) &&
indexNames.Count == 1)
{
inferredIndexName = indexNames[0];
}
else if (indexNames.Contains(specifiedIndexName,
StringComparer.Ordinal))
{
inferredIndexName = specifiedIndexName;
}
else if (string.IsNullOrEmpty(inferredIndexName) && // <=
indexNames.Count > 0)
throw new InvalidOperationException("Local Secondary Index range
key conditions are used but no index could be inferred from
model. Specified index name = " + specifiedIndexName);
....
}Анализатор насторожила проверка string.IsNullOrEmpty(inferredIndexName). Действительно, строке inferredIndexName присваивают null, далее значение этой переменной нигде не изменяется, а потом её зачем-то проверяют на null или пустую строку. Выглядит подозрительно. Давайте внимательно посмотрим на приведённый фрагмент кода. Я умышленно не стал его сокращать для лучшего понимания ситуации. Итак, в первом операторе if (а также в следующем), некоторым образом проверяют переменную specifiedIndexName. В зависимости от результата проверок, переменная inferredIndexName получает новое значение. Теперь обратим внимание на третий оператор if. Тело этого оператора (выброс исключения) будет выполнено в случае indexNames.Count > 0, так как первая часть условия - string.IsNullOrEmpty(inferredIndexName) - всегда истина. Возможно, просто перепутаны переменные specifiedIndexName и inferredIndexName. Или же третья проверка должна быть без else, представляя собой автономный оператор if:
if (string.IsNullOrEmpty(specifiedIndexName) &&
indexNames.Count == 1)
{
inferredIndexName = indexNames[0];
}
else if (indexNames.Contains(specifiedIndexName,
StringComparer.Ordinal))
{
inferredIndexName = specifiedIndexName;
}
if (string.IsNullOrEmpty(inferredIndexName) &&
indexNames.Count > 0)
throw new InvalidOperationException(....);В данном случае сложно дать однозначный ответ о вариантах исправления этого кода. Но определённо необходимо, чтобы автор его проинспектировал.
NullReferenceException
Предупреждение PVS-Studio: V3095 [CWE-476] The 'conditionValues' object was used before it was verified against null. Check lines: 228, 238. AWSSDK.Core.Net45 JsonPolicyWriter.cs 228
private static void writeConditions(....)
{
....
foreach (....)
{
IList<string> conditionValues = keyEntry.Value;
if (conditionValues.Count == 0) // <=
continue;
....
if (conditionValues != null && conditionValues.Count != 0)
{
....
}
....
}
}Классика жанра. Переменную conditionValues используют без предварительной проверки на равенство null. При этом далее в коде такая проверка выполняется, но уже поздно. :) Код можно исправить следующим образом:
private static void writeConditions(....)
{
....
foreach (....)
{
IList<string> conditionValues = keyEntry.Value;
if (conditionValues != null && conditionValues.Count == 0)
continue;
....
if (conditionValues != null && conditionValues.Count != 0)
{
....
}
....
}
}В коде было найдено ещё несколько подобных ошибок.
Предупреждения PVS-Studio:
Следующее предупреждение очень похоже по смыслу, но ситуация обратна рассмотренной выше.
Предупреждение PVS-Studio: V3125 [CWE-476] The 'state' object was used after it was verified against null. Check lines: 139, 127. AWSSDK.Core.Net45 RefreshingAWSCredentials.cs 139
private void UpdateToGeneratedCredentials(
CredentialsRefreshState state)
{
string errorMessage;
if (ShouldUpdate)
{
....
if (state == null)
errorMessage = "Unable to generate temporary credentials";
else
....
throw new AmazonClientException(errorMessage);
}
state.Expiration -= PreemptExpiryTime; // <=
....
}Один из фрагментов кода содержит проверку значения переменной state на равенство null. Далее по коду переменную state используют для того, чтобы отписаться от события PreemptExpiryTime, при этом проверка на равенство null уже не производится, и возможен выброс исключения NullReferenceException. Более безопасный вариант кода:
private void UpdateToGeneratedCredentials(
CredentialsRefreshState state)
{
string errorMessage;
if (ShouldUpdate)
{
....
if (state == null)
errorMessage = "Unable to generate temporary credentials";
else
....
throw new AmazonClientException(errorMessage);
}
if (state != null)
state.Expiration -= PreemptExpiryTime;
....
}В коде есть и другие подобные ошибки.
Предупреждения PVS-Studio:
Безальтернативная реальность
Предупреждение PVS-Studio: V3009 [CWE-393] It's odd that this method always returns one and the same value of 'true'. AWSSDK.Core.Net45 Lexer.cs 651
private static bool State19 (....)
{
while (....) {
switch (....) {
case '"':
....
return true;
case '\\':
....
return true;
default:
....
continue;
}
}
return true;
}Метод всегда возвращает true. Давайте посмотрим, насколько это критично для вызывающего кода. Я отследил использование метода State19. Он участвует в заполнении массива обработчиков fsm_handler_table наравне с другими подобными методами (их 28 с именами, соответственно, от State1 до State28). Здесь важно отметить, что помимо State19, для некоторых других обработчиков также были выданы предупреждения V3009 [CWE-393]. Это обработчики: State23, State26, State27, State28. Предупреждения, выданные анализатором для них:
Вот как выглядит объявление и инициализация массива обработчиков:
private static StateHandler[] fsm_handler_table;
....
private static void PopulateFsmTables ()
{
fsm_handler_table = new StateHandler[28] {
State1,
State2,
....
State19,
....
State23,
....
State26,
State27,
State28
};Для полноты картины - посмотрим код одного из обработчиков, к которому у анализатора не было претензий, например, State2:
private static bool State2 (....)
{
....
if (....) {
return true;
}
switch (....) {
....
default:
return false;
}
}А вот так происходит вызов обработчиков:
public bool NextToken ()
{
....
while (true) {
handler = fsm_handler_table[state - 1];
if (! handler (fsm_context)) // <=
throw new JsonException (input_char);
....
}
....
}Как видим, исключение будет выброшено в случае возврата false. В нашем случае, для обработчиков State19, State23, State26, State27 и State28 этого не произойдёт никогда. Выглядит подозрительно. С другой стороны, целых пять обработчиков имеют схожее поведение (всегда вернут true), так что, возможно, это так было задумано и не является следствием опечатки.
Зачем я на всем этом так подробно остановился? Данная ситуация очень показательна в том плане, что статический анализатор часто способен лишь указать на подозрительную конструкцию. И даже человек (не машина), не обладающий достаточными знаниями о проекте, потратив время на изучение кода, всё равно не способен дать исчерпывающего ответа о наличии ошибки. Код необходимо изучить разработчику.
Бессмысленные проверки
Предупреждение PVS-Studio: V3022 [CWE-571] Expression 'doLog' is always true. AWSSDK.Core.Net45 StoredProfileAWSCredentials.cs 235
private static bool ValidCredentialsExistInSharedFile(....)
{
....
var doLog = false;
try
{
if (....)
{
return true;
}
else
{
doLog = true;
}
}
catch (InvalidDataException)
{
doLog = true;
}
if (doLog) // <=
{
....
}
....
}Обратите внимание на переменную doLog. После инициализации значением false, далее в коде эта переменная получит значение true во всех случаях. Таким образом, проверка if (doLog) всегда истинна. Возможно, ранее в данном методе существовала ветка, в которой переменной doLog не присваивалось никакого значения, тогда на момент проверки она могла содержать значение false, полученное при инициализации. Но теперь такой ветки нет.
Ещё одна подобная ошибка:
Предупреждение PVS-Studio: V3022 Expression '!result' is always false. AWSSDK.CognitoSync.PCL SQLiteLocalStorage.cs 353
public void PutValue(....)
{
....
bool result = PutValueHelper(....);
if (!result) <=
{
_logger.DebugFormat("{0}",
@"Cognito Sync - SQLiteStorage - Put Value Failed");
}
else
{
UpdateLastModifiedTimestamp(....);
}
....
}Анализатор утверждает, что значение переменной result всегда true. Такое возможно лишь в случае, если метод PutValueHelper будет всегда возвращать true. Посмотрим на этот метод:
private bool PutValueHelper(....)
{
....
if (....))
{
return true;
}
if (record == null)
{
....
return true;
}
else
{
....
return true;
}
}Действительно, метод вернёт true при любом условии. Более того, анализатор выдал для этого метода предупреждение. Предупреждение PVS-Studio: V3009 [CWE-393] It's odd that this method always returns one and the same value of 'true'. SQLiteLocalStorage.cs 1016
Я умышленно не стал приводить это предупреждение ранее, когда рассматривал другие ошибки V3009, а приберёг его для этого случая. Таким образом, анализатор был прав, указав на ошибку V3022 в вызывающем коде.
Copy-Paste. Again
Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'this.token == JsonToken.String' to the left and to the right of the '||' operator. AWSSDK.Core.Net45 JsonReader.cs 343
public bool Read()
{
....
if (
(this.token == JsonToken.ObjectEnd ||
this.token == JsonToken.ArrayEnd ||
this.token == JsonToken.String || // <=
this.token == JsonToken.Boolean ||
this.token == JsonToken.Double ||
this.token == JsonToken.Int ||
this.token == JsonToken.UInt ||
this.token == JsonToken.Long ||
this.token == JsonToken.ULong ||
this.token == JsonToken.Null ||
this.token == JsonToken.String // <=
))
{
....
}
....
}Дважды сравнивают поле this.token со значением JsonToken.String перечисления JsonToken. Вероятно, одно из сравнений должно содержать другое значение перечисления. Если это так, то допущена серьёзная ошибка.
Рефакторинг + невнимательность?
Предупреждение PVS-Studio: V3025 [CWE-685] Incorrect format. A different number of format items is expected while calling 'Format' function. Arguments not used: AWSConfigs.AWSRegionKey. AWSSDK.Core.Net45 AWSRegion.cs 116
public InstanceProfileAWSRegion()
{
....
if (region == null)
{
throw new InvalidOperationException(
string.Format(CultureInfo.InvariantCulture,
"EC2 instance metadata was not available or did not contain
region information.",
AWSConfigs.AWSRegionKey));
}
....
}Вероятно, строка формата для метода string.Format ранее содержала спецификатор вывода {0}, для которого и был задан аргумент AWSConfigs.AWSRegionKey. Затем строку изменили, спецификатор пропал, а вот аргумент удалить забыли. Приведённый фрагмент кода работает без ошибок (исключение было бы выброшено в обратном случае - спецификатор без аргумента), но выглядит некрасиво. Код следует исправить следующим образом:
if (region == null)
{
throw new InvalidOperationException(
"EC2 instance metadata was not available or did not contain
region information.");
}Unsafe
Предупреждение PVS-Studio: V3083 [CWE-367] Unsafe invocation of event 'mOnSyncSuccess', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. AWSSDK.CognitoSync.PCL Dataset.cs 827
protected void FireSyncSuccessEvent(List<Record> records)
{
if (mOnSyncSuccess != null)
{
mOnSyncSuccess(this, new SyncSuccessEventArgs(records));
}
}Достаточно распространённая ситуация небезопасного вызова обработчика события. Между проверкой переменной mOnSyncSuccess на равенство null и вызовом обработчика от события могут успеть отписаться, и его значение станет нулевым. Вероятность такого сценария мала, но код всё же лучше сделать более безопасным:
protected void FireSyncSuccessEvent(List<Record> records)
{
mOnSyncSuccess?.Invoke(this, new SyncSuccessEventArgs(records));
}В коде есть и другие подобные ошибки.
Предупреждения PVS-Studio:
Недоработанный класс
Предупреждение PVS-Studio: V3126 Type 'JsonData' implementing IEquatable<T> interface does not override 'GetHashCode' method. AWSSDK.Core.Net45 JsonData.cs 26
public class JsonData : IJsonWrapper, IEquatable<JsonData>
{
....
}Класс JsonData содержит достаточно много кода, поэтому я не стал приводить его целиком, ограничившись только объявлением. Этот класс действительно не содержит переопределённого метода GetHashCode, что небезопасно, так как может привести к ошибочному поведению при использовании типа JsonData для работы, например, с коллекциями. Вероятно, в настоящее время проблемы нет, но в дальнейшем стратегия использования этого типа может измениться. Более подробно данная ошибка описана в документации.
Заключение
Вот и все интересные ошибки, которые мне удалось обнаружить в коде AWS SDK для .NET при помощи статического анализатора PVS-Studio. Ещё раз подчеркну качество проекта. Я обнаружил весьма небольшое число ошибок для 5 миллионов строк кода. Хотя, вероятно, более тщательный анализ выданных предупреждений позволил бы мне добавить к этому списку ещё несколько ошибок. Но также весьма вероятно и то, что я зря причислил некоторые из описанных предупреждений к ошибкам. Однозначные выводы в таком случае всегда делает только разработчик, который находится в контексте проверяемого кода.