Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure System.Runtime.GetNotifications can't break MaxStackSize constraint #3485

Merged
merged 3 commits into from
Jun 11, 2024

Conversation

AnnaShaleva
Copy link
Member

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:

arr.value[i] = deepCopy(it.value[i], seen, asImmutable)

Going further, C# node clears refs for notification arguments (for array and underlying array items). Sytem.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:

s.refs.Add(e.value)
.

Thus, no functional changes from the NeoGo side. And we won't intentionally break our node to follow C# pre-Domovoi invalid behaviour.

Close #3484, close #3482.

Allow []string to be converted to stackitem.Item.

Signed-off-by: Anna Shaleva <[email protected]>
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]>
Copy link

codecov bot commented Jun 11, 2024

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 86.10%. Comparing base (b66cea5) to head (effba1f).

Current head effba1f differs from pull request most recent head d156cea

Please upload reports for the commit d156cea to get more accurate results.

Files Patch % Lines
pkg/vm/stackitem/item.go 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3485      +/-   ##
==========================================
- Coverage   86.13%   86.10%   -0.04%     
==========================================
  Files         331      331              
  Lines       38482    38487       +5     
==========================================
- Hits        33146    33138       -8     
- Misses       3805     3818      +13     
  Partials     1531     1531              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

No functional changes, just add more details to the error.

Signed-off-by: Anna Shaleva <[email protected]>
@roman-khimov roman-khimov merged commit 02627e9 into master Jun 11, 2024
19 checks passed
@roman-khimov roman-khimov deleted the test-maxstacksize branch June 11, 2024 16:04
AnnaShaleva added a commit that referenced this pull request Jun 11, 2024
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants