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

Add ToJson overload #2671

Merged
merged 7 commits into from
Mar 12, 2022
Merged

Add ToJson overload #2671

merged 7 commits into from
Mar 12, 2022

Conversation

erikzhang
Copy link
Member

Alternative to #2669

@erikzhang
Copy link
Member Author

@shargon @Liaojinghui

src/neo/VM/Helper.cs Outdated Show resolved Hide resolved
src/neo/VM/Helper.cs Outdated Show resolved Hide resolved
src/neo/VM/Helper.cs Outdated Show resolved Hide resolved
src/neo/VM/Helper.cs Outdated Show resolved Hide resolved
@Jim8y
Copy link
Contributor

Jim8y commented Mar 9, 2022

I am not sure, but dont you think 4G is too big?

By the way, we still have another problem which is user can deploy a smart contract on N3 that keeps calling newbuffer, the maximum memory allowed is 2G, and the GAS to trigger this smart contract is 0.15GAS, the maximum memory consumed to execute such transaction is 2.3G on my private node.
@shargon
smart contract:

[push(1024*1024) newbuffer] * 2048 ret

@roman-khimov
Copy link
Contributor

the maximum memory allowed is 2G, and the GAS to trigger this smart contract is 0.15GAS, the maximum memory consumed to execute such transaction is 2.3G on my private node.

neo-project/neo-vm#245 and neo-project/neo-vm#369.

I'd still like to have more strict constraints (like ~128MB), but we're in 2022 now and every phone around has 4+GB, so maybe it's not worth the trouble? As for GAS cost, btw, 0.15GAS seems to be appropriate, it's mostly about CPU time spent and CPU doesn't have to do a lot of work to allocate 2G.

@Jim8y
Copy link
Contributor

Jim8y commented Mar 9, 2022

But think about that, you have a 4Gb json to process or a 2Gb value to persist. Indeed it is memory, but you gonna deal with this memory.

@roman-khimov
Copy link
Contributor

you have a 4Gb json

Where will it come from? Oracle responses are limited (IIRC, 64K), transaction size is limited too (100K, but script can only be 64K). Sure, you can construct some JSON in memory, but I think this activity will be limited by the maximum Buffer capacity that is 1M.

a 2Gb value to persist

With StoragePrice of 10000 (or 0.0001) that'd be more than 200K GAS which is ~1M$ at the moment. I think we know all people with enough GAS to try doing that by name and it's not likely they're interested in doing this.

@Jim8y
Copy link
Contributor

Jim8y commented Mar 9, 2022

I agree with everything you mentioned. However, we are not talking about usage cases, we are talking about attacks:

This exploit i constructed can generate a 1024 GB json

newbuffer, dup, dup,,,,,,,pack(from here, every stackitem is 1GB), dup, dup, dup, dup, dup

at

ConsoleHelper.Info("Result Stack: ", new JArray(engine.ResultStack.Select(p => p.ToJson())).ToString());
or anywhere that calls ToArray or ToJson

and it could be 2048TB if i deploy a smart contract.

Saddly Storage is not the only place where persist happens, Log also has file IO. And the issue would be worse if we have to answer a RPC request from user, imaging sending a 4Gb result to user.

@roman-khimov
Copy link
Contributor

This exploit i constructed can generate a 1024 GB json

Well, it won't after the fix (and appropriate limit of course).

Storage is not the only place persist happens, what about Logs

Sure, but this was all started by 0x40e171249642639be3e54dd6acfe5948b1d3f240f505e7a3403cdc9bd3e1068a mentioned in #2666. Notice that NeoGo had zero problems with this transaction, because the way we save application logs, stack is serialized into binary format that has appropriate size checks. We have a data loss in this particular case because this stack can't really be saved, but it's OK, the node is still fully operational. We had a problem with invoke* though (anticipated in some ways, we knew of the differences between two ways to JSONize stack items), OK, we've fixed it now.

The way I see it is we have some reasonable limits in the VM, as far as we know they work fine. There are some ways for data to get in/out of the VM and they're all well-known. Stack items can be serialized into binary or into JSON (in two different ways). As long as all these ways are protected we should be fine. If every plugin implements something of its own, that's plugin's problem. ToJson method from this PR is good in that every plugin can reuse it and be fine.

src/neo/VM/Helper.cs Outdated Show resolved Hide resolved
JArray result = new();
foreach (var item in stack)
result.Add(ToJson(item, null, ref maxSize));
if (maxSize < 0) throw new InvalidOperationException("Max size reached.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (maxSize < 0) throw new InvalidOperationException("Max size reached.");

src/neo/VM/Helper.cs Outdated Show resolved Hide resolved
shargon and others added 2 commits March 11, 2022 10:20
Co-authored-by: Jinghui Liao <jinghui@wayne.edu>
Co-authored-by: Jinghui Liao <jinghui@wayne.edu>
@shargon shargon merged commit ee29d38 into develop Mar 12, 2022
@shargon shargon deleted the stack-tojson branch March 12, 2022 08:05
@superboyiii superboyiii mentioned this pull request Mar 18, 2022
18 tasks
shargon added a commit that referenced this pull request Apr 6, 2022
* Add ToJson overload (#2671)

* Add ToJson overload

* change

* Update src/neo/VM/Helper.cs

* Update src/neo/VM/Helper.cs

* Update src/neo/VM/Helper.cs

* Update src/neo/VM/Helper.cs

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

* Update src/neo/VM/Helper.cs

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

Co-authored-by: Shargon <shargon@gmail.com>
Co-authored-by: Jinghui Liao <jinghui@wayne.edu>

* Fix oom (#2665)

* Fix oom

* Revert reorder

* parameters order

Co-authored-by: Erik Zhang <erik@neo.org>

* Optimize inventory (#2659)

* add `murmur32` to crypto lib (#2604)

* 3.2.0

* fix

Co-authored-by: Shargon <shargon@gmail.com>
Co-authored-by: Jinghui Liao <jinghui@wayne.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants