[Neo Core Bug]fix 3300 #3301

Jim8y · 2024-06-07T05:36:58Z

Description

This pr fixes the issue 3300

Fixes # #3300

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

How Has This Been Tested?

Test Configuration:

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

dusmart · 2024-06-07T05:41:05Z

You caught me so quickly. This GetNotifications is exactly what I'm exploiting with.

dusmart · 2024-06-07T07:20:49Z

Your PR is definitely a good one. It not only fixed my issue, but also fixed another issue.

~~Considering this attack vector (seems that I can't come up with a POC because of the immutable copy from Runtime.Notify).~~

contract A send a notification NA
contract B relies on that notification NA and reads that, then changed it to NA'
contract C also relies on that notification, and it can only get the NA' because the State is not deep copied

AnnaShaleva · 2024-06-07T07:51:53Z

src/Neo.VM/ReferenceCounter.cs

+        private void DecreaseCounter(int count = 1)
+        {
+            references_count -= count;
+            if (references_count < 0) throw new InvalidOperationException("Reference counter is negative.");


But how is it ever possible? Have you faced with the issue when refcount is negative?

Yes, its @dusmart who found the issue. With Array object deepcopied and removed from the stack, all of its references are removed as well, when we add that copied array to the stack, its internal subitems will not be added to the reference counter, but when we remove that copied item from the stack again, the counter will consider the array subitems.

BArray = AArray.DeepCopy

Remove BArray from stack // this will remove BArray and its subitem from the counter.

Aadd BArray back to stack // This will only add BArray itself back to the counter

Remove BArray from stack // This will remove BArray and its subitems from the counter, and create minus counter.

AArray.DeepCopy doesn't increase the counter?

AArray.DeepCopy doesn't increase the counter?

It should increase the counter:

neo/src/Neo.VM/Types/Array.cs

Line 75 in 6e8c730

referenceCounter.AddReference(item, this);

if (references_count < 0) throw new InvalidOperationException("Reference counter is negative.");

I think we should not change this code. It's a bug in the handlers implementation, not in the reference counter logic. So we should fix the source problem, not its consequence.

reference counter is supposed to be not negative, it should be the counter's property, should not rely on the callers.

@Jim8y let's discuss it here, we have a designated thread for it. If we add this change, then it may affect some old transactions. And I'd like to see @roman-khimov's opinion on this topic.

Why? isn't you saying its supposed to be positive? then what problem can this change cause? It happened to be negative in the past?

i suggest to move the reference counter into another pr and keep the fix simple

src/Neo/SmartContract/ApplicationEngine.Runtime.cs

AnnaShaleva · 2024-06-07T11:10:26Z

src/Neo.VM/ReferenceCounter.cs

+        private void DecreaseCounter(int count = 1)
+        {
+            references_count -= count;
+            if (references_count < 0) throw new InvalidOperationException("Reference counter is negative.");


AArray.DeepCopy doesn't increase the counter?

It should increase the counter:

neo/src/Neo.VM/Types/Array.cs

Line 75 in 6e8c730

referenceCounter.AddReference(item, this);

AnnaShaleva · 2024-06-07T11:24:43Z

src/Neo.VM/ReferenceCounter.cs

+        private void DecreaseCounter(int count = 1)
+        {
+            references_count -= count;
+            if (references_count < 0) throw new InvalidOperationException("Reference counter is negative.");


if (references_count < 0) throw new InvalidOperationException("Reference counter is negative.");

I think we should not change this code. It's a bug in the handlers implementation, not in the reference counter logic. So we should fix the source problem, not its consequence.

AnnaShaleva · 2024-06-07T11:35:16Z

src/Neo/SmartContract/NotifyEventArgs.cs

@@ -69,7 +69,7 @@ public StackItem ToStackItem(ReferenceCounter referenceCounter)
            {
                ScriptHash.ToArray(),
                EventName,
-                State
+                State.OnStack ? State : State.DeepCopy(true)


It seems to me that this change is the one that actually fixes the problem. But the problem is that it requires one more hardfork if we care about safety since it changes the behaviour.

just apply to the hardfork you created. will update.

Hardfork has being applied.

* 'fix-3300' of github.com:Jim8y/neo: Fixed up `main.yml` (neo-project#3292) [Neo Core Style] Rename parameters, update comments, fix code style (neo-project#3294)

Jim8y · 2024-06-07T13:09:03Z

#3301 (comment)

I can not agree, counter can not and should not be negative, that should not be ensured by outside callers, but should be an internal feature, we have showed you the case where the counter can be negative, how could you sure that others who write the code will not cause the same problem.

Jim8y · 2024-06-07T13:12:41Z

#3301 (comment)

We know it can increase the counter, but it decrease after the deepcopy command finishes. As the deepcopied value is not on the stack.

cschuchardt88

How about a test?

dusmart · 2024-06-08T04:43:53Z

@dusmartcan you help to perform a test

Seems that master branch is not suitable for me. When I build the NEO.CLI locally on master branch, it keeps fail because it tries to create files under my /bin/ path. Information are like this Access to the path '/bin/Neo.CLI' is denied..

I'm going to apply these changes manually upon v3.7.4

dusmart · 2024-06-08T04:51:59Z

tested OK with applying diff to v3.7.4

diff --git a/src/Neo/SmartContract/NotifyEventArgs.cs b/src/Neo/SmartContract/NotifyEventArgs.cs
index 93c124ea..d99b0426 100644
--- a/src/Neo/SmartContract/NotifyEventArgs.cs
+++ b/src/Neo/SmartContract/NotifyEventArgs.cs
@@ -69,7 +69,7 @@ public StackItem ToStackItem(ReferenceCounter referenceCounter)
             {
                 ScriptHash.ToArray(),
                 EventName,
-                State
+                State.DeepCopy(true)
             };
         }
     }

neo> invoke 0x179ab5d297fd34ecd48643894242fc3527f42853 bingo [{"type":"Integer","value":2000}]
Error: You have to open the wallet first.
Invoking script with: 'AdAHEcAfDAViaW5nbwwUUyj0JzX8QkKJQ4bU7DT9l9K1mhdBYn1bUg=='
VM State: FAULT
Gas Consumed: 0.12349045
Result Stack: []
Error: MaxStackSize exceed: 152010

* 'fix-3300' of github.com:Jim8y/neo: Fix error when vc_redist is missing (neo-project#3293)

added

src/Neo/SmartContract/ApplicationEngine.Runtime.cs

This test ensures that NeoGo node doesn't have the DeepCopy problem described in neo-project/neo#3300 and fixed in neo-project/neo#3301. This problem leads to the fact that Notifications items are not being properly refcounted by C# node which leads to possibility to build an enormously large object on stack. Go node doesn't have this problem. The reason (at least, as I understand it) is in the fact that C# node performs objects refcounting inside the DeepCopy even if the object itself is not yet on stack. I.e. System.Runtime.Notify handler immediately adds references to the notification argumetns inside DeepCopy: https://github.com/neo-project/neo/blob/b1d27f0189861167b8005a7e40e6d8500fb48cc4/src/Neo.VM/Types/Array.cs#L108 https://github.com/neo-project/neo/blob/b1d27f0189861167b8005a7e40e6d8500fb48cc4/src/Neo.VM/Types/Array.cs#L75 Whereas Go node just performs the honest DeepCopy without references counting: https://github.com/nspcc-dev/neo-go/blob/b66cea5cccbcc8446312b012e29aaf2d1837430a/pkg/vm/stackitem/item.go#L1223 Going further, C# node clears refs for notification arguments (for array and underlying array items). System.Runtime.GetNotifications pushes the notificaiton args array back on stack and increments counter only for the external array, not for its arguments. Which results in negative refcounter once notificaiton is removed from the stack. The fix itself (https://github.com/Jim8y/neo/blob/f471c0542ddd8f527478fbdcda76a3ab9194b958/src/Neo/SmartContract/NotifyEventArgs.cs#L84) doesn't need to be ported to NeoGo because Go node adds object to the refcounter only at the moment when it's being pushed to stack by System.Runtime.GetNotifications handler. This object is treated as new object since it was deepcopied earlier by System.Runtime.Notify handler: https://github.com/nspcc-dev/neo-go/blob/b66cea5cccbcc8446312b012e29aaf2d1837430a/pkg/vm/stack.go#L178. Thus, no functoinal changes from the NeoGo side. And we won't intentionally break our node to follow C# pre-Domovoi invalid behaviour. Close #3484, close #3482. Signed-off-by: Anna Shaleva <[email protected]>

Add a note about System.Runtime.GetNotifications refcounting to Domovoi hardfork. Ref. neo-project/neo#3301 and #3485. Although NeoGo doesn't have anything to be updated, there's a behaviour difference between C# and Go nodes before Domovoi hardfork, it deserves a comment. Signed-off-by: Anna Shaleva <[email protected]>

* [Neo Core Bug]fix 3300 (#3301) * fix 3300 * update format * add state subitems to ref counter, with suggestion from DuSmart * apply hardfork * format * my mistake * fix hardfork * remove negative check * add unit test * apply anna's suggestion --------- Co-authored-by: Shargon <[email protected]> Co-authored-by: NGD Admin <[email protected]> * SmartContract: use executing contract state to check permissions (#3290) It's not correct to use an updated contract state got from native Management to check for the allowed method call. We need to use manifest from the currently executing context for that. It may be critical for cases when executing contract is being updated firstly, and after that it calls another contract. So we need an old (executing) contract manifest for this check. This change is moved under D hardfork to avoid state diff issues on nodes update. Although it should be noted that it's hard to meet the trigger criteria. A port of nspcc-dev/neo-go#3473. This bug was discovered during the similar problem described in nspcc-dev/neo-go#3471 and fixed in nspcc-dev/neo-go#3472. I've checked all other similar usages and the rest of them use proper contract state (executing one, not the Management's one). Signed-off-by: Anna Shaleva <[email protected]> Co-authored-by: Shargon <[email protected]> Co-authored-by: Jimmy <[email protected]> Co-authored-by: Vitor Nazário Coelho <[email protected]> * v3.7.5 * Neo.CLI: enable hardforks for NeoFS mainnet (#3240) Otherwise this configuration file is broken. Port changes from nspcc-dev/neo-go#3446. Signed-off-by: Anna Shaleva <[email protected]> * fix workflow & FS config * remove hardfork for fs testnet --------- Signed-off-by: Anna Shaleva <[email protected]> Co-authored-by: Jimmy <[email protected]> Co-authored-by: Shargon <[email protected]> Co-authored-by: NGD Admin <[email protected]> Co-authored-by: Anna Shaleva <[email protected]> Co-authored-by: Vitor Nazário Coelho <[email protected]>

Jim8y added 2 commits June 7, 2024 13:35

fix 3300

1f8e36a

update format

c8d9322

Jim8y added Waiting for Review Critical Issues (bugs) that need to be fixed ASAP Bug Used to tag confirmed bugs labels Jun 7, 2024

Jim8y requested a review from a team June 7, 2024 05:40

Jim8y requested review from shargon, cschuchardt88, vncoelho, roman-khimov, AnnaShaleva, superboyiii and NGDAdmin June 7, 2024 05:45

Jim8y added the Blocked This issue can't be worked at the moment label Jun 7, 2024

add state subitems to ref counter, with suggestion from DuSmart

81abdd3

AnnaShaleva reviewed Jun 7, 2024

View reviewed changes

shargon reviewed Jun 7, 2024

View reviewed changes

src/Neo/SmartContract/ApplicationEngine.Runtime.cs Outdated Show resolved Hide resolved

Merge branch 'master' into fix-3300

fde92e6

AnnaShaleva reviewed Jun 7, 2024

View reviewed changes

Jim8y added 3 commits June 7, 2024 20:30

apply hardfork

ee4c159

Merge branch 'fix-3300' of github.com:Jim8y/neo into fix-3300

d8e12be

* 'fix-3300' of github.com:Jim8y/neo: Fixed up `main.yml` (neo-project#3292) [Neo Core Style] Rename parameters, update comments, fix code style (neo-project#3294)

format

c723ae1

Jim8y removed the Blocked This issue can't be worked at the moment label Jun 7, 2024

Jim8y added the Ready to Merge label Jun 7, 2024

Jim8y mentioned this pull request Jun 7, 2024

[Neo VM Bug]Fix negative counter issue #3304

Closed

15 tasks

shargon previously approved these changes Jun 7, 2024

View reviewed changes

Merge branch 'master' into fix-3300

afc5349

cschuchardt88 previously requested changes Jun 7, 2024

View reviewed changes

Jim8y added 2 commits June 8, 2024 18:46

add unit test

6afd380

Merge branch 'fix-3300' of github.com:Jim8y/neo into fix-3300

2e742cc

* 'fix-3300' of github.com:Jim8y/neo: Fix error when vc_redist is missing (neo-project#3293)

Jim8y dismissed stale reviews from vncoelho and shargon via 2e742cc June 8, 2024 10:47

Jim8y commented Jun 8, 2024

View reviewed changes

src/Neo/SmartContract/ApplicationEngine.Runtime.cs Outdated Show resolved Hide resolved

AnnaShaleva mentioned this pull request Jun 10, 2024

Ensure System.Runtime.GetNotifications handler operates with a deep copy of notifications nspcc-dev/neo-go#3484

Closed

apply anna's suggestion

6529bfa

Jim8y mentioned this pull request Jun 11, 2024

[V3.7.5 Checklist] This is a checklist for version 3.7.5 hotfix #3321

Closed

6 tasks

Jim8y added this to the v3.7.5 milestone Jun 11, 2024

Merge branch 'master' into fix-3300

f471c05

NGDAdmin approved these changes Jun 11, 2024

View reviewed changes

cschuchardt88 approved these changes Jun 11, 2024

View reviewed changes

NGDAdmin merged commit b1d27f0 into neo-project:master Jun 11, 2024
6 of 7 checks passed

dusmart mentioned this pull request Jun 11, 2024

make it possible to build large objects lazynode/Tanya#33

Merged

shargon mentioned this pull request Jun 11, 2024

[Neo VM Bug] Fix negative counter issue #3327

Closed

15 tasks

AnnaShaleva mentioned this pull request Jun 11, 2024

How to handle [possible] negative VM refcounter #3328

Open

AnnaShaleva mentioned this pull request Jun 11, 2024

Ensure System.Runtime.GetNotifications can't break MaxStackSize constraint nspcc-dev/neo-go#3485

Merged

AnnaShaleva mentioned this pull request Jun 11, 2024

docs: add a note about System.Runtime.GetNotifications refcounting to HFDomovoi docs nspcc-dev/neo-go#3486

Merged

Jim8y deleted the fix-3300 branch July 19, 2024 01:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Neo Core Bug]fix 3300 #3301

[Neo Core Bug]fix 3300 #3301

Jim8y commented Jun 7, 2024

dusmart commented Jun 7, 2024

dusmart commented Jun 7, 2024 •

edited

Loading

AnnaShaleva Jun 7, 2024

Jim8y Jun 7, 2024 •

edited

Loading

shargon Jun 7, 2024

AnnaShaleva Jun 7, 2024

AnnaShaleva Jun 7, 2024

AnnaShaleva Jun 7, 2024

Jim8y Jun 7, 2024 •

edited

Loading

vang1ong7ang Jun 7, 2024

AnnaShaleva Jun 7, 2024

AnnaShaleva Jun 7, 2024

AnnaShaleva Jun 7, 2024

Jim8y Jun 7, 2024

Jim8y Jun 7, 2024

Jim8y commented Jun 7, 2024

Jim8y commented Jun 7, 2024

cschuchardt88 left a comment

dusmart commented Jun 8, 2024 •

edited

Loading

dusmart commented Jun 8, 2024

[Neo Core Bug]fix 3300 #3301

[Neo Core Bug]fix 3300 #3301

Conversation

Jim8y commented Jun 7, 2024

Description

Type of change

How Has This Been Tested?

Checklist:

dusmart commented Jun 7, 2024

dusmart commented Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Jim8y Jun 7, 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

Jim8y Jun 7, 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

Jim8y commented Jun 7, 2024

Jim8y commented Jun 7, 2024

cschuchardt88 left a comment

Choose a reason for hiding this comment

dusmart commented Jun 8, 2024 • edited Loading

dusmart commented Jun 8, 2024

dusmart commented Jun 7, 2024 •

edited

Loading

Jim8y Jun 7, 2024 •

edited

Loading

Jim8y Jun 7, 2024 •

edited

Loading

dusmart commented Jun 8, 2024 •

edited

Loading