Skip to content

fix: Read() data truncation issue #142

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
clementnuss wants to merge 2 commits into from
Closed

fix: Read() data truncation issue #142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f4b89e0
test: Read() truncation issue for large headers
clementnuss Feb 2, 2026
979a453
fix: Read() data truncation issue
clementnuss Feb 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions protocol.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ type Conn struct {
readErr error
conn net.Conn
bufReader *bufio.Reader
reader io.Reader
header *Header
ProxyHeaderPolicy Policy
Validate Validator
Expand Down Expand Up @@ -161,7 +160,6 @@ func NewConn(conn net.Conn, opts ...func(*Conn)) *Conn {

pConn := &Conn{
bufReader: br,
reader: io.MultiReader(br, conn),
conn: conn,
}

Expand All @@ -183,7 +181,21 @@ func (p *Conn) Read(b []byte) (int, error) {
return 0, p.readErr
}

return p.reader.Read(b)
// First drain any buffered data from header parsing,
// then read directly from the underlying connection.
n := 0
if p.bufReader != nil && p.bufReader.Buffered() > 0 {
n, _ = p.bufReader.Read(b)
if p.bufReader.Buffered() == 0 {
p.bufReader = nil
}
}

if n < len(b) {
nn, err := p.conn.Read(b[n:])
Copy link
Owner

@pires pires Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC this blocks waiting for the buffer to be filled which is somewhat of a violation of the io.Reader contract:

If some data is available but not len(p) bytes, Read conventionally returns what is available instead of waiting for more.

The bufio.Reader handled this.

The bytes are taken from at most one Read on the underlying Reader, hence n may be less than len(p). To read exactly len(p) bytes, use io.ReadFull(b, p).

Copy link
Contributor Author

@clementnuss clementnuss Feb 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it blocks more than what the previous code was doing. with a multireader, assuming you have no buffered data, the MultiReader would have received an EOF from the first reader (bufReader) and would then have called the next reader (reader) with a Read(b) call (which might have blocked as well).

Copy link
Owner

@pires pires Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC a bufio.Reader backed by an open net.Conn will only return io.EOF on .Read(b) when the net.Conn is closed.

return n + nn, err
}
return n, nil
Copy link
Owner

@pires pires Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following my comments from above, I think this logic can be improved

    // Drain the buffer.
    if p.bufReader != nil && p.bufReader.Buffered() > 0 {
        n, err := p.bufReader.Read(b)
        // If we got ANY data, return immediately. Do not block on conn.
        // We do not return EOF here because the underlying conn might have more.
        if err == io.EOF {
             p.bufReader = nil // Done with buffer
             return n, nil     // Return n, not EOF, so next Read hits p.conn
        }
        return n, err
    }

    // We're done with the buffer.
    // From now on, read directly from the underlying connection.
    return p.conn.Read(b)

Copy link
Contributor Author

@clementnuss clementnuss Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this version does not fix the bug.
my expectation, e.g. in ingress-nginx controller, when calling this library with a 16KiB buffer and using conn.Read(b), is that at most 16KiB will be read in a single call, and most importantly, that if say 2KiB are available on the connection (having been previously buffered or not), those 2KiB will end up in my buffer.

I don't want to use a io.ReadFull in the upstream code to achieve that, as I don't want necessarily need to fill the entire 16KiB buffer.

perhaps my expectation is incorrect, but I think the following code in ingress-nginx should not need a modification:
https://github.com/kubernetes/ingress-nginx/blob/a3088e571b36fee8052d550b7bc8c1c93bf7a1e7/pkg/tcpproxy/tcp.go#L60-L69

Copy link
Owner

@pires pires Feb 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In #142 (comment) you are agreeing with the assessment that conn.Read(b) returns whatever is available and doesn't block until b is full. What happened when the io.MultiReader was implemented was that we stopped piping the underlying conn directly to the library consumer because the buffer reads would never return EOF, hence the io.MultiReader would not iterate to the next io.Reader, namely the underlying conn.

Copy link
Owner

@pires pires Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic I proposed above is incorrect. Here's the revised one:

	// Drain the buffer if it exists and has data.
	if p.bufReader != nil {
		if p.bufReader.Buffered() > 0 {
			n, err := p.bufReader.Read(b)

			// Did we empty the buffer?
			// Buffering a net.Conn means the buffer doesn't return io.EOF until the connection returns io.EOF.
			// Therefore, we use Buffered() == 0 to detect if we are done with the buffer.
			if p.bufReader.Buffered() == 0 {
				// Garbage collect the buffer.
				p.bufReader = nil
			}

			// Return immediately. Do not touch p.conn.
			// If err is EOF here, it means the connection is actually closed,
			// so we should return that error to the user anyway.
			return n, err
		}
		// If buffer was empty to begin with (shouldn't happen with the >0 check
		// but good for safety), clear it.
		p.bufReader = nil
	}

	// From now on, read directly from the underlying connection.
	return p.conn.Read(b)

Copy link
Contributor Author

@clementnuss clementnuss Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I see your point about the MultiReader which never iterated to the underlying conn, so that data was always read in 256bytes chunks. that was not what I was bothered with in the first place.

this 2nd version is fixing that aspect, but it's still making this library act in a non-transparent way w.r.t. the user

}

// Write wraps original conn.Write.
Expand Down
54 changes: 54 additions & 0 deletions protocol_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1984,6 +1984,60 @@ func TestCopyFromWrappedConnectionToWrappedConnection(t *testing.T) {
}
}

// chunkedConn wraps a net.Conn and limits reads to simulate TCP chunking.
type chunkedConn struct {
net.Conn
maxRead int
}

func (c *chunkedConn) Read(b []byte) (int, error) {
if len(b) > c.maxRead {
b = b[:c.maxRead]
}
return c.Conn.Read(b)
}

// TestConnReadTruncatesData demonstrates that Conn.Read() only returns
// buffered data when the initial TCP read is smaller than the payload.
func TestConnReadTruncatesData(t *testing.T) {
const payloadSize = 400

proxyHeader := []byte("PROXY TCP4 192.168.1.1 192.168.1.2 12345 443\r\n")
payload := bytes.Repeat([]byte("X"), payloadSize)
fullData := append(proxyHeader, payload...)

serverConn, clientConn := net.Pipe()
defer func() {
serverCloseErr := serverConn.Close()
clientCloseErr := clientConn.Close()
if serverCloseErr != nil || clientCloseErr != nil {
t.Errorf("failed to close connection: %v, %v", serverCloseErr, clientCloseErr)
}
}()

go func() {
_, _ = clientConn.Write(fullData)
}()

// Simulate TCP delivering only 256 bytes in first read
chunked := &chunkedConn{Conn: serverConn, maxRead: 256}

// Create a ProxyProto-wrapped connection
conn := NewConn(chunked)
_ = conn.SetReadDeadline(time.Now().Add(time.Second))

buf := make([]byte, 16384)
n, _ := conn.Read(buf)
Copy link
Owner

@pires pires Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where our understanding of io.Reader.Read(b) contract is different. In my understanding, Read(b) returns available data immediately instead of waiting to fill b. A way to ensure b is filled is to rely on io.ReadFull.

Copy link
Contributor Author

@clementnuss clementnuss Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is the case for a typical reader. here however your library is acting as a transparent "pipe" between an underlying conn from which we abstract away the proxy protocol header. therefore I would think it's reasonable to also transparently forward the Read() call to the underlying Conn, as if the library hadn't done anything.

also, if b is a 16KiB buffer and I use conn.Read(b), it's not going to wait until b is filled: if 4KiB are ready (typical linux socket buffer size), then it'll receive 4096, nil.

Copy link
Owner

@pires pires Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mislead you for some weird reason. The theoretical maximum size of a v2 header is 65,551 bytes (16 bytes fixed + 65,535 bytes variable).


t.Logf("Sent: %d bytes payload (after %d byte PROXY header)", payloadSize, len(proxyHeader))
t.Logf("Read: %d bytes", n)

if n < payloadSize {
t.Errorf("BUG: Read returned %d bytes, expected %d (lost %d bytes)",
n, payloadSize, payloadSize-n)
}
}

func benchmarkTCPProxy(size int, b *testing.B) {
// create and start the echo backend
backend, err := net.Listen("tcp", testLocalhostRandomPort)
Expand Down