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

Fix: Limit the number in the slab #622

Closed
wants to merge 2 commits into from
Closed

Fix: Limit the number in the slab #622

wants to merge 2 commits into from

Conversation

silence-coding
Copy link
Contributor

FIX: #621

@silence-coding
Copy link
Contributor Author

@seanmonstar @carllerche

@seanmonstar seanmonstar requested a review from hawkw June 10, 2022 00:23
@silence-coding
Copy link
Contributor Author

@hawkw Thank you.

@silence-coding
Copy link
Contributor Author

silence-coding commented Jun 13, 2022 •
edited
Loading

@hawkw Hello, may I ask when you are free to check it out? I think this serious ddos problem should be avoided or fixed as soon as possible.

hawkw
hawkw reviewed Jun 13, 2022
View reviewed changes
Copy link
Collaborator

@hawkw hawkw left a comment •
edited
Loading

Choose a reason for hiding this comment

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

hmm, is it really correct to GOAWAY the whole connection in this case? sending a GOAWAY will tell the client it can never initiate new streams on this connection, but those slab indices will become available again (in clear_expired_reset_streams). so, this condition is transient. is there a reason we can't reset the new stream rather than GOAWAYing the whole connection? it is entirely possible i'm overlooking something here, i haven't looked at this code in a while...

src/proto/streams/streams.rs Outdated Show resolved Hide resolved
src/proto/streams/streams.rs Outdated Show resolved Hide resolved
@seanmonstar
Copy link
Member

It seems in fn open(), we check that the number of currently active received streams is under the max, and if at the limit, then the stream is rejected (likely reset). With this new change, the stream would be accepted, but if we had too many pending reset streams, the connection would go away. That seems... aggressive...

Then again, if a user has decided to have no more than some limit, it's probably to reduce resource usage... Resetting more streams would just add to the "too many"... The problem, I think, is if a remote has been keeping the local peer at the max, and then a stream is reset, it would pretty much immediately close it all down.

@seanmonstar
Copy link
Member

Shouldn't it be sufficient to just set the max_reset_streams to something? Anything over that limit should be forgotten immediately, no?

@silence-coding
Copy link
Contributor Author

hmm, is it really correct to GOAWAY the whole connection in this case? sending a GOAWAY will tell the client it can never initiate new streams on this connection, but those slab indices will become available again (in clear_expired_reset_streams). so, this condition is transient. is there a reason we can't reset the new stream rather than GOAWAYing the whole connection? it is entirely possible i'm overlooking something here, i haven't looked at this code in a while...

I'm just starting to get to know H2 and I'm not familiar with it. I just think it's safer to disconnect in this scenario. Developers can optimize this problem by tweaking max_concurrent_streams and max_concurrent_reset_streams.

@silence-coding
Copy link
Contributor Author

It seems in fn open(), we check that the number of currently active received streams is under the max, and if at the limit, then the stream is rejected (likely reset). With this new change, the stream would be accepted, but if we had too many pending reset streams, the connection would go away. That seems... aggressive...

Then again, if a user has decided to have no more than some limit, it's probably to reduce resource usage... Resetting more streams would just add to the "too many"... The problem, I think, is if a remote has been keeping the local peer at the max, and then a stream is reset, it would pretty much immediately close it all down.

@seanmonstar Sorry, I didn't understand you well. Is it a direct GOAWAY or REST_STREAM? Or something better.

@silence-coding
Copy link
Contributor Author

Who can answer the problem of the PR or submit a new PR with a better solution to fix the problem? #621

@silence-coding
Copy link
Contributor Author

Who can answer the problem of the PR or submit a new PR with a better solution to fix the problem? #621

@seanmonstar @hawkw Can someone answer that? If this problem is not solved, services built on the crate using h2 are at high ddos risk.

@silence-coding
Copy link
Contributor Author

silence-coding commented Jul 25, 2022 •
edited
Loading

@seanmonstar @hawkw How will the h2 community deal with this problem in the future? If you guys are busy, is there anyone else in the community familiar with this issue? #621

@silence-coding
Copy link
Contributor Author

Does anyone pay attention to this issue?

@silence-coding
Copy link
Contributor Author

@LucioFranco Hello, can this take a little of your precious time? Thank you.

LucioFranco
LucioFranco requested changes Oct 17, 2022
View reviewed changes
Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Hi sorry for the delay, I think we've had a lot of open issues and it takes some time to get to all of them.

src/proto/streams/streams.rs

if me.store.num_wired_streams() > me.counts.max_streams() {
tracing::error!("HEADERS: number of streams exceeds the upper limit ({})", me.counts.max_streams());
return Err(Error::remote_go_away(
Copy link
Member

Choose a reason for hiding this comment

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

Its not clear to me that this is the correct response as stated above. Would be good to get some reference from the spec or any other implementations for what they do here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't find anything more useful
https://www.rfc-editor.org/rfc/rfc7540#section-6.5.2

@LucioFranco
Copy link
Member

Would also be good to get CI to run against this change which may just need a new commit to trigger it.

@silence-coding
Copy link
Contributor Author

Sorry, I clicked re-request by mistake. @LucioFranco

@silence-coding
Copy link
Contributor Author

close by #668

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hawkw hawkw hawkw left review comments

@LucioFranco LucioFranco Awaiting requested review from LucioFranco

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

Successfully merging this pull request may close these issues.

Stream stacking occurs when H2 processes HTTP2 RST_STREAM frames.
4 participants
@silence-coding @seanmonstar @LucioFranco @hawkw