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.

Sign up for GitHub

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

Jump to bottom

Disallow changing notifications #1259

Merged
merged 2 commits into from
Aug 5, 2020
Merged

Disallow changing notifications #1259

merged 2 commits into from
Aug 5, 2020

Conversation

fyrchik
Copy link
Contributor

@fyrchik fyrchik commented Aug 4, 2020

Close #1199 .

roman-khimov
roman-khimov requested changes Aug 4, 2020
View reviewed changes
Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not enough to make a copy in runtimeNotify, another copy should be made in System.Runtime.GetNotifications handler.

pkg/vm/stackitem/item.go Outdated Show resolved Hide resolved
pkg/core/interop_system.go
@@ -282,7 +282,7 @@ func runtimeNotify(ic *interop.Context, v *vm.VM) error {
ne := state.NotificationEvent{
ScriptHash: v.GetCurrentScriptHash(),
Name: name,
Item: stackitem.NewArray(args),
Item: stackitem.DeepCopy(stackitem.NewArray(args)).(*stackitem.Array),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can avoid NewArray here, elem.Item() should be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because argument can be a struct, and we always store an array.

AnnaShaleva
AnnaShaleva previously requested changes Aug 5, 2020
View reviewed changes
pkg/vm/stackitem/item.go Outdated Show resolved Hide resolved
pkg/vm/stackitem/item.go Show resolved Hide resolved
fyrchik added 2 commits August 5, 2020 11:19
@fyrchik
stackitem: implement DeepCopy
b8cc33d
@fyrchik
core: deepcopy notification in System.Runtime.Notify
194da64 
Make it impossible to change already emitted notifications via
`System.Runtime.GetNotifications`.
@roman-khimov roman-khimov merged commit f5fbef1 into master Aug 5, 2020
@roman-khimov roman-khimov deleted the fix/notifications branch August 5, 2020 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roman-khimov roman-khimov roman-khimov approved these changes

@AnnaShaleva AnnaShaleva Awaiting requested review from AnnaShaleva AnnaShaleva is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Disallow changing notifications
3 participants
@fyrchik @roman-khimov @AnnaShaleva