Harden ProxySQL PostgreSQL protocol handling against malformed packets - P2#5429
Merged
renecannao merged 10 commits intoMar 2, 2026
Conversation
The buffer2array() function could allocate less than 5 bytes for a packet when pkgsize was malformed to be smaller than the header size. This violated PostgreSQL protocol assumptions that packets are always >= 5 bytes. Ensure minimum allocation of sizeof(header) bytes regardless of the claimed packet size in the header.
Replace strlen() with memchr() to safely search for null terminators within buffer bounds. If no null terminator is found, treat the packet as malformed and break out of the processing loop.
…rminator checking * Add validation to reject packets smaller than 6 bytes before processing * Add null terminator check
Improve error handling in admin_session_handler when processing PostgreSQL query packets: * Fix potential underflow in query_length calculation when handling very small packets (< 2 bytes) * Fix NULL pointer dereference in strcasecmp when query_no_space is not initialized due to early packet validation failures
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChanged PgSQL connection/session control flow and parsing: PgSQL_Connection::flush gains an Changes
Sequence DiagramsequenceDiagram
participant Client
participant PgSQL_Session
participant PgSQL_Connection
participant libpq as libpq/PQ
participant NoticeHandler
rect rgba(200,150,255,0.5)
Note over Client,PgSQL_Connection: Resync flush flow (flush(is_resync=true))
end
Client->>PgSQL_Session: send query / trigger reset
PgSQL_Session->>PgSQL_Connection: resync_start()
PgSQL_Connection->>libpq: flush(is_resync=true)
alt flush succeeds
libpq-->>PgSQL_Connection: OK
PgSQL_Connection->>NoticeHandler: PQsetNoticeReceiver(unhandled_notice_cb)
PgSQL_Connection-->>PgSQL_Session: ASYNC_RESET_SESSION_END
else flush fails (is_resync=true)
libpq-->>PgSQL_Connection: Error
PgSQL_Connection->>PgSQL_Connection: set resync_failed=true
PgSQL_Connection-->>PgSQL_Session: continue/resync retry
end
PgSQL_Session->>PgSQL_Connection: resync_cont()
PgSQL_Connection->>libpq: flush(is_resync=true)
libpq-->>PgSQL_Connection: Ready / Error
PgSQL_Connection->>NoticeHandler: PQsetNoticeReceiver(notice_handler_cb)
PgSQL_Connection-->>PgSQL_Session: resync complete / resume
PgSQL_Session-->>Client: normal operation resumed
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances ProxySQL's robustness and stability when handling PostgreSQL protocol traffic by introducing comprehensive validation checks for various packet types. The changes address previous vulnerabilities where malformed client data could lead to undefined behavior or crashes, ensuring that ProxySQL gracefully handles invalid input and maintains operational integrity. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces significant hardening for ProxySQL's PostgreSQL protocol handling, addressing potential vulnerabilities from malformed packets. The changes are comprehensive, covering startup packet processing, simple and extended query protocols, data stream buffering, and the admin interface. Key improvements include replacing unbounded string searches with bounded memory searches, adding packet size validation, and ensuring the presence of null terminators. The addition of new regression and fuzzing-style tests is commendable and greatly increases confidence in the robustness of these changes. The code is well-written, and I've only identified a minor opportunity for refactoring to reduce code duplication. Overall, this is an excellent contribution that enhances the security and stability of the PostgreSQL protocol implementation.
Comment on lines
+5992
to
+5999
| // set query retries | ||
| mybe->server_myds->query_retries_on_failure = pgsql_thread___query_retries_on_failure; | ||
| // if a number of retries is set in mysql_query_rules, that takes priority | ||
| if (qpo) { | ||
| if (qpo->retries >= 0) { | ||
| mybe->server_myds->query_retries_on_failure = qpo->retries; | ||
| } | ||
| } |
There was a problem hiding this comment.
This block of code for setting query retries is duplicated in handle_post_sync_describe_message (line 6146) and handle_post_sync_execute_message (line 6433).
To improve maintainability and reduce redundancy, consider refactoring this logic into a private helper function within the PgSQL_Session class.
For example, you could add:
void PgSQL_Session::set_backend_query_retries(PgSQL_Backend* backend) {
if (!backend || !backend->server_myds) {
return;
}
backend->server_myds->query_retries_on_failure = pgsql_thread___query_retries_on_failure;
if (qpo && qpo->retries >= 0) {
backend->server_myds->query_retries_on_failure = qpo->retries;
}
}Then, you can replace the duplicated blocks in all three locations with a single call: set_backend_query_retries(mybe);
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
lib/Admin_Handler.cpp (1)
4734-4739: Consider decouplingquery_no_spacefree-size from mutablequery_length.These null checks are good. A small follow-up would make cleanup more robust: track
query_no_spaceallocation length separately, sincequery_lengthis mutated later for rewritten queries.Proposed refactor
char *query=NULL; unsigned int query_length = 0; char* query_no_space = NULL; + unsigned int query_no_space_length_alloc = 0; unsigned int query_no_space_length = 0; @@ query_no_space=(char *)l_alloc(query_length); + query_no_space_length_alloc = query_length; memcpy(query_no_space,query,query_length); @@ if (query_no_space) { - l_free(query_length, query_no_space); + l_free(query_no_space_length_alloc, query_no_space); } if (query) { l_free(query_length, query); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/Admin_Handler.cpp` around lines 4734 - 4739, The cleanup currently uses the mutable query_length when freeing both query_no_space and query, which can be wrong because query_length is modified later; introduce a separate size variable (e.g., query_no_space_len) when you allocate query_no_space and use that fixed length when calling l_free(query_no_space_len, query_no_space) in the Admin_Handler.cpp cleanup block, leaving the existing l_free(query_length, query) call unchanged; update allocations and any size computations to set query_no_space_len at creation and clear pointers/lengths after free to avoid double-free or size-mismatch issues.lib/PgSQL_Session.cpp (1)
5992-6000: Consider extracting repeated retry-policy assignment into a helper.The same retry-selection block appears in Parse/Describe/Execute post-sync paths; centralizing it would reduce drift risk and keep behavior aligned.
Also applies to: 6146-6154, 6433-6441
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/PgSQL_Session.cpp` around lines 5992 - 6000, Extract the repeated logic that sets mybe->server_myds->query_retries_on_failure into a small helper (e.g., set_query_retries_from_qpo) that takes the target server_myds pointer, the default pgsql_thread___query_retries_on_failure, and the qpo struct (with qpo->retries), then use that helper in the current block and the Parse/Describe/Execute post-sync paths to replace duplicated code; ensure the helper applies the default when qpo is null or qpo->retries < 0 and assign the resolved value back to mybe->server_myds->query_retries_on_failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/Admin_Handler.cpp`:
- Around line 2879-2885: The validation currently checks query_length and
hdr.data.ptr but not that the payload is null-terminated; update the same guard
in Admin_Handler.cpp to also verify that the last byte is '\0' before accepting
the packet. Concretely, after confirming query_length >= 2 and hdr.data.ptr !=
NULL, check that ((unsigned char*)hdr.data.ptr)[query_length - 1] == '\0'; if
not, call proxy_warning("Query not null-terminated: %u bytes\n", query_length),
call SPA->send_error_msg_to_client(sess, "Malformed query packet"), set
run_query = false and goto __run_query (same flow as the existing error branch).
In `@lib/PgSQL_Connection.cpp`:
- Line 1938: The notice_handler_cb callback can dereference a NULL query_result
during reset_session_start because PQsetNoticeReceiver is set before
query_result is initialized; update PgSQL_Connection::notice_handler_cb to first
check if this->query_result (or the member used) is non-null before accessing or
calling methods on it, and if it's null simply return / ignore the notice (or
log safely), ensuring notice_handler_cb safely handles the uninitialized state
when PQsetNoticeReceiver is installed; no other behavior changes required.
In `@lib/PgSQL_Session.cpp`:
- Around line 2076-2091: Move the malformed-Q packet validation to before any
query parsing or special-query handling: perform the pkt.size < 6 check, compute
query_len = pkt.size - 5 and set query_ptr = (char*)pkt.ptr + 5, and verify
query_ptr[query_len - 1] == '\0' before calling CurrentQuery.begin() and
handler_special_queries(); on failure call l_free(pkt.size, pkt.ptr), set
handler_ret = -1 and return handler_ret (same behavior as current checks) so
undersized or non-null-terminated packets never reach CurrentQuery.begin() or
handler_special_queries().
In `@test/tap/tests/pgsql-test_malformed_packet-t.cpp`:
- Around line 992-993: The ADMIN malformed-packet test is calling
run_malformed_packet_tests with cl.pgsql_host instead of the admin host, so
replace the host argument with the admin host (use cl.admin_host) while keeping
the admin port (cl.pgsql_admin_port) and the "ADMIN" role; update the other
identical calls at the nearby occurrences (the ones flagged around 1012-1013) so
all ADMIN runs target cl.admin_host rather than cl.pgsql_host.
- Around line 144-145: The variable handled_gracefully is tautological because
it accepts all recv() outcomes; change the check to assert meaningful outcomes:
when bytes_received <= 0 assert the connection was closed (handled as error),
and when bytes_received > 0 inspect the received buffer (e.g., check the first
byte or use the existing parser function) to ensure it is an ERROR/ERR response
rather than treating it as valid protocol data; update the assertions that use
handled_gracefully (including the similar block around the other occurrence at
lines 608-611) to validate the actual error-response semantics instead of using
a combined boolean that is always true.
- Around line 546-572: The test treats an authentication failure as a passing
condition; change the logic so that if authentication did not succeed (detected
via auth_type from read_message), the test fails immediately and does not
attempt the "post-authentication malformed packet" step. Specifically, in the
block using auth_type and auth_data (and the later similar block), replace the
branch that sends malformed_data when auth_type == 'E' with a failing ok(0, ...)
message indicating authentication did not succeed (use test_name.c_str()) and
return; only call send(sock, malformed_data.data(), malformed_data.size(), 0)
after confirming successful authentication from read_message. Ensure both
occurrences (around variables auth_type/auth_data, send, malformed_data) are
updated.
---
Nitpick comments:
In `@lib/Admin_Handler.cpp`:
- Around line 4734-4739: The cleanup currently uses the mutable query_length
when freeing both query_no_space and query, which can be wrong because
query_length is modified later; introduce a separate size variable (e.g.,
query_no_space_len) when you allocate query_no_space and use that fixed length
when calling l_free(query_no_space_len, query_no_space) in the Admin_Handler.cpp
cleanup block, leaving the existing l_free(query_length, query) call unchanged;
update allocations and any size computations to set query_no_space_len at
creation and clear pointers/lengths after free to avoid double-free or
size-mismatch issues.
In `@lib/PgSQL_Session.cpp`:
- Around line 5992-6000: Extract the repeated logic that sets
mybe->server_myds->query_retries_on_failure into a small helper (e.g.,
set_query_retries_from_qpo) that takes the target server_myds pointer, the
default pgsql_thread___query_retries_on_failure, and the qpo struct (with
qpo->retries), then use that helper in the current block and the
Parse/Describe/Execute post-sync paths to replace duplicated code; ensure the
helper applies the default when qpo is null or qpo->retries < 0 and assign the
resolved value back to mybe->server_myds->query_retries_on_failure.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
include/PgSQL_Connection.hinclude/PgSQL_Session.hlib/Admin_Handler.cpplib/PgSQL_Connection.cpplib/PgSQL_Data_Stream.cpplib/PgSQL_Protocol.cpplib/PgSQL_Session.cpptest/tap/groups/groups.jsontest/tap/tests/pgsql-reg_test_5415_copy_error_recovery-t.cpptest/tap/tests/pgsql-test_malformed_packet-t.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/PgSQL_Connection.cpp (1)
921-931:⚠️ Potential issue | 🟡 MinorRestore notice receiver on reset timeout and failure paths.
Line 922 restores
unhandled_notice_cbonly inASYNC_RESET_SESSION_END, but bothASYNC_RESET_SESSION_TIMEOUTandASYNC_RESET_SESSION_FAILED(lines 928-930) bypass that state. On timeout or failure, the connection remains active without restoring the notice receiver, leavingnotice_handler_cbinstalled if it was active before reset started.Proposed fix
case ASYNC_RESET_SESSION_FAILED: case ASYNC_RESET_SESSION_SUCCESSFUL: case ASYNC_RESET_SESSION_TIMEOUT: + PQsetNoticeReceiver(pgsql_conn, &PgSQL_Connection::unhandled_notice_cb, this); break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/PgSQL_Connection.cpp` around lines 921 - 931, The switch handling reset states fails to restore the notice receiver except in ASYNC_RESET_SESSION_END; ensure PQsetNoticeReceiver(pgsql_conn, &PgSQL_Connection::unhandled_notice_cb, this) is invoked on the failure and timeout paths as well (i.e., in the ASYNC_RESET_SESSION_FAILED and ASYNC_RESET_SESSION_TIMEOUT handling) so the connection doesn't retain the previous notice_handler_cb; locate the switch around ASYNC_RESET_SESSION_END / ASYNC_RESET_SESSION_FAILED / ASYNC_RESET_SESSION_TIMEOUT in PgSQL_Connection and add the same PQsetNoticeReceiver call before calling NEXT_IMMEDIATE or returning when is_error_present()/failure/timeout occur.lib/PgSQL_Session.cpp (1)
5695-5696:⚠️ Potential issue | 🟠 MajorUse bounded formatting for
std::string_viewin log output.Line 5695-5696 passes
command.data()to%swherecommandis astd::string_view. Sincestd::string_viewdata is not guaranteed to be null-terminated, this causes unbounded memory access. Use%.*swithcommand.size()instead (similar to the correct usage at line 5685 in the same function).🛠️ Proposed fix
- proxy_info("Received command '%s'%s. Switching to Fast Forward mode (Session Type:0x%02X)\n", - command.data(), client_info.c_str(), session_type); + proxy_info("Received command '%.*s'%s. Switching to Fast Forward mode (Session Type:0x%02X)\n", + static_cast<int>(command.size()), command.data(), client_info.c_str(), session_type);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/PgSQL_Session.cpp` around lines 5695 - 5696, The proxy_info call using proxy_info("Received command '%s'%s. Switching to Fast Forward mode (Session Type:0x%02X)\n", command.data(), client_info.c_str(), session_type) is unsafe because command is a std::string_view and may not be null-terminated; change the format to use a bounded string specifier (%.*s) and pass static_cast<int>(command.size()) before command.data(), keeping client_info.c_str() and session_type as-is so the logged command is safely length-limited (refer to proxy_info, command, client_info, session_type).
♻️ Duplicate comments (1)
test/tap/tests/pgsql-test_malformed_packet-t.cpp (1)
144-149:⚠️ Potential issue | 🟠 MajorGraceful-handling assertions still accept every
recv()outcome.At Line 144 and Line 613, the logic currently allows both error/timeout and success paths, so the assertion is effectively non-discriminating.
Proposed fix
- bool connection_closed = (bytes_received <= 0); - bool got_response = (bytes_received > 0); - bool handled_gracefully = connection_closed || got_response; + const bool recv_failed = (bytes_received < 0); + const bool connection_closed = (bytes_received == 0); + const bool got_response = (bytes_received > 0); + const bool handled_gracefully = !recv_failed && (connection_closed || got_response); @@ - bool connection_closed = (bytes_received <= 0); + const bool recv_failed = (bytes_received < 0); + const bool connection_closed = (bytes_received == 0); bool got_error_response = (bytes_received > 0 && buffer[0] == 'E'); bool got_other_response = (bytes_received > 0 && buffer[0] != 'E'); @@ - bool handled_gracefully = connection_closed || got_error_response || got_other_response; + const bool handled_gracefully = !recv_failed && + (connection_closed || got_error_response || got_other_response);Also applies to: 613-621
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/pgsql-test_malformed_packet-t.cpp` around lines 144 - 149, The current combined boolean handled_gracefully (using connection_closed, got_response) makes the test accept every recv() outcome; replace the single ok(handled_gracefully, ...) check with an explicit branch that asserts the exact expected outcome: if bytes_received <= 0 then call ok(connection_closed, "%s: Malformed packet caused connection to close (received: %ld bytes)", test_name.c_str(), (long)bytes_received); else call ok(got_response, "%s: Malformed packet produced a response (received: %ld bytes)", test_name.c_str(), (long)bytes_received); this uses the existing variables connection_closed, got_response, bytes_received and test_name and ensures the test distinguishes the two paths rather than accepting all outcomes.
🧹 Nitpick comments (1)
lib/PgSQL_Session.cpp (1)
5993-6000: Consider extracting duplicated retry-policy setup into one helper.The same
query_retries_on_failureblock is repeated in Parse/Describe/Execute post-sync paths. A small helper would reduce drift risk.♻️ Refactor sketch
+inline void PgSQL_Session::apply_query_retry_policy(PgSQL_Data_Stream* server_myds) { + server_myds->query_retries_on_failure = pgsql_thread___query_retries_on_failure; + if (qpo && qpo->retries >= 0) { + server_myds->query_retries_on_failure = qpo->retries; + } +}- // set query retries - mybe->server_myds->query_retries_on_failure = pgsql_thread___query_retries_on_failure; - // if a number of retries is set in mysql_query_rules, that takes priority - if (qpo) { - if (qpo->retries >= 0) { - mybe->server_myds->query_retries_on_failure = qpo->retries; - } - } + apply_query_retry_policy(mybe->server_myds);Also applies to: 6147-6154, 6435-6442
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/PgSQL_Session.cpp` around lines 5993 - 6000, Extract the repeated logic that sets mybe->server_myds->query_retries_on_failure into a small helper (e.g., set_query_retries_on_failure or apply_query_retry_policy) which takes the backend/session struct and the qpo (query options) pointer, defaults to pgsql_thread___query_retries_on_failure and overrides with qpo->retries when qpo is non-null and qpo->retries >= 0; then replace the three duplicated blocks in the Parse, Describe and Execute post-sync paths with calls to that helper using the existing symbols mybe, mybe->server_myds, pgsql_thread___query_retries_on_failure and qpo to avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/tap/tests/pgsql-test_malformed_packet-t.cpp`:
- Around line 482-490: The header reads use inconsistent recv() checks: the
1-byte type read checks "n <= 0" while the 4-byte length read checks "n != 4",
which can allow short reads to slip through; update both header reads to use the
same exact-read pattern used for the message body (the recv_exact-style loop) so
they loop until the expected number of bytes are received or an error/EOF
occurs. Specifically, replace the single-call recv(sock, type_buf, 1, 0) and
recv(sock, len_buf, 4, 0) with a small helper-style loop (or call an existing
recv_exact) that attempts to read exactly 1 and exactly 4 bytes respectively,
returning false on short read or error; make this change where type_buf and
len_buf are read so behavior matches the body-read loop.
---
Outside diff comments:
In `@lib/PgSQL_Connection.cpp`:
- Around line 921-931: The switch handling reset states fails to restore the
notice receiver except in ASYNC_RESET_SESSION_END; ensure
PQsetNoticeReceiver(pgsql_conn, &PgSQL_Connection::unhandled_notice_cb, this) is
invoked on the failure and timeout paths as well (i.e., in the
ASYNC_RESET_SESSION_FAILED and ASYNC_RESET_SESSION_TIMEOUT handling) so the
connection doesn't retain the previous notice_handler_cb; locate the switch
around ASYNC_RESET_SESSION_END / ASYNC_RESET_SESSION_FAILED /
ASYNC_RESET_SESSION_TIMEOUT in PgSQL_Connection and add the same
PQsetNoticeReceiver call before calling NEXT_IMMEDIATE or returning when
is_error_present()/failure/timeout occur.
In `@lib/PgSQL_Session.cpp`:
- Around line 5695-5696: The proxy_info call using proxy_info("Received command
'%s'%s. Switching to Fast Forward mode (Session Type:0x%02X)\n", command.data(),
client_info.c_str(), session_type) is unsafe because command is a
std::string_view and may not be null-terminated; change the format to use a
bounded string specifier (%.*s) and pass static_cast<int>(command.size()) before
command.data(), keeping client_info.c_str() and session_type as-is so the logged
command is safely length-limited (refer to proxy_info, command, client_info,
session_type).
---
Duplicate comments:
In `@test/tap/tests/pgsql-test_malformed_packet-t.cpp`:
- Around line 144-149: The current combined boolean handled_gracefully (using
connection_closed, got_response) makes the test accept every recv() outcome;
replace the single ok(handled_gracefully, ...) check with an explicit branch
that asserts the exact expected outcome: if bytes_received <= 0 then call
ok(connection_closed, "%s: Malformed packet caused connection to close
(received: %ld bytes)", test_name.c_str(), (long)bytes_received); else call
ok(got_response, "%s: Malformed packet produced a response (received: %ld
bytes)", test_name.c_str(), (long)bytes_received); this uses the existing
variables connection_closed, got_response, bytes_received and test_name and
ensures the test distinguishes the two paths rather than accepting all outcomes.
---
Nitpick comments:
In `@lib/PgSQL_Session.cpp`:
- Around line 5993-6000: Extract the repeated logic that sets
mybe->server_myds->query_retries_on_failure into a small helper (e.g.,
set_query_retries_on_failure or apply_query_retry_policy) which takes the
backend/session struct and the qpo (query options) pointer, defaults to
pgsql_thread___query_retries_on_failure and overrides with qpo->retries when qpo
is non-null and qpo->retries >= 0; then replace the three duplicated blocks in
the Parse, Describe and Execute post-sync paths with calls to that helper using
the existing symbols mybe, mybe->server_myds,
pgsql_thread___query_retries_on_failure and qpo to avoid drift.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/Admin_Handler.cpplib/PgSQL_Connection.cpplib/PgSQL_Session.cpptest/tap/tests/pgsql-test_malformed_packet-t.cpp
rahim-kanji
force-pushed
the
v3.0_strengthen-pgsql-protocol-validation
branch
from
March 1, 2026 21:19
a2eb295 to
8c9af56
Compare
…s beyond packet boundaries
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/PgSQL_Session.cpp (1)
5695-5697:⚠️ Potential issue | 🟠 MajorUse bounded formatting for
std::string_viewin logs.At line 5695,
command.data()is printed with%s, butcommandis astd::string_viewwhich is not guaranteed to be null-terminated. This can cause buffer over-read. The same function already demonstrates the correct approach (line 5683) using%.*swith the size.🛠️ Proposed fix
- proxy_info("Received command '%s'%s. Switching to Fast Forward mode (Session Type:0x%02X)\n", - command.data(), client_info.c_str(), session_type); + proxy_info("Received command '%.*s'%s. Switching to Fast Forward mode (Session Type:0x%02X)\n", + (int)command.size(), command.data(), client_info.c_str(), session_type);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/PgSQL_Session.cpp` around lines 5695 - 5697, The log call in proxy_info uses command.data() with "%s" but command is a std::string_view and may not be null-terminated; change the format to use bounded string formatting "%.*s" and pass (int)command.size() before command.data() so the output is safe (e.g., update the proxy_info invocation that currently formats command, client_info and session_type in the block that sets session_fast_forward). Ensure you keep client_info.c_str() and session_type as before and only replace the "%s" usage for the command argument.
♻️ Duplicate comments (1)
test/tap/tests/pgsql-test_malformed_packet-t.cpp (1)
144-149:⚠️ Potential issue | 🟠 Major
handled_gracefullyis still tautological in both phases.Line 146 and Line 627 combine predicates that cover every
recv()outcome, so these assertions cannot fail and don’t validate malformed-packet handling semantics.🔧 Proposed fix
- bool connection_closed = (bytes_received <= 0); - bool got_response = (bytes_received > 0); - bool handled_gracefully = connection_closed || got_response; + bool connection_closed = (bytes_received == 0); + bool timeout = (bytes_received < 0 && (errno == EAGAIN || errno == EWOULDBLOCK)); + bool got_error_response = (bytes_received > 0 && buffer[0] == 'E'); + bool handled_gracefully = connection_closed || timeout || got_error_response; @@ - bool connection_closed = (bytes_received <= 0); - bool got_error_response = (bytes_received > 0 && buffer[0] == 'E'); - bool got_other_response = (bytes_received > 0 && buffer[0] != 'E'); - bool handled_gracefully = connection_closed || got_error_response || got_other_response; + bool connection_closed = (bytes_received == 0); + bool timeout = (bytes_received < 0 && (errno == EAGAIN || errno == EWOULDBLOCK)); + bool got_protocol_response = (bytes_received > 0); + bool handled_gracefully = connection_closed || timeout || got_protocol_response;Also applies to: 621-631
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/pgsql-test_malformed_packet-t.cpp` around lines 144 - 149, The current assertion uses handled_gracefully = connection_closed || got_response which is tautological and can never fail; instead make the assertion check the phase-specific expected outcome: replace the combined predicate with an explicit check depending on the test phase (e.g., if this is the "expect-close" phase assert connection_closed (bytes_received <= 0), and if this is the "expect-response" phase assert got_response (bytes_received > 0)); update the ok(...) call to reflect the chosen expectation (use test_name.c_str() and bytes_received in the message) and remove the always-true handled_gracefully variable so the test actually validates malformed-packet handling.
🧹 Nitpick comments (3)
lib/PgSQL_Connection.cpp (1)
1216-1234: Minor: Log message may lack context in resync mode.When
is_resyncis true and flush fails,set_error_from_PQerrorMessage()is skipped, but theproxy_erroron line 2231 still callsget_error_code_with_message(). This may produce a less informative log since the error info wasn't populated.Consider logging
PQerrorMessage(pgsql_conn)directly in resync mode to capture the actual failure reason:🔧 Suggested improvement
else { if (!is_resync) { set_error_from_PQerrorMessage(); + proxy_error("Failed to flush data to backend. %s\n", get_error_code_with_message().c_str()); } else { resync_failed = true; + proxy_error("Failed to flush data to backend during resync. %s\n", PQerrorMessage(pgsql_conn)); } - proxy_error("Failed to flush data to backend. %s\n", get_error_code_with_message().c_str()); async_exit_status = PG_EVENT_NONE; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/PgSQL_Connection.cpp` around lines 1216 - 1234, The flush path in PgSQL_Connection::flush skips set_error_from_PQerrorMessage() when is_resync is true but still calls proxy_error with get_error_code_with_message(), which can be empty; update the failure branch so that when is_resync is true you capture the raw PQerrorMessage(pgsql_conn) (or include it in the proxy_error call) instead of relying solely on get_error_code_with_message(), and ensure resync_failed remains set; reference PgSQL_Connection::flush, the is_resync flag, set_error_from_PQerrorMessage(), proxy_error(), get_error_code_with_message(), PQerrorMessage(), and resync_failed to locate and implement this change.lib/PgSQL_Session.cpp (2)
5681-5687: Add a defensive null guard before readingPSarrayIN->len.At Line 5683,
client_myds->PSarrayINis dereferenced directly. Given this PR’s hardening goals, a guard here avoids a crash if queue initialization is unexpectedly missing on an edge path.🛡️ Proposed hardening
- if (client_myds->PSarrayIN->len) { + if (client_myds && client_myds->PSarrayIN && client_myds->PSarrayIN->len) { proxy_error("Cannot switch to fast forward mode: unexpected pending packets in client_myds->PSarrayIN (len=%u). Command: %.*s\n", client_myds->PSarrayIN->len, (int)command.size(), command.data()); return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/PgSQL_Session.cpp` around lines 5681 - 5687, Add a defensive null check for client_myds->PSarrayIN before dereferencing its len in the fast-forward switch path: verify that client_myds and client_myds->PSarrayIN are non-null (the check around the existing proxy_error/return false block inside the same function in PgSQL_Session.cpp), and if PSarrayIN is null, log a clear proxy_error indicating missing/NULL PSarrayIN and return false; otherwise keep the existing len check and behavior.
5993-6000: Retry policy assignment is duplicated in three post-sync paths.The same
query_retries_on_failureinitialization logic is repeated in parse/describe/execute handlers. Extracting to one helper will reduce drift risk and keep retry behavior consistent.Also applies to: 6147-6154, 6434-6441
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/PgSQL_Session.cpp` around lines 5993 - 6000, The repeated initialization of mybe->server_myds->query_retries_on_failure from pgsql_thread___query_retries_on_failure with an override from qpo->retries (mysql_query_rules) is duplicated in the parse/describe/execute post-sync paths; refactor by creating a single helper (e.g., a function like set_query_retries_on_failure(SessionBackend* mybe, QueryParserOptions* qpo) or similar) that encapsulates: set to pgsql_thread___query_retries_on_failure, then if qpo && qpo->retries >= 0 set to qpo->retries, and call that helper from the three locations (currently containing the duplicated blocks around mybe->server_myds->query_retries_on_failure) to ensure consistent behavior and reduce duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/PgSQL_Session.cpp`:
- Around line 5695-5697: The log call in proxy_info uses command.data() with
"%s" but command is a std::string_view and may not be null-terminated; change
the format to use bounded string formatting "%.*s" and pass (int)command.size()
before command.data() so the output is safe (e.g., update the proxy_info
invocation that currently formats command, client_info and session_type in the
block that sets session_fast_forward). Ensure you keep client_info.c_str() and
session_type as before and only replace the "%s" usage for the command argument.
---
Duplicate comments:
In `@test/tap/tests/pgsql-test_malformed_packet-t.cpp`:
- Around line 144-149: The current assertion uses handled_gracefully =
connection_closed || got_response which is tautological and can never fail;
instead make the assertion check the phase-specific expected outcome: replace
the combined predicate with an explicit check depending on the test phase (e.g.,
if this is the "expect-close" phase assert connection_closed (bytes_received <=
0), and if this is the "expect-response" phase assert got_response
(bytes_received > 0)); update the ok(...) call to reflect the chosen expectation
(use test_name.c_str() and bytes_received in the message) and remove the
always-true handled_gracefully variable so the test actually validates
malformed-packet handling.
---
Nitpick comments:
In `@lib/PgSQL_Connection.cpp`:
- Around line 1216-1234: The flush path in PgSQL_Connection::flush skips
set_error_from_PQerrorMessage() when is_resync is true but still calls
proxy_error with get_error_code_with_message(), which can be empty; update the
failure branch so that when is_resync is true you capture the raw
PQerrorMessage(pgsql_conn) (or include it in the proxy_error call) instead of
relying solely on get_error_code_with_message(), and ensure resync_failed
remains set; reference PgSQL_Connection::flush, the is_resync flag,
set_error_from_PQerrorMessage(), proxy_error(), get_error_code_with_message(),
PQerrorMessage(), and resync_failed to locate and implement this change.
In `@lib/PgSQL_Session.cpp`:
- Around line 5681-5687: Add a defensive null check for client_myds->PSarrayIN
before dereferencing its len in the fast-forward switch path: verify that
client_myds and client_myds->PSarrayIN are non-null (the check around the
existing proxy_error/return false block inside the same function in
PgSQL_Session.cpp), and if PSarrayIN is null, log a clear proxy_error indicating
missing/NULL PSarrayIN and return false; otherwise keep the existing len check
and behavior.
- Around line 5993-6000: The repeated initialization of
mybe->server_myds->query_retries_on_failure from
pgsql_thread___query_retries_on_failure with an override from qpo->retries
(mysql_query_rules) is duplicated in the parse/describe/execute post-sync paths;
refactor by creating a single helper (e.g., a function like
set_query_retries_on_failure(SessionBackend* mybe, QueryParserOptions* qpo) or
similar) that encapsulates: set to pgsql_thread___query_retries_on_failure, then
if qpo && qpo->retries >= 0 set to qpo->retries, and call that helper from the
three locations (currently containing the duplicated blocks around
mybe->server_myds->query_retries_on_failure) to ensure consistent behavior and
reduce duplication.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/Admin_Handler.cpplib/PgSQL_Connection.cpplib/PgSQL_Session.cpptest/tap/tests/pgsql-test_malformed_packet-t.cpp
Comment on lines
+131
to
+132
| ssize_t bytes_sent = send(sock, data.data(), data.size(), 0); | ||
| if (bytes_sent < 0) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, let's check if the file exists and get its total line count
file="test/tap/tests/pgsql-test_malformed_packet-t.cpp"
if [ -f "$file" ]; then
echo "=== File info ==="
wc -l "$file"
echo
fi
# Read the specific lines mentioned in the review
echo "=== Lines 131-132 ==="
sed -n '128,135p' "$file"
echo
echo "=== Lines 540-541 ==="
sed -n '537,544p' "$file"
echo
echo "=== Lines 557-558 ==="
sed -n '554,561p' "$file"
echo
echo "=== Lines 603-604 ==="
sed -n '600,607p' "$file"Repository: sysown/proxysql
Length of output: 1382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="test/tap/tests/pgsql-test_malformed_packet-t.cpp"
echo "=== Search for send_exact or similar helpers ==="
rg -n --type=cpp 'send_exact|while.*send|short.write' "$file" || echo "No matches found"
echo
echo "=== All send() calls in the file ==="
rg -n --type=cpp '\bsend\s*\(' "$file"
echo
echo "=== Check file for any short-write checks ==="
rg -n --type=cpp 'sent.*size|bytes_sent.*size|!=' "$file" | head -20Repository: sysown/proxysql
Length of output: 960
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="test/tap/tests/pgsql-test_malformed_packet-t.cpp"
echo "=== Context around lines 131-132 and 132-140 (after send) ==="
sed -n '130,145p' "$file"
echo
echo "=== Context around lines 603-604 and after (showing what happens with 'sent' var) ==="
sed -n '602,620p' "$file"
echo
echo "=== Check if there are any retry mechanisms or loops for send ==="
rg -n --type=cpp -B2 -A5 'while.*\(|for.*\(' "$file" | head -30Repository: sysown/proxysql
Length of output: 2936
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="test/tap/tests/pgsql-test_malformed_packet-t.cpp"
echo "=== Check packet building to understand payload expectations ==="
rg -n --type=cpp 'build_.*packet|malformed_data|startup|password_pkt' "$file" | head -30Repository: sysown/proxysql
Length of output: 1650
Add short-write handling to all send() calls to prevent truncated test payloads.
Socket writes only check for errors (< 0) but ignore partial writes. If send() returns fewer bytes than requested, the truncated packet proceeds to the server, compromising the test's malformed packet coverage and causing flaky behavior depending on socket buffer conditions.
Affected locations: 131-132, 540-541, 557-558, 603-604.
Implement a helper like send_exact() that loops until all bytes are sent or an error occurs:
Proposed fix
+bool send_exact(int sock, const void* buf, size_t len) {
+ const uint8_t* ptr = static_cast<const uint8_t*>(buf);
+ size_t sent = 0;
+ while (sent < len) {
+ ssize_t n = send(sock, ptr + sent, len - sent, 0);
+ if (n <= 0) return false;
+ sent += static_cast<size_t>(n);
+ }
+ return true;
+}
@@
- ssize_t bytes_sent = send(sock, data.data(), data.size(), 0);
- if (bytes_sent < 0) {
+ if (!send_exact(sock, data.data(), data.size())) {
@@
- if (send(sock, startup.data(), startup.size(), 0) < 0) {
+ if (!send_exact(sock, startup.data(), startup.size())) {
@@
- if (send(sock, password_pkt.data(), password_pkt.size(), 0) < 0) {
+ if (!send_exact(sock, password_pkt.data(), password_pkt.size())) {
@@
- ssize_t sent = send(sock, malformed_data.data(), malformed_data.size(), 0);
- if (sent < 0) {
+ if (!send_exact(sock, malformed_data.data(), malformed_data.size())) {
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/PgSQL_Extended_Query_Message.cpp (1)
178-180: Clarify the statement-type comment to match wire format.The guard is correct, but Line 178 says “null-terminated string” for statement type. This field is a single byte (
'S'/'P'), so the comment is misleading.Suggested comment-only diff
- // Validate remaining length for statement type (at least 1 byte for null-terminated string) + // Validate remaining length for statement type (exactly 1 byte)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/PgSQL_Extended_Query_Message.cpp` around lines 178 - 180, The comment about the "statement type" is incorrect: that field is a single byte ('S' or 'P'), not a null-terminated string. Update the comment near the guard that checks "if (offset >= pkt_len)" to state that the code is validating at least one byte remains for the single-byte statement type (use variable names offset and pkt_len), and mention valid values ('S'/'P') to clarify the wire format.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/PgSQL_Extended_Query_Message.cpp`:
- Around line 178-180: The comment about the "statement type" is incorrect: that
field is a single byte ('S' or 'P'), not a null-terminated string. Update the
comment near the guard that checks "if (offset >= pkt_len)" to state that the
code is validating at least one byte remains for the single-byte statement type
(use variable names offset and pkt_len), and mention valid values ('S'/'P') to
clarify the wire format.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
test/tap/tests/pgsql-test_malformed_packet-t.cpp (2)
131-132:⚠️ Potential issue | 🟠 MajorHandle partial
send()writes; current checks can silently transmit truncated packets.All four call sites only check
< 0. A short positive write is currently treated as success, which can invalidate malformed-packet assertions.Proposed fix
+bool send_exact(int sock, const void* buf, size_t len) { + const uint8_t* ptr = static_cast<const uint8_t*>(buf); + size_t sent = 0; + while (sent < len) { + ssize_t n = send(sock, ptr + sent, len - sent, 0); + if (n <= 0) return false; + sent += static_cast<size_t>(n); + } + return true; +} @@ - ssize_t bytes_sent = send(sock, data.data(), data.size(), 0); - if (bytes_sent < 0) { + if (!send_exact(sock, data.data(), data.size())) { @@ - if (send(sock, startup.data(), startup.size(), 0) < 0) { + if (!send_exact(sock, startup.data(), startup.size())) { @@ - if (send(sock, password_pkt.data(), password_pkt.size(), 0) < 0) { + if (!send_exact(sock, password_pkt.data(), password_pkt.size())) { @@ - ssize_t sent = send(sock, malformed_data.data(), malformed_data.size(), 0); - if (sent < 0) { + if (!send_exact(sock, malformed_data.data(), malformed_data.size())) {Also applies to: 543-544, 560-561, 606-607
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/pgsql-test_malformed_packet-t.cpp` around lines 131 - 132, The send() calls (using variables sock, data.data(), data.size(), and bytes_sent) only check for bytes_sent < 0 and therefore treat short positive writes as full success, which can produce truncated packets; update each call site to loop until all bytes are written (track an offset/remaining bytes and call send repeatedly) or treat any bytes_sent != (ssize_t)remaining as an error, ensuring you adjust the buffer pointer (data.data() + offset) and remaining length accordingly; apply this fix to all four sites referenced (the sends at the current location and the ones around the other mentioned line ranges).
603-631:⚠️ Potential issue | 🟠 MajorPhase 2 assertion can pass before malformed packet impact is observed.
After
AuthenticationOk, the server may still emit startup completion messages (S/K/Z). Current logic sends malformed data immediately and accepts anybytes_received > 0, so unrelated protocol traffic can produce false positives.Proposed fix
// Authentication successful - now we have an authenticated connection + // Drain until ReadyForQuery to ensure subsequent response corresponds to malformed packet + bool ready_for_query = false; + for (int i = 0; i < 16; ++i) { + char msg_type = 0; + std::vector<uint8_t> msg_data; + if (!read_message(sock, msg_type, msg_data)) break; + if (msg_type == 'Z') { + ready_for_query = true; + break; + } + if (msg_type == 'E') break; + } + if (!ready_for_query) { + close(sock); + ok(0, "%s: Did not reach ReadyForQuery before malformed packet", test_name.c_str()); + return; + } // Step 5: Send the malformed packet on the authenticated connection @@ - bool got_protocol_response = (bytes_received > 0); - bool handled_gracefully = connection_closed || timeout || got_protocol_response; + bool got_error_response = (bytes_received > 0 && buffer[0] == 'E'); + bool handled_gracefully = connection_closed || timeout || got_error_response; @@ - ok(handled_gracefully, "%s: Phase 2 malformed packet sent (closed=%d, timeout=%d, response=%d, first=0x%02X)", - test_name.c_str(), (int)connection_closed, (int)timeout, (int)got_protocol_response, + ok(handled_gracefully, "%s: Phase 2 malformed packet handled (closed=%d, timeout=%d, error=%d, first=0x%02X)", + test_name.c_str(), (int)connection_closed, (int)timeout, (int)got_error_response, bytes_received > 0 ? (unsigned char)buffer[0] : 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/pgsql-test_malformed_packet-t.cpp` around lines 603 - 631, The current Phase 2 check (using recv once and treating any bytes_received>0 as success) can be a false positive because post-authentication protocol noise (ParameterStatus 'S', BackendKeyData 'K', ReadyForQuery 'Z') may arrive before the malformed packet is processed; update the logic around the recv loop that sends malformed_data and reads responses (variables/functions: malformed_data, send, recv, buffer, bytes_received, connection_closed, timeout, got_protocol_response, handled_gracefully) to loop reading until either (a) the connection is closed, (b) a timeout elapses, or (c) a definite error/errant message indicating the malformed packet was handled (inspect the first byte of each message in buffer[0] and ignore ordinary startup/post-auth messages like 'S','K','Z' while continuing to wait), and only set handled_gracefully true when you observe connection close, timeout, or an actual error response for the malformed packet rather than any incidental protocol message; ensure the loop has a bounded overall timeout to avoid hanging.
🧹 Nitpick comments (1)
lib/PgSQL_Session.cpp (1)
5993-6000: Extract repeated query-retry policy setup into one helper.The same retry-selection block appears in three post-sync handlers. Centralizing it will reduce drift risk.
♻️ Suggested refactor
+inline void PgSQL_Session::apply_query_retry_policy(PgSQL_Data_Stream* myds) { + myds->query_retries_on_failure = pgsql_thread___query_retries_on_failure; + if (qpo && qpo->retries >= 0) { + myds->query_retries_on_failure = qpo->retries; + } +}- // set query retries - mybe->server_myds->query_retries_on_failure = pgsql_thread___query_retries_on_failure; - // if a number of retries is set in mysql_query_rules, that takes priority - if (qpo) { - if (qpo->retries >= 0) { - mybe->server_myds->query_retries_on_failure = qpo->retries; - } - } + apply_query_retry_policy(mybe->server_myds);Also applies to: 6147-6154, 6434-6441
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/PgSQL_Session.cpp` around lines 5993 - 6000, Extract the repeated retry-selection block into a single helper (e.g., a static function apply_query_retries_policy(MyBackend* mybe, QueryOptions* qpo) or similar) that sets mybe->server_myds->query_retries_on_failure to pgsql_thread___query_retries_on_failure by default and overrides it when qpo && qpo->retries >= 0; then replace the duplicate blocks found in the three post-sync handlers with a call to this helper (referencing mybe, qpo and the field server_myds->query_retries_on_failure and the constant pgsql_thread___query_retries_on_failure).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/tap/tests/pgsql-test_malformed_packet-t.cpp`:
- Around line 131-132: The send() calls (using variables sock, data.data(),
data.size(), and bytes_sent) only check for bytes_sent < 0 and therefore treat
short positive writes as full success, which can produce truncated packets;
update each call site to loop until all bytes are written (track an
offset/remaining bytes and call send repeatedly) or treat any bytes_sent !=
(ssize_t)remaining as an error, ensuring you adjust the buffer pointer
(data.data() + offset) and remaining length accordingly; apply this fix to all
four sites referenced (the sends at the current location and the ones around the
other mentioned line ranges).
- Around line 603-631: The current Phase 2 check (using recv once and treating
any bytes_received>0 as success) can be a false positive because
post-authentication protocol noise (ParameterStatus 'S', BackendKeyData 'K',
ReadyForQuery 'Z') may arrive before the malformed packet is processed; update
the logic around the recv loop that sends malformed_data and reads responses
(variables/functions: malformed_data, send, recv, buffer, bytes_received,
connection_closed, timeout, got_protocol_response, handled_gracefully) to loop
reading until either (a) the connection is closed, (b) a timeout elapses, or (c)
a definite error/errant message indicating the malformed packet was handled
(inspect the first byte of each message in buffer[0] and ignore ordinary
startup/post-auth messages like 'S','K','Z' while continuing to wait), and only
set handled_gracefully true when you observe connection close, timeout, or an
actual error response for the malformed packet rather than any incidental
protocol message; ensure the loop has a bounded overall timeout to avoid
hanging.
---
Nitpick comments:
In `@lib/PgSQL_Session.cpp`:
- Around line 5993-6000: Extract the repeated retry-selection block into a
single helper (e.g., a static function apply_query_retries_policy(MyBackend*
mybe, QueryOptions* qpo) or similar) that sets
mybe->server_myds->query_retries_on_failure to
pgsql_thread___query_retries_on_failure by default and overrides it when qpo &&
qpo->retries >= 0; then replace the duplicate blocks found in the three
post-sync handlers with a call to this helper (referencing mybe, qpo and the
field server_myds->query_retries_on_failure and the constant
pgsql_thread___query_retries_on_failure).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/PgSQL_Extended_Query_Message.cpplib/PgSQL_Session.cpptest/tap/tests/pgsql-test_malformed_packet-t.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/PgSQL_Extended_Query_Message.cpp
…cing * Fix test assertions to properly validate malformed packet handling
rahim-kanji
force-pushed
the
v3.0_strengthen-pgsql-protocol-validation
branch
from
March 2, 2026 08:24
5bf5680 to
d0ad4b6
Compare
|
renecannao
changed the base branch from
v3.0
to
v3.0_pgsql-copy-error-recovery-issue-5415
March 2, 2026 09:38
renecannao
merged commit Mar 2, 2026
d305cde
into
v3.0_pgsql-copy-error-recovery-issue-5415
12 of 15 checks passed
This was referenced Mar 2, 2026
11 tasks
4 tasks
This was referenced May 5, 2026
Closed
This was referenced Jul 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Improve ProxySQL's resilience when handling malformed PostgreSQL protocol packets by adding comprehensive validation throughout the packet processing pipeline.
Problem
The PostgreSQL protocol implementation had several areas where insufficient validation of packet structure, length fields, and string terminators could lead to undefined behavior when receiving malformed data from clients.
Solution
Added defensive checks at critical points in the packet processing flow:
Startup Packet Processing - Replaced unbounded string searches with bounded memory searches to safely handle missing null terminators in connection parameters.
Simple Query Handling - Added minimum size validation and null terminator verification for 'Q' (Query) packets before processing.
Extended Query Protocol - Added bounds checking for Describe and Close messages to prevent reads beyond packet boundaries.
Data Stream Buffering - Enforced minimum allocation sizes to handle undersized length fields.
Admin Interface - Added comprehensive validation for query length, buffer pointers, and proper cleanup handling.
Summary by CodeRabbit
Bug Fixes
Public API
Tests