hashicorp / yamux

Golang connection multiplexing library
Mozilla Public License 2.0
2.19k stars 232 forks source link

stream recvWindow decremented incorretly #94

Open apmattil opened 3 years ago

apmattil commented 3 years ago

at stream.readData():

       if _, err := io.Copy(s.recvBuf, conn); err != nil {
        s.session.logger.Printf("[ERR] yamux: Failed to read stream data: %v", err)
        s.recvLock.Unlock()
        return err
    }

    // Decrement the receive window
    s.recvWindow -= length <-- this should be number of read bytes.
    s.recvLock.Unlock()

when there is connection breaks io.Copy() will return partial reads.

apmattil commented 3 years ago

also unlock missing


if length > s.recvWindow {
        s.session.logger.Printf("[ERR] yamux: receive window exceeded (stream: %d, remain: %d, recv: %d)", s.id, s.recvWindow, length)
                 <--- unlock here -->
        return ErrRecvWindowExceeded
    }
apmattil commented 3 years ago

same problem with session.send(), number of copied bytes from io.Copy() is not respected nor returned to e.g stream.Write()

jefferai commented 2 years ago

As someone who is not super familiar with the codebase can you explain your third comment? I can see how to fix comment 1 (which is in https://github.com/hashicorp/yamux/pull/107 -- mind a review?) and comment 2 has already been addressed. But it's not clear to me how and where things should be updated on the send side, possibly because it seems like for sending it's done async on a channel so I don't see a very obvious way to get information about what was actually sent.

apmattil commented 2 years ago

looks like 3th issue is already fixed. https://github.com/hashicorp/yamux/blob/aad893ec06bc46ab362e4168f05e5d1799efc5ca/stream.go#L480

I can not remember how I fixed the send part .. I'll get back about it.