Send timeout might corrupt outgoing messages

[Matthias247]

Hi everyone,

I just reviewed the library a little bit since I'm also interested in stream multiplexing (actually want something for websockets instead of plain TCP, but I guess yamux would work there too). Overall it looks really well written and I like the fact that you have a spec :-)

One thing that I stumbled upon during review was there might be situations where the send timeout for the session might corrupt outgoing data: If we look at the waitForSendErr function it will start the timout and try to send the data in parallel be handing it over to the send channel.

The select/timeout logic looks like it introduces a data race to me: If the timeout is expiring here the send will still be in progress. That means the data (header and body) will still be in use in the send routine. However the waitForSendErr call returns with an the calling stream could think the data is no longer required and reuse it. It might probably not do it for sendHdr as it sees that writes fail and no further writes and sendHdr mutations follow, but the payload data slice might be reused by the application and mutated - while the send is still in progress. As an end result you could send data to the wire that was not desired.

Fixing this is probably not that easy. I guess that there probably could be some cancellation or sendInProgress flags in the enqueued sendReady struct. If the message was not yet fetched by the writing proc it can be marked as cancelled and will there no longer be processed. However as soon as the writer goroutine catches it the safest thing is to let it write to completion as there is no kind of CancellationToken that can be passed to to the Write or Copy methods.

[dosgo]

I also encountered this problem!

[mafredri]

I believe I've identified the same underlying issue, which I documented in https://github.com/hashicorp/yamux/pull/100. AFAICT, sendHdr is not guaranteed to be protected on access, especially considering sendCh is buffered.