# Ищем ошибки в исходном коде Amazon Web Services SDK для \.NET Приветствую всех любителей покритиковать чужой код\. :\) Сегодня в нашей лаборатории новый материал для исследования \- исходный код проекта AWS SDK для \.NET\. В своё время мы писали статью о проверке AWS SDK для C\+\+\. Тогда не нашлось ничего особо интересного\. Посмотрим, чем нас порадует \.NET версия AWS SDK\. Хорошая возможность в очередной раз продемонстрировать возможности анализатора PVS\-Studio, а также сделать мир немного совершеннее\. ![0605_AWS_SDK_NET_ru/image1.png](https://import.viva64.com/docx/blog/0605_AWS_SDK_NET_ru/image1.png) Amazon Web Services \(AWS\) SDK для \.NET \- это набор инструментов разработчика, предназначенный для создания приложений на основе \.NET в инфраструктуре AWS и позволяющий значительно упростить процесс написания кода\. SDK включает в себя наборы API \.NET для различных сервисов AWS, таких как Amazon S3, Amazon EC2, DynamoDB и других\. Исходный код SDK размещён на [GitHub](https://github.com/aws/aws-sdk-net)\. Как я уже говорил, в своё время мы писали [статью](https://pvs-studio.ru/ru/blog/posts/0378/) о проверке 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 я не нашёл никаких признаков использования нашего анализатора, хотя это ни о чем и не говорит\. Тем не менее, команда, как минимум, [использует](https://aws.amazon.com/ru/blogs/developer/code-analyzers-added-to-aws-sdk-for-net/) анализатор, встроенный в Visual Studio\. Это хорошо, но проверки кода можно [усилить](https://pvs-studio.ru/ru/blog/posts/csharp/0506/) :\)\. Как итог, мне все же удалось найти несколько ошибок в коде SDK и, наконец, пришло время ими поделиться\. **Ошибка в логике** **Предупреждение PVS\-Studio:** [V3008](https://pvs-studio.ru/ru/docs/warnings/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 ```cpp 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_\. Код необходимо исправить следующим образом: ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3110/) \[CWE\-674\] Possible infinite recursion inside 'OnFailure' property\. AWSSDK\.ElasticMapReduce\.Net45 ResizeJobFlowStep\.cs 171 ```cpp OnFailure? onFailure = null; public OnFailure? OnFailure { get { return this.OnFailure; } // <= set { this.onFailure = value; } } ``` Классический пример опечатки, которая приводит к возникновению бесконечной рекурсии в методе доступа _get_ свойства _OnFailure_\. Вместо возврата значения приватного поля _onFailure_ обращаются к свойству _OnFailure_\. Исправленный вариант: ```cpp public OnFailure? OnFailure { get { return this.onFailure; } set { this.onFailure = value; } } ``` Вы спросите: "Как это работало?" Пока \- никак\. Свойство нигде не используется, но это временное явление\. В один прекрасный момент кто\-то начнёт его использовать и обязательно получит непредвиденный результат\. Для предотвращения таких опечаток рекомендуется не использовать идентификаторы, различающиеся только регистром первой буквы\. Ещё одно замечание к данной конструкции \- использование идентификатора, полностью совпадающего с именем типа _OnFailure_\. С точки зрения компилятора это вполне допустимо, но затрудняет восприятие кода человеком\. Другая подобная ошибка: **Предупреждение PVS\-Studio:** [V3110](https://pvs-studio.ru/ru/docs/warnings/v3110/) \[CWE\-674\] Possible infinite recursion inside 'SSES3' property\. AWSSDK\.S3\.Net45 InventoryEncryption\.cs 37 ```cpp private SSES3 sSES3; public SSES3 SSES3 { get { return this.SSES3; } set { this.SSES3 = value; } } ``` Ситуация идентична описанной выше\. Только здесь бесконечная рекурсия возникнет при обращении к свойству _SSES3_ как на чтение, так и на запись\. Исправленный вариант: ```cpp public SSES3 SSES3 { get { return this.sSES3; } set { this.sSES3 = value; } } ``` **Тест на внимательность** Далее следует задание от программиста, увлечённого использованием методики Copy\-Paste\. Посмотрите на то, как код выглядит в редакторе Visual Studio, и попробуйте найти ошибку \(кликните на картинку для увеличения\)\. ![0605_AWS_SDK_NET_ru/image3.png](https://import.viva64.com/docx/blog/0605_AWS_SDK_NET_ru/image3.png) **Предупреждение PVS\-Studio:** [V3029](https://pvs-studio.ru/ru/docs/warnings/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_, убрав всё лишнее\. Теперь видно, что идентичные проверки следуют одна за другой: ```cpp 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:** * V3029 The conditional expressions of the 'if' statements situated alongside each other are identical\. Check lines: 75, 79\. AWSSDK\.CloudDirectory\.Net45 CreateSchemaResponseUnmarshaller\.cs 75 * V3029 The conditional expressions of the 'if' statements situated alongside each other are identical\. Check lines: 105, 109\. AWSSDK\.CloudDirectory\.Net45 GetSchemaAsJsonResponseUnmarshaller\.cs 105 * V3029 The conditional expressions of the 'if' statements situated alongside each other are identical\. Check lines: 201, 205\. AWSSDK\.CodeCommit\.Net45 PostCommentForPullRequestResponseUnmarshaller\.cs 201 * V3029 The conditional expressions of the 'if' statements situated alongside each other are identical\. Check lines: 101, 105\. AWSSDK\.CognitoIdentityProvider\.Net45 VerifySoftwareTokenResponseUnmarshaller\.cs 101 * V3029 The conditional expressions of the 'if' statements situated alongside each other are identical\. Check lines: 72, 76\. AWSSDK\.Glue\.Net45 UpdateConnectionResponseUnmarshaller\.cs 72 * V3029 The conditional expressions of the 'if' statements situated alongside each other are identical\. Check lines: 123, 127\. AWSSDK\.Neptune\.Net45 RestoreDBClusterFromSnapshotResponseUnmarshaller\.cs 123 * V3029 The conditional expressions of the 'if' statements situated alongside each other are identical\. Check lines: 167, 171\. AWSSDK\.Neptune\.Net45 RestoreDBClusterFromSnapshotResponseUnmarshaller\.cs 167 * V3029 The conditional expressions of the 'if' statements situated alongside each other are identical\. Check lines: 127, 131\. AWSSDK\.RDS\.Net45 RestoreDBClusterFromSnapshotResponseUnmarshaller\.cs 127 * V3029 The conditional expressions of the 'if' statements situated alongside each other are identical\. Check lines: 171, 175\. AWSSDK\.RDS\.Net45 RestoreDBClusterFromSnapshotResponseUnmarshaller\.cs 171 * V3029 The conditional expressions of the 'if' statements situated alongside each other are identical\. Check lines: 99, 103\. AWSSDK\.Rekognition\.Net45 RecognizeCelebritiesResponseUnmarshaller\.cs 99 **Что ты такое?** **Предупреждение PVS\-Studio:** [V3062](https://pvs-studio.ru/ru/docs/warnings/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 ```cpp /// /// Dictionary that stores attribute for this event only. /// private Dictionary _attributes = new Dictionary(); /// /// Gets the attribute. /// /// Attribute name. /// The attribute. Return null of attribute doesn't /// exist. 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_\. Давайте попробуем исправить данный код\. Чтобы лучше понять, как это сделать, следует взглянуть на другой метод: ```cpp /// /// Determines whether this instance has attribute the specified /// attributeName. /// /// Attribute name. /// Return true if the event has the attribute, else /// false. 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_ и исправим ошибку: ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3063/) \[CWE\-571\] A part of conditional expression is always true if it is evaluated: string\.IsNullOrEmpty\(inferredIndexName\)\. AWSSDK\.DynamoDBv2\.PCL ContextInternal\.cs 802 ```cpp 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_: ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3095/) \[CWE\-476\] The 'conditionValues' object was used before it was verified against null\. Check lines: 228, 238\. AWSSDK\.Core\.Net45 JsonPolicyWriter\.cs 228 ```cpp private static void writeConditions(....) { .... foreach (....) { IList conditionValues = keyEntry.Value; if (conditionValues.Count == 0) // <= continue; .... if (conditionValues != null && conditionValues.Count != 0) { .... } .... } } ``` Классика жанра\. Переменную _conditionValues_ используют без предварительной проверки на равенство _null_\. При этом далее в коде такая проверка выполняется, но уже поздно\. :\) Код можно исправить следующим образом: ```cpp private static void writeConditions(....) { .... foreach (....) { IList conditionValues = keyEntry.Value; if (conditionValues != null && conditionValues.Count == 0) continue; .... if (conditionValues != null && conditionValues.Count != 0) { .... } .... } } ``` В коде было найдено ещё несколько подобных ошибок\. **Предупреждения** **PVS\-Studio:** * V3095 \[CWE\-476\] The 'ts\.Listeners' object was used before it was verified against null\. Check lines: 140, 143\. AWSSDK\.Core\.Net45 Logger\.Diagnostic\.cs 140 * V3095 \[CWE\-476\] The 'obj' object was used before it was verified against null\. Check lines: 743, 745\. AWSSDK\.Core\.Net45 JsonMapper\.cs 743 * V3095 \[CWE\-476\] The 'multipartUploadMultipartUploadpartsList' object was used before it was verified against null\. Check lines: 65, 67\. AWSSDK\.S3\.Net45 CompleteMultipartUploadRequestMarshaller\.cs 65 Следующее предупреждение очень похоже по смыслу, но ситуация обратна рассмотренной выше\. **Предупреждение PVS\-Studio:** [V3125](https://pvs-studio.ru/ru/docs/warnings/v3125/) \[CWE\-476\] The 'state' object was used after it was verified against null\. Check lines: 139, 127\. AWSSDK\.Core\.Net45 RefreshingAWSCredentials\.cs 139 ```cpp 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_\. Более безопасный вариант кода: ```cpp 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:** * V3125 \[CWE\-476\] The 'wrappedRequest\.Content' object was used after it was verified against null\. Check lines: 395, 383\. AWSSDK\.Core\.Net45 HttpHandler\.cs 395 * V3125 \[CWE\-476\] The 'datasetUpdates' object was used after it was verified against null\. Check lines: 477, 437\. AWSSDK\.CognitoSync\.Net45 Dataset\.cs 477 * V3125 \[CWE\-476\] The 'cORSConfigurationCORSConfigurationcORSRulesListValue' object was used after it was verified against null\. Check lines: 125, 111\. AWSSDK\.S3\.Net45 PutCORSConfigurationRequestMarshaller\.cs 125 * V3125 \[CWE\-476\] The 'lifecycleConfigurationLifecycleConfigurationrulesListValue' object was used after it was verified against null\. Check lines: 157, 68\. AWSSDK\.S3\.Net45 PutLifecycleConfigurationRequestMarshaller\.cs 157 * V3125 \[CWE\-476\] The 'this\.Key' object was used after it was verified against null\. Check lines: 199, 183\. AWSSDK\.S3\.Net45 S3PostUploadRequest\.cs 199 **Безальтернативная реальность** **Предупреждение PVS\-Studio:** [V3009](https://pvs-studio.ru/ru/docs/warnings/v3009/) \[CWE\-393\] It's odd that this method always returns one and the same value of 'true'\. AWSSDK\.Core\.Net45 Lexer\.cs 651 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3009/) \[CWE\-393\]\. Это обработчики: _State23, State26, State27, State28_\. Предупреждения, выданные анализатором для них: * V3009 \[CWE\-393\] It's odd that this method always returns one and the same value of 'true'\. AWSSDK\.Core\.Net45 Lexer\.cs 752 * V3009 \[CWE\-393\] It's odd that this method always returns one and the same value of 'true'\. AWSSDK\.Core\.Net45 Lexer\.cs 810 * V3009 \[CWE\-393\] It's odd that this method always returns one and the same value of 'true'\. AWSSDK\.Core\.Net45 Lexer\.cs 822 * V3009 \[CWE\-393\] It's odd that this method always returns one and the same value of 'true'\. AWSSDK\.Core\.Net45 Lexer\.cs 834 Вот как выглядит объявление и инициализация массива обработчиков: ```cpp private static StateHandler[] fsm_handler_table; .... private static void PopulateFsmTables () { fsm_handler_table = new StateHandler[28] { State1, State2, .... State19, .... State23, .... State26, State27, State28 }; ``` Для полноты картины \- посмотрим код одного из обработчиков, к которому у анализатора не было претензий, например, _State2_: ```cpp private static bool State2 (....) { .... if (....) { return true; } switch (....) { .... default: return false; } } ``` А вот так происходит вызов обработчиков: ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3022/) \[CWE\-571\] Expression 'doLog' is always true\. AWSSDK\.Core\.Net45 StoredProfileAWSCredentials\.cs 235 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression '\!result' is always false\. AWSSDK\.CognitoSync\.PCL SQLiteLocalStorage\.cs 353 ```cpp public void PutValue(....) { .... bool result = PutValueHelper(....); if (!result) <= { _logger.DebugFormat("{0}", @"Cognito Sync - SQLiteStorage - Put Value Failed"); } else { UpdateLastModifiedTimestamp(....); } .... } ``` Анализатор утверждает, что значение переменной _result_ всегда _true_\. Такое возможно лишь в случае, если метод _PutValueHelper_ будет всегда возвращать _true_\. Посмотрим на этот метод: ```cpp private bool PutValueHelper(....) { .... if (....)) { return true; } if (record == null) { .... return true; } else { .... return true; } } ``` Действительно, метод вернёт _true_ при любом условии\. Более того, анализатор выдал для этого метода предупреждение\. **Предупреждение PVS\-Studio:** [V3009](https://pvs-studio.ru/ru/docs/warnings/v3009/) \[CWE\-393\] It's odd that this method always returns one and the same value of 'true'\. SQLiteLocalStorage\.cs 1016 Я умышленно не стал приводить это предупреждение ранее, когда рассматривал другие ошибки [V3009](https://pvs-studio.ru/ru/docs/warnings/v3009/), а приберёг его для этого случая\. Таким образом, анализатор был прав, указав на ошибку [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) в вызывающем коде\. **Copy\-Paste\. Again** **Предупреждение PVS\-Studio:** [V3001](https://pvs-studio.ru/ru/docs/warnings/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 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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 ```cpp 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_\. Затем строку изменили, спецификатор пропал, а вот аргумент удалить забыли\. Приведённый фрагмент кода работает без ошибок \(исключение было бы выброшено в обратном случае \- спецификатор без аргумента\), но выглядит некрасиво\. Код следует исправить следующим образом: ```cpp if (region == null) { throw new InvalidOperationException( "EC2 instance metadata was not available or did not contain region information."); } ``` **Unsafe** **Предупреждение PVS\-Studio:** [V3083](https://pvs-studio.ru/ru/docs/warnings/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 ```cpp protected void FireSyncSuccessEvent(List records) { if (mOnSyncSuccess != null) { mOnSyncSuccess(this, new SyncSuccessEventArgs(records)); } } ``` Достаточно распространённая ситуация небезопасного вызова обработчика события\. Между проверкой переменной _mOnSyncSuccess_ на равенство _null_ и вызовом обработчика от события могут успеть отписаться, и его значение станет нулевым\. Вероятность такого сценария мала, но код всё же лучше сделать более безопасным: ```cpp protected void FireSyncSuccessEvent(List records) { mOnSyncSuccess?.Invoke(this, new SyncSuccessEventArgs(records)); } ``` В коде есть и другие подобные ошибки\. **Предупреждения PVS\-Studio:** * V3083 \[CWE\-367\] Unsafe invocation of event 'mOnSyncFailure', NullReferenceException is possible\. Consider assigning event to a local variable before invoking it\. AWSSDK\.CognitoSync\.PCL Dataset\.cs 839 * V3083 \[CWE\-367\] Unsafe invocation of event, NullReferenceException is possible\. Consider assigning event to a local variable before invoking it\. AWSSDK\.Core\.PCL AmazonServiceClient\.cs 332 * V3083 \[CWE\-367\] Unsafe invocation of event, NullReferenceException is possible\. Consider assigning event to a local variable before invoking it\. AWSSDK\.Core\.PCL AmazonServiceClient\.cs 344 * V3083 \[CWE\-367\] Unsafe invocation of event, NullReferenceException is possible\. Consider assigning event to a local variable before invoking it\. AWSSDK\.Core\.PCL AmazonServiceClient\.cs 357 * V3083 \[CWE\-367\] Unsafe invocation of event 'mExceptionEvent', NullReferenceException is possible\. Consider assigning event to a local variable before invoking it\. AWSSDK\.Core\.PCL AmazonServiceClient\.cs 366 * V3083 \[CWE\-367\] Unsafe invocation of event, NullReferenceException is possible\. Consider assigning event to a local variable before invoking it\. AWSSDK\.Core\.PCL AmazonWebServiceRequest\.cs 78 * V3083 \[CWE\-367\] Unsafe invocation of event 'OnRead', NullReferenceException is possible\. Consider assigning event to a local variable before invoking it\. AWSSDK\.Core\.PCL EventStream\.cs 97 * V3083 \[CWE\-367\] Unsafe invocation of event, NullReferenceException is possible\. Consider assigning event to a local variable before invoking it\. AWSSDK\.Core\.Android NetworkReachability\.cs 57 * V3083 \[CWE\-367\] Unsafe invocation of event, NullReferenceException is possible\. Consider assigning event to a local variable before invoking it\. AWSSDK\.Core\.Android NetworkReachability\.cs 94 * V3083 \[CWE\-367\] Unsafe invocation of event, NullReferenceException is possible\. Consider assigning event to a local variable before invoking it\. AWSSDK\.Core\.iOS NetworkReachability\.cs 54 **Недоработанный класс** **Предупреждение PVS\-Studio:** [V3126](https://pvs-studio.ru/ru/docs/warnings/v3126/) Type 'JsonData' implementing IEquatable interface does not override 'GetHashCode' method\. AWSSDK\.Core\.Net45 JsonData\.cs 26 ```cpp public class JsonData : IJsonWrapper, IEquatable { .... } ``` Класс _JsonData_ содержит достаточно много кода, поэтому я не стал приводить его целиком, ограничившись только объявлением\. Этот класс действительно не содержит переопределённого метода _GetHashCode_, что небезопасно, так как может привести к ошибочному поведению при использовании типа _JsonData_ для работы, например, с коллекциями\. Вероятно, в настоящее время проблемы нет, но в дальнейшем стратегия использования этого типа может измениться\. Более подробно данная ошибка описана в [документации](https://pvs-studio.ru/ru/docs/warnings/v3126/)\. **Заключение** Вот и все интересные ошибки, которые мне удалось обнаружить в коде AWS SDK для \.NET при помощи статического анализатора PVS\-Studio\. Ещё раз подчеркну качество проекта\. Я обнаружил весьма небольшое число ошибок для 5 миллионов строк кода\. Хотя, вероятно, более тщательный анализ выданных предупреждений позволил бы мне добавить к этому списку ещё несколько ошибок\. Но также весьма вероятно и то, что я зря причислил некоторые из описанных предупреждений к ошибкам\. Однозначные выводы в таком случае всегда делает только разработчик, который находится в контексте проверяемого кода\.