From a3eaef409b49bbb2144f4c9bb9b90032dfa0e56a Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Thu, 16 Apr 2026 16:26:15 -0600 Subject: [PATCH 1/3] Fix sysvar buffer reuse --- AUTHORS | 2 + connection.go | 7 ++- connection_test.go | 137 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 145 insertions(+), 1 deletion(-) diff --git a/AUTHORS b/AUTHORS index 05e71df48..42c7f02c0 100644 --- a/AUTHORS +++ b/AUTHORS @@ -92,6 +92,7 @@ Lunny Xiao Maciej Zimnoch Michael Woolnough Minh Quang +Morgan Tocker Nao Yokotsuka Nathanial Murphy Nicola Peduzzi @@ -136,6 +137,7 @@ Ziheng Lyu # Organizations Barracuda Networks, Inc. +Block, Inc. Counting Ltd. Defined Networking Inc. DigitalOcean Inc. diff --git a/connection.go b/connection.go index 5648e47d8..29603d8b5 100644 --- a/connection.go +++ b/connection.go @@ -477,7 +477,12 @@ func (mc *mysqlConn) getSystemVar(name string) ([]byte, error) { dest := make([]driver.Value, resLen) if err = rows.readRow(dest); err == nil { - return dest[0].([]byte), mc.skipRows() + // Copy the value before reading more packets. + // dest[0] is a slice into the read buffer, and skipRows + // may call fill() which moves data in the buffer, + // invalidating previously returned slices. + val := append([]byte(nil), dest[0].([]byte)...) + return val, mc.skipRows() } } return nil, err diff --git a/connection_test.go b/connection_test.go index 440ecbff7..bf97cd8be 100644 --- a/connection_test.go +++ b/connection_test.go @@ -15,6 +15,7 @@ import ( "errors" "net" "testing" + "time" ) func TestInterpolateParams(t *testing.T) { @@ -204,3 +205,139 @@ func (bc badConnection) Write(b []byte) (n int, err error) { func (bc badConnection) Close() error { return nil } + +// chunkedConn is a net.Conn that serves pre-built data chunks, one per Read +// call. This simulates the behavior seen with TLS connections, where the +// server's TLS library typically produces a separate TLS record per write +// and Go's crypto/tls.Read returns one record at a time. +type chunkedConn struct { + chunks [][]byte + readCount int +} + +func (c *chunkedConn) Read(b []byte) (int, error) { + if c.readCount >= len(c.chunks) { + return 0, errors.New("no more data") + } + n := copy(b, c.chunks[c.readCount]) + c.readCount++ + return n, nil +} + +func (c *chunkedConn) Write(b []byte) (int, error) { return len(b), nil } // swallow writes (e.g. COM_QUERY) +func (c *chunkedConn) Close() error { return nil } +func (c *chunkedConn) LocalAddr() net.Addr { return nil } +func (c *chunkedConn) RemoteAddr() net.Addr { return nil } +func (c *chunkedConn) SetDeadline(_ time.Time) error { return nil } +func (c *chunkedConn) SetReadDeadline(_ time.Time) error { return nil } +func (c *chunkedConn) SetWriteDeadline(_ time.Time) error { return nil } + +var _ net.Conn = (*chunkedConn)(nil) + +// makePacket wraps a payload in a MySQL protocol packet header. +func makePacket(seq byte, payload []byte) []byte { + pkt := make([]byte, 4+len(payload)) + pkt[0] = byte(len(payload)) + pkt[1] = byte(len(payload) >> 8) + pkt[2] = byte(len(payload) >> 16) + pkt[3] = seq + copy(pkt[4:], payload) + return pkt +} + +// TestGetSystemVarBufferReuse verifies that getSystemVar returns a value that +// is not corrupted by the subsequent skipRows/readUntilEOF call. +// +// Background: getSystemVar sends "SELECT @@" and reads the result using +// the low-level packet API. The row value is returned as a []byte slice that +// points directly into the read buffer (buffer.cachedBuf). After extracting +// the value, getSystemVar calls skipRows/readUntilEOF to consume the trailing +// EOF packet. If the EOF data is not already in the buffer, this triggers +// buffer.fill(), which reads new network data into cachedBuf starting at +// offset 0 — overwriting the memory the value slice still references. +// +// On plain TCP connections this bug is less commonly observed because +// small back-to-back writes may be coalesced into a single TCP segment +// (by Nagle's algorithm or kernel buffering), so both packets often +// arrive in one Read and fill() is not called again. However, this is +// not guaranteed — TCP_NODELAY (Go's default) disables Nagle, and +// packet boundaries depend on timing. +// +// With TLS the bug is much more likely: the server's TLS library +// typically produces a separate TLS record per write call, and Go's +// crypto/tls.Read returns one record at a time, so the row data and +// trailing EOF almost always arrive in separate Reads — triggering +// fill() and corrupting the value. +// +// The test feeds each protocol packet as a separate Read call via chunkedConn +// (mimicking TLS record boundaries), guaranteeing that fill() must be called +// for the trailing EOF. After fill() reads the 9-byte EOF into +// cachedBuf[0:8], it overwrites the row value which sits at cachedBuf[5:13] +// (4-byte packet header + 1-byte length prefix = value starts at offset 5 +// within the 4096-byte cachedBuf). +func TestGetSystemVarBufferReuse(t *testing.T) { + // Protocol response for: SELECT @@max_allowed_packet → "67108864" + // + // Sequence numbers start at 1 (client sent COM_QUERY as seq 0). + // + // seq 1: column count = 1 + // seq 2: column definition (minimal valid) + // seq 3: EOF (end of column defs) + // seq 4: row data — length-encoded string "67108864" + // seq 5: EOF (end of rows) + + colCountPkt := makePacket(1, []byte{0x01}) + + colDef := []byte{ + 0x03, 'd', 'e', 'f', // catalog = "def" + 0x00, // schema = "" + 0x00, // table = "" + 0x00, // org_table = "" + 0x14, // name length = 20 + '@', '@', 'm', 'a', 'x', '_', 'a', 'l', 'l', 'o', + 'w', 'e', 'd', '_', 'p', 'a', 'c', 'k', 'e', 't', + 0x00, // org_name = "" + 0x0c, // length of fixed fields + 0x3f, 0x00, // charset = 63 (binary) + 0x14, 0x00, 0x00, 0x00, // column_length = 20 + 0x0f, // type = FIELD_TYPE_VARCHAR + 0x00, 0x00, // flags + 0x00, // decimals + 0x00, 0x00, // filler + } + colDefPkt := makePacket(2, colDef) + + eof1 := makePacket(3, []byte{0xfe, 0x00, 0x00, 0x02, 0x00}) + + // Row: length-encoded string "67108864" (8 bytes → length prefix 0x08) + rowPkt := makePacket(4, []byte{0x08, '6', '7', '1', '0', '8', '8', '6', '4'}) + + eof2 := makePacket(5, []byte{0xfe, 0x00, 0x00, 0x02, 0x00}) + + // Each packet arrives in its own Read call, simulating TLS record + // boundaries where each server Write becomes a separate TLS record + // and each client Read returns exactly one record. + conn := &chunkedConn{chunks: [][]byte{colCountPkt, colDefPkt, eof1, rowPkt, eof2}} + + mc := &mysqlConn{ + netConn: conn, + buf: newBuffer(), + cfg: NewConfig(), + closech: make(chan struct{}), + maxAllowedPacket: defaultMaxAllowedPacket, + sequence: 1, // after COM_QUERY (seq 0) + } + + val, err := mc.getSystemVar("max_allowed_packet") + if err != nil { + t.Fatalf("getSystemVar failed: %v", err) + } + + const expected = "67108864" + if got := string(val); got != expected { + t.Fatalf("getSystemVar(max_allowed_packet) = %q (% 02x), want %q\n"+ + "Value was likely corrupted by buffer.fill() overwriting the "+ + "slice returned by readRow when reading the trailing EOF packet.", + got, val, expected) + } +} From 1b0d23597aa560e66e7b9ee96462210c4b33c717 Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Sat, 18 Apr 2026 19:40:08 -0600 Subject: [PATCH 2/3] incorporate changes from review --- connection.go | 17 ++++++-------- connection_test.go | 55 ++++++++++++++-------------------------------- connector.go | 2 +- 3 files changed, 25 insertions(+), 49 deletions(-) diff --git a/connection.go b/connection.go index 29603d8b5..b1660a508 100644 --- a/connection.go +++ b/connection.go @@ -453,12 +453,11 @@ func (mc *mysqlConn) query(query string, args []driver.Value) (*textRows, error) } // Gets the value of the given MySQL System Variable -// The returned byte slice is only valid until the next read -func (mc *mysqlConn) getSystemVar(name string) ([]byte, error) { +func (mc *mysqlConn) getSystemVar(name string) (string, error) { // Send command handleOk := mc.clearResult() if err := mc.writeCommandPacketStr(comQuery, "SELECT @@"+name); err != nil { - return nil, err + return "", err } // Read Result @@ -471,21 +470,19 @@ func (mc *mysqlConn) getSystemVar(name string) ([]byte, error) { if resLen > 0 { // Columns if err := mc.skipColumns(resLen); err != nil { - return nil, err + return "", err } } dest := make([]driver.Value, resLen) if err = rows.readRow(dest); err == nil { - // Copy the value before reading more packets. - // dest[0] is a slice into the read buffer, and skipRows - // may call fill() which moves data in the buffer, - // invalidating previously returned slices. - val := append([]byte(nil), dest[0].([]byte)...) + // Convert to string before skipRows, which may + // overwrite the read buffer that dest[0] points into. + val := string(dest[0].([]byte)) return val, mc.skipRows() } } - return nil, err + return "", err } // cancel is called when the query has canceled. diff --git a/connection_test.go b/connection_test.go index bf97cd8be..b468aa6ea 100644 --- a/connection_test.go +++ b/connection_test.go @@ -211,16 +211,21 @@ func (bc badConnection) Close() error { // server's TLS library typically produces a separate TLS record per write // and Go's crypto/tls.Read returns one record at a time. type chunkedConn struct { - chunks [][]byte - readCount int + chunks [][]byte + idx int // current chunk index + off int // offset within current chunk } func (c *chunkedConn) Read(b []byte) (int, error) { - if c.readCount >= len(c.chunks) { + if c.idx >= len(c.chunks) { return 0, errors.New("no more data") } - n := copy(b, c.chunks[c.readCount]) - c.readCount++ + n := copy(b, c.chunks[c.idx][c.off:]) + c.off += n + if c.off >= len(c.chunks[c.idx]) { + c.idx++ + c.off = 0 + } return n, nil } @@ -246,35 +251,12 @@ func makePacket(seq byte, payload []byte) []byte { } // TestGetSystemVarBufferReuse verifies that getSystemVar returns a value that -// is not corrupted by the subsequent skipRows/readUntilEOF call. -// -// Background: getSystemVar sends "SELECT @@" and reads the result using -// the low-level packet API. The row value is returned as a []byte slice that -// points directly into the read buffer (buffer.cachedBuf). After extracting -// the value, getSystemVar calls skipRows/readUntilEOF to consume the trailing -// EOF packet. If the EOF data is not already in the buffer, this triggers -// buffer.fill(), which reads new network data into cachedBuf starting at -// offset 0 — overwriting the memory the value slice still references. -// -// On plain TCP connections this bug is less commonly observed because -// small back-to-back writes may be coalesced into a single TCP segment -// (by Nagle's algorithm or kernel buffering), so both packets often -// arrive in one Read and fill() is not called again. However, this is -// not guaranteed — TCP_NODELAY (Go's default) disables Nagle, and -// packet boundaries depend on timing. -// -// With TLS the bug is much more likely: the server's TLS library -// typically produces a separate TLS record per write call, and Go's -// crypto/tls.Read returns one record at a time, so the row data and -// trailing EOF almost always arrive in separate Reads — triggering -// fill() and corrupting the value. +// is not corrupted by the subsequent skipRows call. // -// The test feeds each protocol packet as a separate Read call via chunkedConn -// (mimicking TLS record boundaries), guaranteeing that fill() must be called -// for the trailing EOF. After fill() reads the 9-byte EOF into -// cachedBuf[0:8], it overwrites the row value which sits at cachedBuf[5:13] -// (4-byte packet header + 1-byte length prefix = value starts at offset 5 -// within the 4096-byte cachedBuf). +// The row value returned by readRow points into the read buffer. skipRows may +// call fill(), which overwrites that memory. The test feeds each protocol +// packet as a separate Read call via chunkedConn (mimicking TLS record +// boundaries), guaranteeing that fill() is called for the trailing EOF. func TestGetSystemVarBufferReuse(t *testing.T) { // Protocol response for: SELECT @@max_allowed_packet → "67108864" // @@ -334,10 +316,7 @@ func TestGetSystemVarBufferReuse(t *testing.T) { } const expected = "67108864" - if got := string(val); got != expected { - t.Fatalf("getSystemVar(max_allowed_packet) = %q (% 02x), want %q\n"+ - "Value was likely corrupted by buffer.fill() overwriting the "+ - "slice returned by readRow when reading the trailing EOF packet.", - got, val, expected) + if val != expected { + t.Fatalf("getSystemVar(max_allowed_packet) = %q, want %q", val, expected) } } diff --git a/connector.go b/connector.go index db2bd7cf9..978fda415 100644 --- a/connector.go +++ b/connector.go @@ -182,7 +182,7 @@ func (c *connector) Connect(ctx context.Context) (driver.Conn, error) { mc.Close() return nil, err } - n, err := strconv.Atoi(string(maxap)) + n, err := strconv.Atoi(maxap) if err != nil { mc.Close() return nil, fmt.Errorf("invalid max_allowed_packet value (%q): %w", maxap, err) From 727a936665709e3fc795dd9cefc5185c66046ef4 Mon Sep 17 00:00:00 2001 From: Morgan Tocker Date: Mon, 20 Apr 2026 06:05:45 -0600 Subject: [PATCH 3/3] fix formatting --- connection_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/connection_test.go b/connection_test.go index b468aa6ea..d489c1e3e 100644 --- a/connection_test.go +++ b/connection_test.go @@ -229,7 +229,7 @@ func (c *chunkedConn) Read(b []byte) (int, error) { return n, nil } -func (c *chunkedConn) Write(b []byte) (int, error) { return len(b), nil } // swallow writes (e.g. COM_QUERY) +func (c *chunkedConn) Write(b []byte) (int, error) { return len(b), nil } // swallow writes (e.g. COM_QUERY) func (c *chunkedConn) Close() error { return nil } func (c *chunkedConn) LocalAddr() net.Addr { return nil } func (c *chunkedConn) RemoteAddr() net.Addr { return nil } @@ -272,15 +272,15 @@ func TestGetSystemVarBufferReuse(t *testing.T) { colDef := []byte{ 0x03, 'd', 'e', 'f', // catalog = "def" - 0x00, // schema = "" - 0x00, // table = "" - 0x00, // org_table = "" - 0x14, // name length = 20 + 0x00, // schema = "" + 0x00, // table = "" + 0x00, // org_table = "" + 0x14, // name length = 20 '@', '@', 'm', 'a', 'x', '_', 'a', 'l', 'l', 'o', 'w', 'e', 'd', '_', 'p', 'a', 'c', 'k', 'e', 't', - 0x00, // org_name = "" - 0x0c, // length of fixed fields - 0x3f, 0x00, // charset = 63 (binary) + 0x00, // org_name = "" + 0x0c, // length of fixed fields + 0x3f, 0x00, // charset = 63 (binary) 0x14, 0x00, 0x00, 0x00, // column_length = 20 0x0f, // type = FIELD_TYPE_VARCHAR 0x00, 0x00, // flags