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

Deserialize limit #870

Merged
merged 3 commits into from
Jun 26, 2019
Merged

Deserialize limit #870

merged 3 commits into from
Jun 26, 2019

Conversation

shargon
Copy link
Member

@shargon shargon commented Jun 26, 2019

Before the maximum was 16777216
But in ExecutionEngine is 1048576

@shargon shargon requested a review from erikzhang June 26, 2019 12:12
@codecov-io
Copy link

codecov-io commented Jun 26, 2019 •
edited
Loading

Codecov Report

Merging #870 into master will not change coverage.
The diff coverage is 80%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #870   +/-   ##
=======================================
  Coverage   43.53%   43.53%           
=======================================
  Files         177      177           
  Lines       12566    12566           
=======================================
  Hits         5471     5471           
  Misses       7095     7095
Impacted Files Coverage Δ
neo/SmartContract/InteropService.cs 20.98% <0%> (ø) ⬆️
neo/SmartContract/Helper.cs 66.82% <100%> (ø) ⬆️
...eo/SmartContract/Native/Tokens/Nep5AccountState.cs 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 897df9c...a99c968. Read the comment docs.

@erikzhang
Copy link
Member

They have already been limited.

neo/neo/SmartContract/InteropService.cs

Lines 211 to 212 in 897df9c

if (serialized.Length > engine.MaxItemSize)
return false;

neo/neo/SmartContract/InteropService.cs

Line 233 in 897df9c

item = engine.CurrentContext.EvaluationStack.Pop().GetByteArray().DeserializeStackItem(engine.MaxArraySize);

@shargon
Copy link
Member Author

shargon commented Jun 26, 2019

This solve the problem in deserialization, because the binary data could be "hand made"

@shargon shargon merged commit 0c0e895 into neo-project:master Jun 26, 2019
@shargon shargon deleted the limit-deserialize branch June 26, 2019 14:33
vncoelho
vncoelho reviewed Jun 26, 2019
View reviewed changes
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

Good protections!

@shargon shargon mentioned this pull request Dec 16, 2019
Merged
Thacryba pushed a commit to simplitech/neo that referenced this pull request Feb 17, 2020
@Celia18305
fix links (neo-project#870)
13fab70
Tommo-L pushed a commit to Tommo-L/neo that referenced this pull request Jun 22, 2020
@shargon @Tommo-L
Deserialize limit (neo-project#870)
5a54eae 
* Deserialize limit

* Delete optional parameters

* Use `ExecutionEngine.MaxSizeForBigInteger`
@roman-khimov roman-khimov mentioned this pull request Oct 7, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vncoelho vncoelho vncoelho left review comments

@erikzhang erikzhang erikzhang approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@shargon @codecov-io @erikzhang @vncoelho