Fix getSystemVar buffer reuse

[morgo]

Description

This fixes a bug in getSystemVar identified in performance testing.

AI disclosure: The testcase was authored by Claude.

Checklist

• Code compiles correctly
• Created tests which fail without the change (if possible)
• All tests passing
• Extended the README / documentation, if necessary (not applicable)
• Added myself / the copyright holder to the AUTHORS file

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 750668f5-9ac6-45c8-8632-574f4d0a325f

📥 Commits

Reviewing files that changed from the base of the PR and between 1b0d235 and 727a936.

📒 Files selected for processing (1)

• connection_test.go

✅ Files skipped from review due to trivial changes (1)

• connection_test.go

---

Walkthrough

getSystemVar now returns a string and copies the value before advancing internal buffers; tests add a chunked net.Conn test double to verify no buffer reuse corruption; AUTHORS gained two entries; connector parsing adjusted to use the new string return.

Changes

Cohort / File(s)	Summary
Metadata AUTHORS	Added Morgan Tocker <tocker at gmail.com> to Individual Persons and Block, Inc. to Organizations.
Buffer handling & API connection.go	Changed mysqlConn.getSystemVar signature from ([]byte, error) to (string, error) and copy the returned variable value before calling mc.skipRows() to avoid slice invalidation.
Deterministic tests connection_test.go	Added chunkedConn test double, makePacket helper, and TestGetSystemVarBufferReuse to feed one packet per Read and assert the system variable string is preserved despite buffer reuse.
Connector parsing tweak connector.go	Adjusted parsing of max_allowed_packet to pass the retrieved maxap string directly to strconv.Atoi (aligns with getSystemVar now returning string).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Fix getSystemVar buffer reuse' directly and specifically describes the main change: fixing a buffer reuse bug in the getSystemVar function.
Description check	✅ Passed	The description explains the bug fix, mentions performance testing context, discloses AI involvement, and includes a completed checklist verifying code quality and testing.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.11.4)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

connection.go (1)

455-457: Update the stale doc comment.

With the fix, the returned slice is now a freshly allocated copy and is valid beyond the next read. The existing comment ("The returned byte slice is only valid until the next read") contradicts the new behavior and will mislead future callers about the lifetime contract.

📝 Suggested comment update

 // Gets the value of the given MySQL System Variable
-// The returned byte slice is only valid until the next read
+// The returned byte slice is a copy and is safe to retain.
 func (mc *mysqlConn) getSystemVar(name string) ([]byte, error) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@connection.go` around lines 455 - 457, Update the stale doc comment above the
function getSystemVar to reflect the new lifetime contract: remove "The returned
byte slice is only valid until the next read" and state that the function
returns a freshly allocated copy of the system variable bytes which remains
valid after subsequent reads and until the caller releases it; keep the one-line
summary "Gets the value of the given MySQL System Variable" and add a brief note
about error behavior if desired.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@connection.go`:
- Around line 455-457: Update the stale doc comment above the function
getSystemVar to reflect the new lifetime contract: remove "The returned byte
slice is only valid until the next read" and state that the function returns a
freshly allocated copy of the system variable bytes which remains valid after
subsequent reads and until the caller releases it; keep the one-line summary
"Gets the value of the given MySQL System Variable" and add a brief note about
error behavior if desired.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: cb0ed115-b93b-4d60-ab0c-f6931385555d

📥 Commits

Reviewing files that changed from the base of the PR and between fed2c72 and a3eaef4.

📒 Files selected for processing (3)

• AUTHORS
• connection.go
• connection_test.go

[Copilot]

Pull request overview

This PR fixes a subtle data-corruption bug in mysqlConn.getSystemVar, where the returned []byte could be overwritten by subsequent packet reads due to reuse of the internal read buffer (notably reproducible with TLS-like chunking).

Changes:

• Clone the getSystemVar row value before consuming trailing packets to avoid buffer reuse corruption.
• Add a regression test that simulates packet-per-read behavior (TLS record boundary behavior) to reliably reproduce the issue.
• Update attribution in AUTHORS.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
connection.go	Fixes getSystemVar by copying the returned value before skipRows() can trigger buffer refills.
connection_test.go	Adds a deterministic regression test using a chunked net.Conn test double to simulate TLS-like reads.
AUTHORS	Adds an individual and organization entry per checklist.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coveralls]

coverage: 82.596% (+0.09%) from 82.508% — morgo:fix-getSystemVar-buffer-reuse into go-sql-driver:master