refraction-networking / conjure

Conjure Refraction Networking station code
https://refraction.network
Apache License 2.0
66 stars 19 forks source link

Halfpipe buffering fix for oob slice access. #255

Closed jmwample closed 8 months ago

jmwample commented 8 months ago

It is somehow possible for the proxy to make an out of bounds access in the halfpipe slice creation. It is not clear how this is possible since we are calling read with a fixed size buffer, but the issue seems to have occurred on several stations independently.

12:44:51 conjure: panic: runtime error: slice bounds out of range [:32804] with capacity 32768
12:44:51 conjure: goroutine 192061459 [running]:
12:44:51 conjure: github.com/refraction-networking/conjure/pkg/station/lib.halfPipe({0xbcdb28, 0xc003cac040}, {0xbcd618, 0xc0164223c0}, 0xc009744520, 0xc000246b20, {0xc01682a8b8, 0x13}, 0xc005a89680)
12:44:51 conjure:         /opt/conjure/conjure/pkg/station/lib/proxies.go:158 +0x870
12:44:51 conjure: created by github.com/refraction-networking/conjure/pkg/station/lib.Proxy in goroutine 192061268
12:44:51 conjure:         /opt/conjure/conjure/pkg/station/lib/proxies.go:266 +0x5a5

The offending code is here

    buf := make([]byte, 32*1024)
    for {
        nr, er := src.Read(buf)
        if nr > 0 {
            nw, ew := dst.Write(buf[0:nr])

            // Update stats:
            stats.addBytes(int64(nw), isUpload)
            if isUpload {
                Stat().AddBytesUp(int64(nw))
            } else {
                Stat().AddBytesDown(int64(nw))
            }

            if ew == nil && nw != nr {
                ew = io.ErrShortWrite
            }

This PR adds changes that:

  1. move from managing the buffered reads and writes ourselves to wrapping io.Readers and io.Writers and using io.CopyBuffer.
    1. Use wrapped readers to catch the transfer rates each time Read() is called (allowing the same transfer rate granulatity as before
    2. Wraps the readers and writers to capture the first error that occurs, so we can retain error granularity without interfering with the copy.
      • io.CopyBuffer only returns an error, it does not indicate whether the Read() or the Write() end caused the error
  2. While not tested, io.Copy can fuse sockets when appropriate so transfer rates may be faster this way.
  3. (-) More complex construction

PR #254 provides a simpler (but potentially less reliable?) solution to the same issue

jmwample commented 8 months ago

This is being closed in favor of #254. In testing this had issues relating to correct bidirectional transfer and the added complexity is not worth it at the moment.