Inconsistent Value Validation of sendWindowCapacity

[frystal]

Version:

js-libp2p-yamux v5.0.0

Platform:

Ubuntu20.04

Subsystem:

yamux

Severity:

Low

Description:

Following the libp2p Specification, both sendWindowCapacity and recvWindowCapacity should have a maximum value of uint32.MAX - 1. However, the library currently only checks if recvWindowCapacity exceeds this threshold.

It correctly identifies the issue with recvWindowCapacity.

It does not perform a similar check for sendWindowCapacity

In the go-implement, sendWindow is strictly restricted within the uint32 threshold.

I think it should be a bug, thus the library should also enforce that sendWindowCapacity adheres to the uint32 threshold when receiving remote window update messages.

Steps to reproduce the error: None

[wemeetagain]

I don't see in the yamux specs where it says that either the send or recv window capacity needs to be a uint32.

I think the main implementation consideration is that we don't send any overflowed number over the wire. In the case of the sendWindowCapacity, this is something we track that's incremented based on window update messages we receive (and decremented based on messages we send).

So I don't see the reason, at least according to the spec, or implementation considerations, that we should artificially limit the capacity to uint32, or do extra work to enforce that.

[frystal]

My concern is that it might lead to inconsistency between the sendWindowCapacity and recvWindowCapacity of two nodes. The code currently only restricts the recvWindowCapacity from exceeding the 'maxStreamWindowSize', but it does not impose a restriction on the sendWindowCapacity. I think that if the code restricts recvWindowCapacity, it should also impose a restriction on sendWindowCapacity. This is important because the remote peer might not adhere to the same restriction method as the js implementation, potentially leading to inconsistencies in window sizes between the two nodes.

[wemeetagain]

Thats a valid concern, but I don't think that limiting sendWindowCapacity necessarily helps. In fact, barring a spec requirement, I think having an uncapped sendWindowCapacity is possibly more correct. If desynchronization happens as a result of an overflow, that would be a bug in the implementation that sent the window update.

Walking thru the scenario:

1. our sendWindowCapacity starts synchronized with the peer's "receive window" at 256k
2. the peer decides that its "receive window" can be increased by N a. the peer increments its "receive window" by N b. the peer sends a window update (by N)
3. we receive the window update and increment our sendWindowCapacity by N

Lets assume N=2^32 - 256k. If we treat sendWindowCapacity strictly as a uint32, then the result rolls over to 0. If we treat sendWindowCapacity as a number, then the result is 2**32.

Given that we don't know what restrictions the peer has on its receive window, and the spec describes a window update as a delta used to increase the window size, I'd say the correct behavior is trust that the peer knows what its doing, and we should treat sendWindowCapacity as a number and not overflow.

If the peer actually is overflowing its own receive window, that would likely be a bug in its implementation, since the spec gives no provision for using window updates to shrink its receive window.