# Код блокчейн\-проектов Neo и NBitcoin VS анализатор кода\. Кто\-кого? Ввиду своей специфики блокчейн\-разработка крайне требовательна к качеству кода: мало того, что незамеченная ошибка может привести к большим финансовым проблемам, она и вовсе может оказаться необратимой\. Стоит ли здесь пренебрегать использованием анализатора кода? Выясним это, проверив код проектов Neo и NBitcoin\. ![1312_NeoAndNBitcoin_ru/image1.png](https://import.viva64.com/docx/blog/1312_NeoAndNBitcoin_ru/image1.png) ## Введение Ранее я упомянул про специфику блокчейн\-разработки\. Давайте разберёмся, что конкретно подразумевалось\. Во\-первых, многие блокчейн\-проекты работают с цифровыми активами, имеющими реальную ценность: токенами, криптовалютами, NFT, правами доступа и т\. д\. Ошибка в коде может привести не просто к сбою в работе программы, а к прямым финансовым потерям пользователей\. Во\-вторых, исправить код блокчейн\-проекта после релиза либо проблематично \(в случае кода децентрализованной сети, где необходимо, чтобы каждый из узлов согласился принять правки\), либо вовсе невозможно \(в случае смарт\-контрактов\)\. Зачастую также невозможно изменить и информацию, попавшую в блокчейн, в том числе и некорректную, например, полученную из\-за ошибки в коде\. Эти ограничения — цена за почти идеальную целостность данных\. Технически это обеспечивается хранением полной или частичной копии блокчейна на каждом из его узлов\. В результате согласования сети посредством специальных алгоритмов определяется актуальная версия блокчейна, которой придерживается большинство узлов сети\. Если версия блокчейна на некотором узле противоречит общепринятой, этот узел автоматически синхронизирует с ней свою версию сети\. Благодаря этому любые изменения состояния блокчейна, не согласованные с сетью, будут отвергнуты\. Думаю, это далеко неполный список, но его вполне достаточно, чтобы сформировать представление о том, насколько велика может быть цена ошибки в коде блокчейн\-проекта\. В этой статье мы рассмотрим примеры как явных, так и потенциальных ошибок, найденных с помощью статического анализатора кода PVS\-Studio в двух open source C\# проектах: * [Neo](https://github.com/neo-project/neo) — полноценная блокчейн\-платформа, управляемая сообществом; * [NBitcoin](https://github.com/MetacoSA/NBitcoin/tree/master) — библиотека для работы с биткоином на платформе \.NET\. ## О PVS\-Studio [PVS\-Studio](https://pvs-studio.ru/ru/) — инструмент для автоматического поиска потенциальных ошибок и уязвимостей безопасности в коде, также именуемый статическим анализатором кода\. На момент написания статьи PVS\-Studio поддерживает анализ C\#, C, C\+\+ и Java кода\. PVS\-Studio используется через [интеграцию](https://pvs-studio.ru/ru/pvs-studio/#integration) с различными инструментами разработки, будь то IDE, сборочные системы, CI\-сервисы или другие инструменты для проверки кода, например, SonarCube\. Для анализа Neo и NBitcoin использовался базовый подход через плагин для IDE, в нашем случае — Visual Studio\. Такой плагин предоставляет возможность быстрого запуска анализа решений/проектов/отдельных файлов прямо из IDE: ![1312_NeoAndNBitcoin_ru/image2.png](https://import.viva64.com/docx/blog/1312_NeoAndNBitcoin_ru/image2.png) А также простой и удобный интерфейс для работы с результатами анализа: ![1312_NeoAndNBitcoin_ru/image3.png](https://import.viva64.com/docx/blog/1312_NeoAndNBitcoin_ru/image3.png) Теперь, когда мы получили общее представление об анализаторе, давайте, наконец, перейдём к обзору найденных ошибок\. ## Обзор потенциальных ошибок, найденных в коде Neo Начнём с блокчейна Neo версии [3\.8\.2](https://github.com/neo-project/neo/releases/tag/v3.8.2) \(последняя на момент написания статьи\)\. ### Подозрительно избыточное выражение ```cpp internal static CommandStringToken Parse(...., ref int index, ....) { .... var ret = new CommandStringToken(....); index += end - index; return ret; } ``` Предупреждение анализатора: [V3107](https://pvs-studio.ru/ru/docs/warnings/v3107/) Identical expression 'index' to the left and to the right of compound assignment\. [CommandStringToken\.cs 80](https://github.com/neo-project/neo/blob/v3.8.2/src/Neo.ConsoleService/CommandStringToken.cs#L80) Обратите внимание на странное выражение `index += end – index`\. Оно эквивалентно `index = index – index + end`\. Результатом операции вычитания всегда будет `0`\. Следовательно, результатом всего выражения будет значение переменной `end`\. Получается, что вычитание избыточно\. Это может быть признаком наличия ошибки, если, к примеру, вместо `index` должно было вычитаться какое\-то другое значение\. ### Некорректный формат ```cpp public override string ToString() { var sb = new StringBuilder(); sb.AppendFormat("{1:X04} {2,-10}{3}{4}", Position, OpCode, DecodeOperand()); return sb.ToString(); } ``` Предупреждение анализатора: [V3025](https://pvs-studio.ru/ru/docs/warnings/v3025/) \[CWE\-685\] Incorrect format\. A different number of format items is expected while calling 'AppendFormat' function\. Format items not used: \{3\}, \{4\}\. Arguments not used: 1st\. [VMInstruction\.cs 105](https://github.com/neo-project/neo/blob/v3.8.2/src/Neo.CLI/Tools/VMInstruction.cs#L105) Вызов переопределённого метода `ToString` неизбежно приведёт к исключению\. Всё из\-за некорректного вызова `sb.AppendFormat`, в котором допущены сразу две ошибки: * число аргументов для вставки меньше числа плейсхолдеров в строке формата, что и станет причиной исключения; * даже если первая проблема будет исправлена \(число аргументов и плейсхолдеров станет равным\), этот вызов всё так же будет вызывать исключение, т\. к\. индексация плейсхолдеров начинается с 0, а не с 1\. Из этого следует, что для плейсхолдера с индексом 4 необходим 5\-й аргумент, который будет отсутствовать\. ### Ошибка из\-за путаницы в приоритетах операторов ```cpp public override int Size => base.Size + ChangeViewMessages?.Values.GetVarSize() ?? 0 + 1 + PrepareRequestMessage?.Size ?? 0 + PreparationHash?.Size ?? 0 + PreparationMessages?.Values.GetVarSize() ?? 0 + CommitMessages?.Values.GetVarSize() ?? 0; ``` Предупреждение анализатора: [V3123](https://pvs-studio.ru/ru/docs/warnings/v3123/) \[CWE\-783\] Perhaps the '??' operator works in a different way than it was expected\. Its priority is lower than priority of other operators in its left part\. [RecoveryMessage\.cs 35](https://github.com/neo-project/neo/blob/v3.8.2/src/Plugins/DBFTPlugin/Messages/RecoveryMessage/RecoveryMessage.cs#L35) На этом коде анализатор выдал сразу несколько предупреждений диагностики V3123, но для компактности я привёл только одно\. Оператор `??` имеет меньший приоритет по сравнению с оператором `+`\. Однако форматирование приведенного выражения указывает на то, что разработчик ожидал обратное\. Но есть ли в этом случае разница, какая операция будет выполнена первой? Для ответа на этот вопрос рассмотрим пример одного подвыражения сложения, если `ChangeViewMessages` равен `null`: ```cpp base.Size + ChangeViewMessages?.Values.GetVarSize() ?? 0 ``` Неважно, какое значение будет у `base.Size`, результат подвыражения всегда будет 0, ведь в результате сложения `base.Size` c `null` получится всё тот же `null`\. Если поместить `ChangeViewMessages?.Values.GetVarSize() ?? 0` в скобки, изменив тем самым порядок вычисления операций, в результатом будет `base.Size`\. ### Потенциальное null\-разыменование **Issue 1** ```cpp public OracleNeoFSProtocol(Wallet wallet, ECPoint[] oracles) { byte[] key = oracles.Select(p => wallet.GetAccount(p)) .Where(p => p is not null && p.HasKey && !p.Lock) .FirstOrDefault() .GetKey() .PrivateKey; privateKey = key.LoadPrivateKey(); } ``` Предупреждение анализатора: [V3146](https://pvs-studio.ru/ru/docs/warnings/v3146/) \[CWE\-476\] Possible null dereference\. The 'FirstOrDefault' can return default null value\. [OracleNeoFSProtocol\.cs 37](https://github.com/neo-project/neo/blob/v3.8.2/src/Plugins/OracleService/Protocols/OracleNeoFSProtocol.cs#L37) Перед нами типичный пример потенциального `null`\-разыменования, а именно разыменование возвращаемого значения метода `FirstOfDefault`, вызываемого у коллекции элементов ссылочного типа\. Если коллекция окажется пуста, этот метод вернёт `null`, что впоследствии приведёт к выбросу исключения\. **Issue 2** ```cpp public bool ValidatorsChanged { get { .... TrimmedBlock currentBlock = NativeContract.Ledger.GetTrimmedBlock(....); TrimmedBlock previousBlock = NativeContract.Ledger.GetTrimmedBlock(...., currentBlock.Header // <= .PrevHash); return currentBlock.Header.NextConsensus != previousBlock.Header.NextConsensus; // <= } } ``` Предупреждения анализатора: [V3080](https://pvs-studio.ru/ru/docs/warnings/v3080/) \[CWE\-476\] Possible null dereference\. Consider inspecting 'currentBlock'\. [ConsensusContext\.cs 89](https://github.com/neo-project/neo/blob/v3.8.2/src/Plugins/DBFTPlugin/Consensus/ConsensusContext.cs#L89) [V3080](https://pvs-studio.ru/ru/docs/warnings/v3080/) \[CWE\-476\] Possible null dereference\. Consider inspecting 'previousBlock'\. [ConsensusContext\.cs 90](https://github.com/neo-project/neo/blob/v3.8.2/src/Plugins/DBFTPlugin/Consensus/ConsensusContext.cs#L90) Анализатор дважды предупреждает о возможном `null`\-разыменовании и указывает на переменные `currentBlock` и `previousBlock`\. Но почему разыменование этих переменных опасно? Предлагаю поискать ответ в источнике значений обеих переменных — методе `GetTrimmedBlock`: ```cpp public TrimmedBlock GetTrimmedBlock(IReadOnlyStore snapshot, UInt256 hash) { if (snapshot is null) throw new ArgumentNullException(nameof(snapshot)); var key = CreateStorageKey(Prefix_Block, hash); if (snapshot.TryGet(key, out var item)) return item.Value.AsSerializable(); return null; } ``` Метод действительно может вернуть `null`\. Тем не менее остаётся возможность того, что это происходит в каких\-то специфичных случаях\. Наверняка этот момент может уточнить только автор кода, но мы можем примерно оценить вероятность, посмотрев, как часто проверяется возвращаемое значение `GetTrimmedBlock` на `null` в целом\. В результате такой проверки я выяснил, что возвращаемое значение проверяется в 70% вызовов, что свидетельствует о значительном риске выброса исключения\. **Issue 3** ```cpp private void OnTimer(Timer timer) { .... if ( timer.Height != context.Block.Index // <= || timer.ViewNumber != context.ViewNumber) { return; } if ( context.Block != null // <= && context.TransactionHashes?.Length > context.Transactions? .Count) { .... } } ``` Предупреждение анализатора: [V3095](https://pvs-studio.ru/ru/docs/warnings/v3095/) \[CWE\-476\] The 'context\.Block' object was used before it was verified against null\. Check lines: 173, 191\. [ConsensusService\.cs 173](https://github.com/neo-project/neo/blob/v3.8.2/src/Plugins/DBFTPlugin/Consensus/ConsensusService.cs#L173) Снова предупреждение о потенциальном `null`\-разыменовании\. Свойство `context.Block` сначала используется без проверки, после чего проверяется на `null`\. В лучшем случае это просто избыточная проверка\. Однако может быть и так, что содержащий разыменование код является доработкой, внесённой позже базовой логики c проверкой\. В этом случае разработчик вполне мог не заметить, что при выполнении этого кода свойство `context.Block` может иметь значение `null`\. ### Некорректный цикл ```cpp [RpcMethodWithParams] protected internal virtual JToken GetCandidates() { .... foreach (var item in resultstack) { var value = (Array)item; foreach (Struct ele in value) { var publickey = ele[0].GetSpan().ToHexString(); json["publickey"] = publickey; json["votes"] = ele[1].GetInteger().ToString(); json["active"] = validators.ToByteArray() .ToHexString() .Contains(publickey); jArray.Add(json); json = new(); } return jArray; // <= } .... } ``` Предупреждение анализатора: [V3020](https://pvs-studio.ru/ru/docs/warnings/v3020/) \[CWE\-670\] An unconditional 'return' within a loop\. [RpcServer\.Blockchain\.cs 380](https://github.com/neo-project/neo/blob/v3.8.2/src/Plugins/RpcServer/RpcServer.Blockchain.cs#L380) Обратите внимание на возврат из метода, выполняемый в теле цикла\. Ввиду отсутствия каких\-либо условий и операторов `continue`, в этом цикле всегда будет выполняться лишь одна итерация, что почти наверняка является серьёзной ошибкой\. Вероятно, была допущена опечатка, и возврат из метода должен был выполняться после цикла, а не внутри\. ### Опечатка в реализации метода Equals ```cpp public bool Equals(Nep11BalanceKey other) { if (other is null) return false; if (ReferenceEquals(this, other)) return true; return UserScriptHash.Equals(other.UserScriptHash) && AssetScriptHash.Equals(AssetScriptHash) // <= && Token.Equals(other.Token); } ``` Предупреждение анализатора: [V3062](https://pvs-studio.ru/ru/docs/warnings/v3062/) An object 'AssetScriptHash' is used as an argument to its own method\. Consider checking the first actual argument of the 'Equals' method\. [Nep11BalanceKey\.cs 57](https://github.com/neo-project/neo/blob/v3.8.2/src/Plugins/TokensTracker/Trackers/NEP-11/Nep11BalanceKey.cs#L57) Простая, но весьма незаметная ошибка\. В приведённой реализации `Equals` значение поля `AssetScriptHash` родительского объекта сравнивается само с собой\. Очевидно, что вместо этого ожидалось его сравнение со значением того же поля объекта `other`\. Отмечу, что анализатор нашёл точно такую же ошибку в эквивалентном коде другого класса: ```cpp public bool Equals(Nep17BalanceKey other) { if (other is null) return false; if (ReferenceEquals(this, other)) return true; return UserScriptHash.Equals(other.UserScriptHash) && AssetScriptHash.Equals(AssetScriptHash); } ``` Предупреждение анализатора: [V3062](https://pvs-studio.ru/ru/docs/warnings/v3062/) An object 'AssetScriptHash' is used as an argument to its own method\. Consider checking the first actual argument of the 'Equals' method\. [Nep17BalanceKey\.cs 50](https://github.com/neo-project/neo/blob/v3.8.2/src/Plugins/TokensTracker/Trackers/NEP-17/Nep17BalanceKey.cs#L50) ## Обзор потенциальных ошибок в NBitcoin Теперь давайте перейдём к обзору потенциальных ошибок в коде NBitcoin версии [9\.0\.1](https://github.com/MetacoSA/NBitcoin/releases/tag/v9.0.1)\. ### Ошибка из\-за невнимательного copy\-paste ```cpp bool SameSigHash(uint a, uint b) { if (a == b) return true; if (GetTransaction() is not IHasForkId) return false; a = ((uint)a & ~(0x40u)); b = ((uint)a & ~(0x40u)); // <= return a == b; } ``` Предупреждение анализатора: [V3127](https://pvs-studio.ru/ru/docs/warnings/v3127/) \[CWE\-682\] Two similar code fragments were found\. Perhaps, this is a typo and 'b' variable should be used instead of 'a' [PSBTInput\.cs 948](https://github.com/MetacoSA/NBitcoin/blob/v9.0.1/NBitcoin/BIP174/PSBTInput.cs#L948) В конце метода переменным `a` и `b` присваивается результат вычисления одного и того же выражения\. Это не обязательно может быть ошибкой, однако сравнение значений этих переменных в последующем операторе возврата не оставляет сомнений\. Вероятно, выражение присваивания переменной `b` было получено путём копирования выражения переменной `a`, но не было отредактировано\. ### Бесконечная рекурсия ```cpp public static Message ReadNext(Socket socket, Network network, uint version, CancellationToken cancellationToken, out PerformanceCounter counter) { return ReadNext(socket, network, version, cancellationToken, out counter); } ``` Предупреждение анализатора: [V3110](https://pvs-studio.ru/ru/docs/warnings/v3110/) \[CWE\-674\] Possible infinite recursion inside 'ReadNext' method\. [Message\.cs 167](https://github.com/MetacoSA/NBitcoin/blob/v9.0.1/NBitcoin/Protocol/Message.cs#L167) Вызов этой перегрузки метода приведёт к `StackOverflowException`, ведь единственное выражение в её блоке — это возврат результата её собственного рекурсивного вызова\. ### Одинаковые блоки в switch ```cpp public override string ToString() { switch (this.Tag) { case Tags.BoolAnd: return "BoolAnd"; case Tags.BoolOr: return "BoolAnd"; // <= case Tags.Add: return "Add"; case Tags.Equal: return "Equal"; case Tags.EqualVerify: return "EqualVerify"; .... } throw new Exception("Unreachable"); } ``` Предупреждение анализатора: [V3139](https://pvs-studio.ru/ru/docs/warnings/v3139/) Two or more case\-branches perform the same actions\. [ScriptToken\.cs 169](https://github.com/MetacoSA/NBitcoin/blob/v9.0.1/NBitcoin/Scripting/ScriptToken.cs#L169) Реализация метода `ToString` содержит `switch`\-выражение, в котором блок одного из `case` дублирует блок другого\. Т\. к\. имеется явный паттерн, подразумевающий уникальность каждого `case`\-блока, можно с уверенностью сказать, что тут опечатка\. ### Перегрузка метода Equals без перегрузки GetHashCode ```cpp public class MoneyBag : IMoney, IEnumerable, IEquatable { .... public bool Equals(MoneyBag other) { return Equals(other as IMoney); } public bool Equals(IMoney other) { if (other is null) return false; var m = new MoneyBag(other); return m._bag.SequenceEqual(_bag); } .... } ``` Предупреждение анализатора: [V3126](https://pvs-studio.ru/ru/docs/warnings/v3126/) Type 'MoneyBag' implementing IEquatable interface does not override 'GetHashCode' method\. [Money\.cs 78](https://github.com/MetacoSA/NBitcoin/blob/v9.0.1/NBitcoin/Money.cs#L78) Анализатор обнаружил класс, реализующий интерфейс `IEquatable`, но не переопределяющий метод `GetHashCode`\. Это потенциально может привести к проблемам, ведь некоторые методы, например, из библиотеки `Linq`, в первую очередь выполняют сравнение по хэш\-коду, и только если эта проверка вернёт `true`, то будет выполнено сравнение с помощью `Equals`\. Если же хэши окажутся не равны, то считается, что и сами объекты тоже не равны\. ## Заключение Даже в блокчейн\-проектах, где требовательность к качеству кода особенно высока, нам удалось найти несколько проблем с помощью статического анализатора PVS\-Studio\. Некоторые из них вполне могут оказаться весьма серьёзными, ведь приводят к неожиданному выбросу исключения, некорректной проверке на равенство и бесконечной рекурсии\. Кроме того, важно отметить, что в статье мы рассмотрели самые явные проблемы, а ведь за кулисами осталось много предупреждений, требующих более глубокого изучения и понимания кода\. Если вы хотите опробовать PVS\-Studio на собственном коде, то можете получить пробную лицензию на официальном [сайте](https://pvs-studio.ru/ru/pvs-studio/try-free/)\. В этом [разделе документации](https://pvs-studio.ru/ru/docs/manual/0046/) можно найти инструкцию по её активации и ссылки на другим разделам, которые помогут вам начать работу с анализатором\. До встречи в следующих статьях\!