Should `StreamMuxerBox` track how many "active" substreams are still around?

[thomaseizinger]

Description

Whilst thinking about #2863, I noticed that it is very easy to (accidentally) circumvent the limit on inbound substreams by "leaking" them to the ConnectionHandler. We do this in various places within rust-libp2p and I am sure it happens in within code-bases of ours users too.

StreamMuxerBox already boxes up each substream as a SubstreamBox. We could extend SubstreamBox with a Weak<()> where StreamMuxerBox owns the corresponding Arc<>. This would make Arc::weak_count effectively be the count of all active (i.e. not dropped) substreams the muxer has given out.

Within Connection, we know about StreamMuxerBox so we could expose functions on it to give us the current count of active inbound and outbound streams, which would allow us to actually enforce a limit on those.

Motivation

Not limiting the number of inbound streams can cause unexpected memory and CPU growth and in the worst case, is exploitable via a DoS attack.

Downsides

Rolling out this change may negatively affect users because they will suddenly experience a limit where there wasn't one before.

Current Implementation

Are you planning to do it yourself in a pull request?

Yes.

[thomaseizinger]

Tagging as "help wanted" because I'd like to get people's input on this idea.

[mxinden]

I like the idea. Though instead of using the Arc::weak_count to enforce a limit, I suggest using it to build a Prometheus metric. I would imagine a Connection emits an event whenever a new stream is created. That event would as well contain the overall count of streams on that connection.

I am against using Arc::weak_count as a limit as I find it unintuitive. We pass ownership of the stream to the ConnectionHandler implementation, thus I think it is the ConnectionHandler that has to manage the resource impact.

[thomaseizinger]

I like the idea. Though instead of using the Arc::weak_count to enforce a limit, I suggest using it to build a Prometheus metric. I would imagine a Connection emits an event whenever a new stream is created. That event would as well contain the overall count of streams on that connection.

I am against using Arc::weak_count as a limit as I find it unintuitive. We pass ownership of the stream to the ConnectionHandler implementation, thus I think it is the ConnectionHandler that has to manage the resource impact.

The issue I see with that is that we don't know, when a stream is de-allocated unless we use some kind of shared, atomic counter that is decremented on Drop. Once we pass the stream to the ConnectionHandler, we lose control over it.

[mxinden]

I think my comment was misleading @thomaseizinger. Let me rephrase:

I am against using Arc::weak_count as a limit as I find it unintuitive.

I am against using Arc::weak_count as a limit. I am in favor of using Arc::weak_count for metrics.

Sorry. Is the second attempt more clear?

[thomaseizinger]

Sort of. I wouldn't know how to enforce a limit via Arc::weak_count?

What I am proposing in this issue is to use expose functions on StreamMuxerBox:

impl StreamMuxerBox {
	pub fn active_inbound_streams(&self) -> usize;
	pub fn active_outbound_streams(&self) -> usize;
}

The implementation will return Arc::weak_count of an Arc that is embedded in every SubstreamBox and thus happen transparently for each muxer. What clients like swarm::Connection do with this information is up to them and not further defined in this issue. We can of course implement a metric on top of that.

The motivation for this issue was to use it in swarm::Connection as the guard for calling StreamMuxer::poll_inbound which should be more robust than the current implementation which only enforces how many substreams are concurrently upgrading. If a protocol does not perform its IO in the InboundUpgrade trait but passes the substream to the ConnectionHandler, this limit is by-passed.

[thomaseizinger]

Draft implementation here: #2878

I noticed that we are unnecessarily exposing the SubstreamBox::new API publicly which is currently used in a test in libp2p-swarm. That test changes with #2861 though so I am planning to remove the SubstreamBox::new API in order to not unnecessarily expose the implementation detail of tracking active streams via Arcs.