Introduce a queue that allows to respect peer window size #817
Conversation
vlad9486
force-pushed
the
fix/yamux-backpressure
branch
3 times, most recently
from
02a5f15 to
2f6a516
Compare
vlad9486
force-pushed
the
fix/yamux-backpressure
branch
from
2f6a516 to
7a942be
Compare
| use std::ops::Sub; | ||
|
|
||
| if let YamuxFrameInner::Data(data) = &mut self.inner { | ||
| if data.len() == pos { |
There was a problem hiding this comment.
Maybe here should be if data.len() <= pos
| .update_window(false, difference); | ||
| .or_insert_with(YamuxStreamState::incoming); | ||
| stream.update_window(false, difference); | ||
| if difference > 0 { |
There was a problem hiding this comment.
this can be 0 or negative? in which cases does that happen?
There was a problem hiding this comment.
In my implementation it is signed, but in go implementation it is unsigned, so the window could only grow. So, this is just sanity check, maybe redundant. Peer cannot harm us by making its own window too big.
There was a problem hiding this comment.
ok, please add a bug_condition! call in the else branch so that if this ever happens for some reason we will know about it. That is something we want to do in general, to avoid such unexpected (unenforced) cases happening silently without us noticing
There was a problem hiding this comment.
I add a runtime error, not bug_condition, because peer can send us this and we should not crash.
There was a problem hiding this comment.
You are right, this is based on external input, so it is not exactly a bug_condition!. Btw bug_condition only panics if a flag is enabled in the environment, otherwise it just logs an error (but again this specific case is not for it)
| } | ||
| } | ||
| YamuxFrameInner::WindowUpdate { .. } => { | ||
| while let Some(frame) = pending_outgoing.pop_front() { |
There was a problem hiding this comment.
Is there any bound or practical limit in how many elements we can have in pending_outgoing?
I'm concerned that someone could try to fill the dispatch queue and and cause blockings in the state machine
| pub writable: bool, | ||
| pub window_theirs: u32, | ||
| pub window_ours: u32, | ||
| pub pending: VecDeque<YamuxFrame>, |
There was a problem hiding this comment.
We should make this bounded
There was a problem hiding this comment.
Yes, I missed this
| // either the window cannot accept any byte, | ||
| // or the queue is already not empty | ||
| // in both cases the whole frame goes in the queue and nothing to send | ||
| stream.pending.push_back(frame); |
There was a problem hiding this comment.
Need to add a limit here. Malicious peer can keep its window small and send an RPC request in a loop.
|
@dkuehr |
vlad9486
force-pushed
the
fix/yamux-backpressure
branch
from
8b5fd33 to
e54fb26
Compare
| } | ||
| } | ||
| YamuxFrameInner::WindowUpdate { difference } => { | ||
| if *difference < 0 { |
There was a problem hiding this comment.
@vlad9486 what about 0 ? that case is not covered. The other condition uses > 0 and this one < 0, which means there is no handling for 0
There was a problem hiding this comment.
@tizoc Yes, initially I wrote <=, but apparently the peer sends window update with zero if it wants to open the stream but not send data, so it sends flag SYN. Of course it could send data with zero length body, but it sends window update.
3 participants
No description provided.