Skip to content
This repository has been archived by the owner on Nov 22, 2023. It is now read-only.

Add buffer limit #512

Closed
wants to merge 1 commit into from
Closed

Conversation

Ashuaidehao
Copy link
Contributor

@Ashuaidehao Ashuaidehao commented Oct 18, 2023

The Buffer type in VM could occupy up to 2GB memory easily, which was abused by malicious users to slow down public nodes.
This PR try to limit max used size of Buffer up to 100MB in a single execution.

@shargon shargon mentioned this pull request Oct 18, 2023
@Jim8y
Copy link
Contributor

Jim8y commented Oct 18, 2023

Its already exploited?

@Ashuaidehao
Copy link
Contributor Author

Not found yet, but it can be exploited at any time.

@shargon
Copy link
Member

shargon commented Oct 18, 2023

With CAT it's possible to get similar results, please check #513

@roman-khimov
Copy link
Contributor

This 2GB limit is known for years and was always considered to be OK. Refs. #369, #245.

While I'd still like it to be smaller, additional calculations can affect performance and compatibility now. And 2GB is not a huge amount of memory these days, every phone around can handle it.

@Jim8y
Copy link
Contributor

Jim8y commented Oct 18, 2023

I agree with @roman-khimov. This should not be consisderd an issue unless it can casue OOM like what @vang1ong7ang and @dusmart did neo-project/neo#2527. Or, say, 100 or 500 or 1000 of such transactions could DOS the consensus.

@dusmart
Copy link

dusmart commented Oct 18, 2023

I agree with @roman-khimov. This should not be consisderd an issue unless it can casue OOM like what @vang1ong7ang and @dusmart did neo-project/neo#2527. Or, say, 100 or 500 or 1000 of such transactions could DOS the consensus.

I recall that we indeed abused the buffer by using lots of opcodes related to it. Seems that newbuffer is still slow enough (v3.6.0 cost about 6 minutes in my recent test, it has been optimized a lot because v3.4.0 cost 37 minutes).

At that experiment, what we did is purely calling newbuffer and drop again and again. The memory initiation and free cost a lot of time. The 2GB itself didn't matter too much.

neo-project/neo#2723

BTW, sorry if I offend you, I don't like this PR's idea of limiting here and there. Makes things harder and harder.

@roman-khimov
Copy link
Contributor

Yeah, just wanted to say that some quick allocation/deallocation patterns can be problematic irrespective of the size. This heavily depends on a lot of things (node, runtime, libraries, OS), but I don't think we know of any particularly bad scenario now (NEWBUFFER test from neo-project/neo#2723 is worst we know of probably).

@shargon shargon mentioned this pull request Oct 19, 2023
Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

MemorySize is not free during pop

@Ashuaidehao
Copy link
Contributor Author

Ashuaidehao commented Oct 19, 2023

MemorySize is not free during pop

Yes, this PR's MemorySize was used like memory gas fee, only concern about how much buffer it created. So cannot use "create and drop" mode to evade the limit.

@vang1ong7ang
Copy link
Contributor

2GB is acceptable if it is charged correctly imo

@Jim8y Jim8y closed this in #514 Oct 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants