fix(session): avoid double-free of stmt_meta pkt in LargePacket (#5639)#5808
Conversation
When handler_WCD_SS_MCQ_qpo_LargePacket() is reached via the COM_STMT_EXECUTE path, MySQL_Protocol::get_binds_from_pkt() has already aliased stmt_meta->pkt to pkt->ptr (see the FIXME at MySQL_Protocol.cpp:2873). The subsequent RequestEnd() -> Query_Info::end() free()s stmt_meta->pkt (the fix introduced for bug #796 at MySQL_Session.cpp:455), and then LargePacket's own l_free(pkt->size, pkt->ptr) frees the same buffer a second time. With jemalloc this silently corrupts the arena and surfaces later as the SIGSEGV reported in #5639, with the crashing frame landing on the l_free call site (+0x10e into LargePacket). Under ASAN the double-free aborts deterministically with a backtrace whose two free() chains match the report exactly. Fix: capture whether CurrentQuery.stmt_meta aliases pkt before calling RequestEnd() (which clears stmt_meta), and skip the second free when the alias was present. For the COM_QUERY path through LargePacket, stmt_meta is always NULL, so the previous behaviour is preserved there. Also add reg_test_5639_stmt_execute_max_allowed_packet-t which exercises the STMT_EXECUTE > mysql-max_allowed_packet path and asserts the session and process survive.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details🔇 Additional comments (1)
📝 WalkthroughWalkthroughAdds a guard in the large-packet error handler to avoid double-free when COM_STMT_EXECUTE reuses the packet buffer, and a regression test that forces an oversized STMT_EXECUTE to assert ER_NET_PACKET_TOO_LARGE while ensuring connections remain usable. ChangesCOM_STMT_EXECUTE Large Packet Overflow
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes a double-free vulnerability in MySQL_Session::handler_WCD_SS_MCQ_qpo_LargePacket by ensuring the packet buffer is not freed twice when aliased by stmt_meta. A new regression test is added to verify that oversized COM_STMT_EXECUTE packets are handled correctly without crashing. Feedback indicates that similar logic should be applied to other handlers like OK_msg and error_msg to prevent potential double-frees in those paths, and suggests using the MYSQL_QUERY() macro in the test suite for consistent error handling.
| const bool stmt_meta_owns_pkt = | ||
| (CurrentQuery.stmt_meta != NULL && CurrentQuery.stmt_meta->pkt == pkt->ptr); | ||
| RequestEnd(NULL, 1153, errmsg.c_str()); | ||
| l_free(pkt->size,pkt->ptr); | ||
| if (!stmt_meta_owns_pkt) { | ||
| l_free(pkt->size,pkt->ptr); | ||
| } |
There was a problem hiding this comment.
The fix correctly addresses the double-free in the LargePacket handler by detecting the alias before RequestEnd() frees it. However, the same double-free vulnerability appears to exist in handler_WCD_SS_MCQ_qpo_OK_msg() (lines 6332-6341) and handler_WCD_SS_MCQ_qpo_error_msg() (lines 6352-6358), which also call RequestEnd() followed by l_free() on the same packet. If a COM_STMT_EXECUTE is blocked or redirected to return an OK or error message via query rules, those handlers will also trigger a double-free of the aliased packet buffer. Consider applying this logic to those handlers as well to fully resolve the issue for all COM_STMT_EXECUTE paths.
| mysql_query(admin, buf); | ||
| mysql_query(admin, "LOAD MYSQL VARIABLES TO RUNTIME"); |
There was a problem hiding this comment.
For consistency with the rest of the test (e.g., lines 103 and 105), consider using the MYSQL_QUERY() macro here instead of calling mysql_query() directly. This ensures uniform error reporting if these queries fail during the restoration process.
| mysql_query(admin, buf); | |
| mysql_query(admin, "LOAD MYSQL VARIABLES TO RUNTIME"); | |
| MYSQL_QUERY(admin, buf); | |
| MYSQL_QUERY(admin, "LOAD MYSQL VARIABLES TO RUNTIME"); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/tap/tests/reg_test_5639_stmt_execute_max_allowed_packet-t.cpp (1)
27-29: ⚡ Quick winUse UPPER_SNAKE_CASE for file-scope constants.
These
constexprnames don’t follow the repository constant naming convention.♻️ Proposed rename
-constexpr unsigned int kMaxAllowedPacket = 8192; // minimum allowed by ProxySQL -constexpr size_t kBoundStringSize = 12 * 1024; // guarantees pkt > kMaxAllowedPacket -constexpr unsigned int kRestoreMaxAllowedPacket = 67108864; // ProxySQL default +constexpr unsigned int MAX_ALLOWED_PACKET = 8192; // minimum allowed by ProxySQL +constexpr size_t BOUND_STRING_SIZE = 12 * 1024; // guarantees pkt > MAX_ALLOWED_PACKET +constexpr unsigned int RESTORE_MAX_ALLOWED_PACKET = 67108864; // ProxySQL default @@ - snprintf(buf, sizeof(buf), "SET mysql-max_allowed_packet=%u", kRestoreMaxAllowedPacket); + snprintf(buf, sizeof(buf), "SET mysql-max_allowed_packet=%u", RESTORE_MAX_ALLOWED_PACKET); @@ - snprintf(buf, sizeof(buf), "SET mysql-max_allowed_packet=%u", kMaxAllowedPacket); + snprintf(buf, sizeof(buf), "SET mysql-max_allowed_packet=%u", MAX_ALLOWED_PACKET); @@ - ok(true, "Lowered mysql-max_allowed_packet to %u", kMaxAllowedPacket); + ok(true, "Lowered mysql-max_allowed_packet to %u", MAX_ALLOWED_PACKET); @@ - // packet exceeds kMaxAllowedPacket. The total wire packet for a single + // packet exceeds MAX_ALLOWED_PACKET. The total wire packet for a single @@ - // kBoundStringSize > kMaxAllowedPacket comfortably crosses the limit. --- - std::string big(kBoundStringSize, 'A'); + // BOUND_STRING_SIZE > MAX_ALLOWED_PACKET comfortably crosses the limit. --- + std::string big(BOUND_STRING_SIZE, 'A');As per coding guidelines
**/*.{cpp,h,hpp}: “Constants and macros should use UPPER_SNAKE_CASE”.Also applies to: 75-76, 102-103, 106-107, 136-139
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/tap/tests/reg_test_5639_stmt_execute_max_allowed_packet-t.cpp` around lines 27 - 29, Rename file-scope constexpr constants to UPPER_SNAKE_CASE: change kMaxAllowedPacket -> MAX_ALLOWED_PACKET, kBoundStringSize -> BOUND_STRING_SIZE, and kRestoreMaxAllowedPacket -> RESTORE_MAX_ALLOWED_PACKET, update all usages accordingly (including any test helpers or asserts that reference these symbols), and apply the same UPPER_SNAKE_CASE renaming to the other file-scope constexpr constants in this file so naming follows the repository guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/tap/tests/reg_test_5639_stmt_execute_max_allowed_packet-t.cpp`:
- Around line 27-29: Rename file-scope constexpr constants to UPPER_SNAKE_CASE:
change kMaxAllowedPacket -> MAX_ALLOWED_PACKET, kBoundStringSize ->
BOUND_STRING_SIZE, and kRestoreMaxAllowedPacket -> RESTORE_MAX_ALLOWED_PACKET,
update all usages accordingly (including any test helpers or asserts that
reference these symbols), and apply the same UPPER_SNAKE_CASE renaming to the
other file-scope constexpr constants in this file so naming follows the
repository guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9c030fc9-1810-4f9e-b721-8421ba64d99a
📒 Files selected for processing (3)
lib/MySQL_Session.cpptest/tap/groups/groups.jsontest/tap/tests/reg_test_5639_stmt_execute_max_allowed_packet-t.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: run / trigger
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names should use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables should use snake_case
Constants and macros should use UPPER_SNAKE_CASE
Use C++17 features; conditional compilation should use#ifdefPROXYSQL31,#ifdefPROXYSQL40,#ifdefPROXYSQLFFTO,#ifdefPROXYSQLTSDB,#ifdefPROXYSQLCLICKHOUSE (PROXYSQLGENAI no longer guards core code)
Use RAII for resource management and jemalloc for memory allocation
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
test/tap/tests/reg_test_5639_stmt_execute_max_allowed_packet-t.cpplib/MySQL_Session.cpp
test/tap/tests/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test files should follow naming pattern test_*.cpp or *-t.cpp in test/tap/tests/
Files:
test/tap/tests/reg_test_5639_stmt_execute_max_allowed_packet-t.cpp
🧠 Learnings (1)
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/reg_test_5639_stmt_execute_max_allowed_packet-t.cpp
🔇 Additional comments (3)
lib/MySQL_Session.cpp (1)
6376-6387: LGTM!test/tap/tests/reg_test_5639_stmt_execute_max_allowed_packet-t.cpp (1)
1-26: LGTM!Also applies to: 31-74, 77-195
test/tap/groups/groups.json (1)
277-277: LGTM!
CI-lint-groups-json requires keys in groups.json to be sorted.
|
1 participant
Summary
MySQL_Session::handler_WCD_SS_MCQ_qpo_LargePacket()when an oversized binary STMT_EXECUTE packet trips themysql-max_allowed_packetcheck.reg_test_5639_stmt_execute_max_allowed_packet-t) registered inmysql84-g6andmysql95-g1.Root cause
MySQL_Protocol::get_binds_from_pkt()aliasesstmt_meta->pkt = pkt.ptr(see the long-standing FIXME atlib/MySQL_Protocol.cpp:2873). InsideLargePacket,RequestEnd(NULL, …)callsQuery_Info::end(), whichfree()sstmt_meta->pkt(the fix introduced for bug #796 atlib/MySQL_Session.cpp:455). The handler then callsl_free(pkt->size, pkt->ptr)— which isfree(pkt->ptr)per the macro ininclude/proxysql_mem.h:51— on the same buffer. Double-free.The reporter's
+0x10ecrash offset in v3.0.7 lands on this exactl_freecall site. With jemalloc the double-free silently corrupts the arena and only crashes intermittently under stress (consistent with the KILL CONNECTION churn the reporter sees alongside the crashes). Under ASAN the abort is deterministic and the two free chains match the reporter's stack exactly.The fix
lib/MySQL_Session.cppinhandler_WCD_SS_MCQ_qpo_LargePacket:```cpp
const bool stmt_meta_owns_pkt =
(CurrentQuery.stmt_meta != NULL && CurrentQuery.stmt_meta->pkt == pkt->ptr);
RequestEnd(NULL, 1153, errmsg.c_str());
if (!stmt_meta_owns_pkt) {
l_free(pkt->size, pkt->ptr);
}
```
Captures the alias before `RequestEnd()` (which clears `stmt_meta`) and skips the second free when the alias was present. For the COM_QUERY path through LargePacket, `stmt_meta` is always NULL (it is only set inside the STMT_EXECUTE handler at `:3587`), so the previous behaviour is preserved there.
Test plan
Summary by CodeRabbit
Bug Fixes
Tests