[`Fix`]: integer overflow in `JumpTable.SubStr` #3496

nan01ab · 2024-09-21T15:01:12Z

Description

Fix integer overflow in JumpTable.SubStr

Type of change

Optimization (the change is only an optimization)
Style (the change is only a code style for better maintenance or standard purpose)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

* null operation * fix array --------- Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com>

Co-authored-by: Jimmy <jinghui@wayne.edu>

* Set `Password` to `SecureString` for Wallet information * Added `UnitTest` for `SecureStringExtensions` * Update tests/Neo.Extensions.Tests/UT_SecureStringExtensions.cs Co-authored-by: Shargon <shargon@gmail.com> * Update src/Neo.Extensions/SecureStringExtensions.cs --------- Co-authored-by: Shargon <shargon@gmail.com>

* rpc parameter parse * update blockchain related apis. * Update src/Plugins/RpcServer/RpcMethodWithParamsAttribute.cs * Delete src/Plugins/RpcServer/JsonPropertyNameAttribute.cs * udpate contract model * Update src/Plugins/RpcServer/Model/BlockHashOrIndex.cs * Apply suggestions from code review Remove comments * Update src/Plugins/RpcServer/RpcServer.cs * fix warnings * ensure it can load both true/false and 1/0 * optimize the pr and add unit tests * add more tests and check the safe max value and safe min value * remove format * remove unused * format * Apply suggestions from code review --------- Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: NGD Admin <154295625+NGDAdmin@users.noreply.github.com>

* test RpcServer.Utilities * test invoke function and script * TestTraverseIterator * TestGetUnclaimedGas * RpcServerSettings.Default with { SessionEnabled = true } * test call with storage changes and events * use wallet in invokefunction * use invalid wallet * invoke without signer * all cases for TraverseIterator * traversing same session twice; not expired session * cover OnTimer * test deserializing complex signers * use Assert.ThrowsException * TestSendFrom and TestSendMany * apply code review with `nameof` * test cancel transaction * TestInvokeContractVerify * improve error message --------- Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: NGD Admin <154295625+NGDAdmin@users.noreply.github.com>

…o-project#3408) * Part-1 `Neo.IO` - move * Part-2 * Added `BigInteger` to `Neo.Extensions` * Found more `BigInteger` * Added `ByteArray` to `Neo.Extensions` * Added `DateTime` Extensions to `Neo.Extensions` * Added `HashSetExtensions`, `HashSetExtensions2`, `IpAddressExtensions`, `AssemblyExtensions`, `StringExtensdions` Deleted `Helper.cs` file * Added Tests * Added `tests` from `Part-2` * Added `tests` for `PART-4` * Add `using Neo.Extensions` for unit tests * Change `HashSetExtensions2` to `HashSetExtensions` * Update tests/Neo.Extensions.Tests/UT_BigIntegerExtensions.cs * Update and rename StringExtensdions.cs to StringExtensdios.cs * Rename StringExtensdios.cs to StringExtensions.cs * `dotnet format` --------- Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: Jimmy <jinghui@wayne.edu> Co-authored-by: NGD Admin <154295625+NGDAdmin@users.noreply.github.com>

* UT_KeyedCollectionSlim * Update UT_KeyedCollectionSlim.cs * Update UT_MemoryReader.cs * Update UT_MemoryReader.cs

* improve error message when wrong wallet is opened * update tests

* Update UT_RpcServer.Blockchain.cs * Update UT_RpcServer.Blockchain.cs * update * fixed bug * format * update * Update NativeContractExtensions.cs * update * Remove conflicting files * update * format

@shargon

* Fixed `UInt160` and expanded class * Cleaned up code for `TryParse` * Fixed `TryParse` * Fixed small bug with `TryParse` * Change `UInt160.Zero` to `static readonly` * benchmark UInt160 * Fix benchmark * Fixed bugs and added features for `UInt160` class * Revert and just keep bug fixes * Made @shargon changes * Set `InvariantCultureIgnoreCase` back for `0x` and `0X` --------- Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: Jimmy <jinghui@wayne.edu> Co-authored-by: Jimmy <jimmy@r3e.network>

* test GetApplicationLog * filter execution type * Test_Commands; refactor * apply review suggestions --------- Co-authored-by: Shargon <shargon@gmail.com>

* Add UT Neo.Extensions * resolve conflicts * Revert "resolve conflicts" This reverts commit 6d0a61b. * add edge case * update mod test --------- Co-authored-by: NGD Admin <154295625+NGDAdmin@users.noreply.github.com> Co-authored-by: Jimmy <jinghui@wayne.edu> Co-authored-by: Jimmy <jimmy@r3e.network>

* Update UT_Utility.cs * TestGetContractState

- Fix typo Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: Jimmy <jinghui@wayne.edu> Co-authored-by: NGD Admin <154295625+NGDAdmin@users.noreply.github.com>

* trigger https oracle * make it internal * return Task.CompletedTask --------- Co-authored-by: Jimmy <jimmy@r3e.network> Co-authored-by: Jimmy <jinghui@wayne.edu> Co-authored-by: Shargon <shargon@gmail.com>

…arameter types. (neo-project#3479) * update rpc node methods signatures to use explicit parameter types. * fix attribute place --------- Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> Co-authored-by: Shargon <shargon@gmail.com>

* Update OpCode.cs * Update JumpTable.Compound.cs * Update OpCode.cs

* parse nef file scripts * nef file path support --------- Co-authored-by: Vitor Nazário Coelho <vncoelho@gmail.com>

…es (neo-project#3486) * fix push integer * Update src/Neo.VM/ScriptBuilder.cs * Update tests/Neo.VM.Tests/UT_ScriptBuilder.cs * Update tests/Neo.VM.Tests/UT_ScriptBuilder.cs

cschuchardt88

@shargon why isn't there a vm limit in this

cschuchardt88 · 2024-09-21T15:24:44Z

src/Neo.VM/JumpTable/JumpTable.Splice.cs

+            if (index >= x.Length)
+                throw new InvalidOperationException($"The value of index {index} is out of range.");
+
+            if (checked(index + count) > x.Length)


Suggested change

if (checked(index + count) > x.Length)

if (unchecked(index + count) > x.Length)

This would be better. If it overflows it will be bigger than the length. This way we don't have dotnet generic overflow exception.

This would be better. If it overflows it will be bigger than the length. This way we don't have dotnet generic overflow exception.

checked here behaves the same as Memcpy.

Maybe we need a HF

Echidna, yes. But checked is likely needed. The only reason this works in NeoGo is because int is mostly 64-bit (on all officially supported NeoGo platforms it is, but in theory NeoGo can be compiled for 32-bit platforms as well).

cschuchardt88 · 2024-09-21T15:27:20Z

src/Neo.VM/JumpTable/JumpTable.Splice.cs

@@ -95,13 +95,22 @@ public virtual void SubStr(ExecutionEngine engine, Instruction instruction)
        {


@shargon why isn't there a vm limit in this?

Because a substr can't be longer than the previous item.. theoretically

roman-khimov · 2024-09-23T12:17:39Z

tests/Neo.VM.Tests/Tests/OpCodes/Splice/SUBSTR.json

@@ -473,6 +473,50 @@
          }
        }
      ]
+    },
+    {
+      "name": "Count Exceed Range Test",


at instruction 18 (SUBSTR): invalid offset in NeoGo.

roman-khimov · 2024-09-23T12:19:32Z

tests/Neo.VM.Tests/Tests/OpCodes/Splice/SUBSTR.json

+      ]
+    },
+    {
+      "name": "Index Exceed Range Test",


at instruction 18 (SUBSTR): invalid offset in NeoGo.

roman-khimov · 2024-09-23T12:20:45Z

tests/Neo.VM.Tests/Tests/OpCodes/Splice/SUBSTR.json

+        "0x0a",
+        "0x00010203040506070809",
+        "PUSHINT32",
+        "0x7FFFFFFF",


I'd also add some tests for INT64, like:

byte(opcode.PUSHINT64), 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0x7F, byte(opcode.PUSH2),

It'll fail (in NeoGo it's at instruction 22 (SUBSTR): not an int32), but just to make sure.

roman-khimov · 2024-09-23T12:29:36Z

src/Neo.VM/JumpTable/JumpTable.Splice.cs

+            if (index >= x.Length)
+                throw new InvalidOperationException($"The value of index {index} is out of range.");
+
+            if (checked(index + count) > x.Length)


Echidna, yes. But checked is likely needed. The only reason this works in NeoGo is because int is mostly 64-bit (on all officially supported NeoGo platforms it is, but in theory NeoGo can be compiled for 32-bit platforms as well).

* fix: concurrency conflict in HeaderCache.Count * Update tests/Neo.UnitTests/Ledger/UT_HeaderCache.cs * Update tests/Neo.UnitTests/Ledger/UT_HeaderCache.cs --------- Co-authored-by: Shargon <shargon@gmail.com>

* Added Builders with tests * Added SignerBuilder and started WitnessRuleBuilder * Added `WitnessConditionBuilder` with tests * Added more logic * Fixed `SignerBuilder` class * Code touch ups * Added more tests * Update src/Neo/Builders/TransactionBuilder.cs * Fixed `And` `Or` and `Not` conditions * Fixed Memory leak * Added error message for Witness scripts --------- Co-authored-by: Jimmy <jinghui@wayne.edu> Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: NGD Admin <154295625+NGDAdmin@users.noreply.github.com>

* Fixed Delete packages for github * Automatic * added continue-on-error: true --------- Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: NGD Admin <154295625+NGDAdmin@users.noreply.github.com>

shargon · 2024-09-27T07:12:07Z

Rebase needed

Jim8y and others added 21 commits August 8, 2024 20:09

[Neo Json Fix] Json null tests (neo-project#3450)

28043cb

* null operation * fix array --------- Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com>

Update README.md (neo-project#3459)

a93adb7

Update RpcServer.Blockchain.cs (neo-project#3458)

0d46c20

Co-authored-by: Jimmy <jinghui@wayne.edu>

Add UT Neo.IO (neo-project#3466)

23f1bce

* UT_KeyedCollectionSlim * Update UT_KeyedCollectionSlim.cs * Update UT_MemoryReader.cs * Update UT_MemoryReader.cs

improve error message when wrong wallet is opened (neo-project#3469)

dc926f7

* improve error message when wrong wallet is opened * update tests

Update UT RpcServer (neo-project#3460)

9ce9c6f

* Update UT_RpcServer.Blockchain.cs * Update UT_RpcServer.Blockchain.cs * update * fixed bug * format * update * Update NativeContractExtensions.cs * update * Remove conflicting files * update * format

test GetApplicationLog (neo-project#3470)

e018bc3

* test GetApplicationLog * filter execution type * Test_Commands; refactor * apply review suggestions --------- Co-authored-by: Shargon <shargon@gmail.com>

Add some UT (neo-project#3476)

da193e9

* Update UT_Utility.cs * TestGetContractState

Update ConstantTimeUtility.cs (neo-project#3472)

3d663f2

- Fix typo Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: Jimmy <jinghui@wayne.edu> Co-authored-by: NGD Admin <154295625+NGDAdmin@users.noreply.github.com>

Test OracleService (neo-project#3475)

2b118b4

* trigger https oracle * make it internal * return Task.CompletedTask --------- Co-authored-by: Jimmy <jimmy@r3e.network> Co-authored-by: Jimmy <jinghui@wayne.edu> Co-authored-by: Shargon <shargon@gmail.com>

Fixing errors in comments (neo-project#3483)

ff58726

* Update OpCode.cs * Update JumpTable.Compound.cs * Update OpCode.cs

parse nef file scripts (neo-project#3482)

c6282cd

* parse nef file scripts * nef file path support --------- Co-authored-by: Vitor Nazário Coelho <vncoelho@gmail.com>

[Neo VM Style] Throw exception for Integer that is larger than 32 byt…

9946450

…es (neo-project#3486) * fix push integer * Update src/Neo.VM/ScriptBuilder.cs * Update tests/Neo.VM.Tests/UT_ScriptBuilder.cs * Update tests/Neo.VM.Tests/UT_ScriptBuilder.cs

fix: int overflow in SubStr

98e834b

nan01ab changed the title ~~fix: integer overflow in JumpTable.SubStr~~ [Fix]: integer overflow in JumpTable.SubStr Sep 21, 2024

cschuchardt88 reviewed Sep 21, 2024

View reviewed changes

cschuchardt88 added the waiting for review label Sep 21, 2024

roman-khimov mentioned this pull request Sep 23, 2024

Adapt new way of NeoVM tests delivering nspcc-dev/neo-go#3204

Open

roman-khimov reviewed Sep 23, 2024

View reviewed changes

nan01ab and others added 4 commits September 23, 2024 20:46

fix: int overflow in SubStr

4a3c207

Update RpcError.cs (neo-project#3498)

a52e321

fix: concurrency conflict in HeaderCache.Count (neo-project#3501)

d6ed5a5

* fix: concurrency conflict in HeaderCache.Count * Update tests/Neo.UnitTests/Ledger/UT_HeaderCache.cs * Update tests/Neo.UnitTests/Ledger/UT_HeaderCache.cs --------- Co-authored-by: Shargon <shargon@gmail.com>

Merge branch 'master' into fix.overflow-in-substr

2494812

Jim8y changed the base branch from master to HF_Echidna September 27, 2024 01:24

cschuchardt88 and others added 2 commits September 27, 2024 10:48

[Fix] Neo Plugins github nuget (neo-project#3493)

6a4ea1f

* Fixed Delete packages for github * Automatic * added continue-on-error: true --------- Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: NGD Admin <154295625+NGDAdmin@users.noreply.github.com>

shargon added hardfork waiting-hardfork bug Used to tag confirmed bugs critical Issues (bugs) that need to be fixed ASAP and removed hardfork labels Sep 27, 2024

Merge branch 'neo-project:master' into fix.overflow-in-substr

20ec1e1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[`Fix`]: integer overflow in `JumpTable.SubStr` #3496

[`Fix`]: integer overflow in `JumpTable.SubStr` #3496

nan01ab commented Sep 21, 2024

cschuchardt88 left a comment

cschuchardt88 Sep 21, 2024

nan01ab Sep 21, 2024 •

edited

Loading

shargon Sep 22, 2024

roman-khimov Sep 23, 2024

cschuchardt88 Sep 21, 2024

shargon Sep 22, 2024

roman-khimov Sep 23, 2024

roman-khimov Sep 23, 2024

roman-khimov Sep 23, 2024

roman-khimov Sep 23, 2024

shargon commented Sep 27, 2024

	if (checked(index + count) > x.Length)
	if (unchecked(index + count) > x.Length)

		@@ -95,13 +95,22 @@ public virtual void SubStr(ExecutionEngine engine, Instruction instruction)
		{

[Fix]: integer overflow in JumpTable.SubStr #3496

Are you sure you want to change the base?

[Fix]: integer overflow in JumpTable.SubStr #3496

Conversation

nan01ab commented Sep 21, 2024

Description

Type of change

Checklist:

cschuchardt88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nan01ab Sep 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shargon commented Sep 27, 2024

[`Fix`]: integer overflow in `JumpTable.SubStr` #3496

[`Fix`]: integer overflow in `JumpTable.SubStr` #3496

nan01ab Sep 21, 2024 •

edited

Loading