search

hyperium / h2

HTTP 2.0 client & server implementation for Rust.
MIT License
1.34k stars 269 forks source link

Fix: Limit the number in the slab #622

Closed silence-coding closed 1 year ago

silence-coding commented 2 years ago

FIX: https://github.com/hyperium/h2/issues/621

silence-coding commented 2 years ago

@seanmonstar @carllerche

silence-coding commented 2 years ago

@hawkw Thank you.

silence-coding commented 2 years ago

@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.

seanmonstar commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Who can answer the problem of the PR or submit a new PR with a better solution to fix the problem? https://github.com/hyperium/h2/issues/621

silence-coding commented 2 years ago

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 commented 2 years ago

@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? https://github.com/hyperium/h2/issues/621

silence-coding commented 2 years ago

Does anyone pay attention to this issue?

silence-coding commented 1 year ago

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

LucioFranco commented 1 year ago

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

silence-coding commented 1 year ago

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

silence-coding commented 1 year ago

close by https://github.com/hyperium/h2/pull/668