go-libp2p-pubsub
go-libp2p-pubsub copied to clipboard
libp2p
Limit the number of inbound streams
Well, it's not a correctness problem, but indeed a malicious peer could cause us to allocate an unlimited number of streams so it is a bug. We can add a check in the stream handler that goes through the open streams in the connection and closes it if there is already an open one.
We can add a check in the stream handler that goes through the open streams in the connection and closes it if there is already an open one.
We can't quite do this, unfortunately, as stream closing is async (the other side might think the stream is closed already). We could try killing the old stream, but that would also be racy. Maybe wait for either the old stream to die, or for some timeout, and if we hit the timeout, close the new stream? Also maybe disconnect from peers that flood us with new streams?
I still think we actually can do it. There is nothing in the protocol that requires more than two streams and we use a single stream created at connection time. Granted, it might interfere with stop/start logic, but that would be a breaking change that is hard to implement anyway.
TL;DR: I think we should just drop all the extra streams.
We really can't.
- Starting with a single connection.
- Peer A opens a stream, peer B receives it.
- Peer A sees the connection die.
- Peer A creates a new connection.
- Peer B sees both connections (hasn't yet seen the first die).
- Peer A opens a new stream, peer B receives it and kills it.
- Peer B finally sees the original connection die, and the original stream dies.
Now there are no streams.
I meant looking at the number of streams within the connection. The new connection shouldn't be affected by the old streams.
Sure, but there are likely many reasons we could temporarily end up with multiple streams. For example, if we implement #332, we may open a stream, close it, then open a new one. The other side may see the new stream before it sees the old one close.
Basically, we cannot rely on the ordering of events across the network, except within a single stream.
I think that #332 is kind of hard to implement without breaking changes anyway.
Hm. Yes, we'd need to be able to react to protocol change events like we now do in the DHT.
- Peer B sees both connections (hasn't yet seen the first die).
I think peer B able to send a PING message to check the stream, isn't it?
In theory, yes. But the pubsub protocol would need to support a PING protocol for that to work. Even then, we'd still have to wait for a timeout to determine if the stream is actually live or dead. Given that, it's simpler to just wait the timeout, then cut one of the streams if both still appear to be live.
Honestly, thinking through this, I think we can just make a rule where:
- If the remote side resets the stream, we retry some number of times per time period.
- If we see a new stream, we cut the old stream. There's a small race here where we may pick the wrong stream, but that's fine because the other side will just open a new one.
I think we are overthinking this; if the remote closes the stream we currently do nothing. If we want to allow a retry, then we can allow up to two concurrent stream and force reset the third attempt.