Harden SonarCloud S5801/S5813 string handling hotspots - #6026
Conversation
Compute command and server version lengths once before comparisons/copy paths: this reduces repeated strlen calls in Proxysql admin command dispatch and initial handshake payload construction, keeping the same behavior while matching cpp:S5813 guidance.
Use compile-time lengths for admin SQL command constants and reuse existing query_no_space_length in checksum dispatch checks to avoid repeated strlen calls across command matching paths.
Introduce a single cached tbh_len in the quoted table-name branch to avoid repeated strlen(tbh) calls when testing and trimming delimiters.
Precompute query template and escaped field lengths before malloc for several config import paths to avoid repeated/unsafe strlen checks in S5813-sensitive hotspots. No semantic change.
Use std::string_view for username/schemaname/hostname digest inputs to avoid raw C string length calls in query digest hashing path while preserving behavior.
Use std::string_view sizing for username/schemaname/hostname inputs in query-digest hashing to remove direct strlen usage in this path and keep semantics intact.
Construct each SHOW FIELDS query by replacing its fixed %s placeholder in a std::string. This removes the manual allocation and nonliteral snprintf path while preserving the existing table-name variants.
Replace temporary malloc buffers and nonliteral snprintf calls with explicit string construction for version, user, and SHOW CREATE TABLE queries. Preserve allocation-failure handling while removing the new Sonar buffer and format-string findings.
Replace function-local delimiter macros with constexpr character arrays and derive their lengths from sizeof. Keep the hash input bytes unchanged while removing macro pollution and the associated Sonar maintainability findings.
Move per-column binary encoding into a dedicated helper so the COPY test no longer nests type handling inside the row and field loops. Preserve the explicit oversized-numeric failure while reducing the Sonar cognitive-complexity and nesting findings.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/tap/tests/pgsql-copy_from_test-t.cpp (1)
512-514: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winFinalize COPY before returning from binary encoding failure.
testSTDIN_TEXT_BINARYcallsPQputCopyDatawithout ending COPY whenencodeBinaryFieldfails, then returns viasuccess == false. That leavesconnin COPY_IN state and can cause later operations such asexecuteTeststo fail with COPY protocol errors. CallPQputCopyEnd(conn, NULL)/ consume COPY results before returning unless the connection is closed.🤖 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/pgsql-copy_from_test-t.cpp` around lines 512 - 514, Update the failure return in testSTDIN_TEXT_BINARY so that when success is false, it finalizes the active COPY operation with PQputCopyEnd(conn, NULL) and consumes the COPY results before returning, unless the connection has already been closed. Preserve the existing direct return behavior for closed connections.
🤖 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.
Inline comments:
In `@lib/network.cpp`:
- Around line 97-101: Update the overlong-path error logging in the Unix socket
setup to use a bounded string format limited to path_len bytes instead of %s.
Preserve the existing ENAMETOOLONG assignment and early return in this
validation branch.
In `@test/tap/tests/pgsql-copy_from_test-t.cpp`:
- Around line 351-372: Update the INT, BOOLEAN, and shared field-encoding logic
in the row writer to match PostgreSQL binary COPY: encode INT values as 4-byte
big-endian payloads rather than memcpy-ing host-endian integers, and handle
BOOLEAN separately by accepting only true/false values and writing a single
binary byte. Keep length prefixes and offsets consistent with each field’s
encoded payload, and reject invalid boolean text.
---
Outside diff comments:
In `@test/tap/tests/pgsql-copy_from_test-t.cpp`:
- Around line 512-514: Update the failure return in testSTDIN_TEXT_BINARY so
that when success is false, it finalizes the active COPY operation with
PQputCopyEnd(conn, NULL) and consumes the COPY results before returning, unless
the connection has already been closed. Preserve the existing direct return
behavior for closed connections.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 882bf08b-a190-4ac4-94aa-fce6ad1fb7b5
📒 Files selected for processing (13)
include/Admin_ifaces.hlib/ClickHouse_Server.cpplib/GTID_Server_Data.cpplib/PgSQL_Connection.cpplib/network.cppplugins/genai/include/MCP_Thread.hplugins/genai/src/MCP_Thread.cppplugins/genai/src/tool_handlers/Config_Tool_Handler.cpptest/tap/tests/admin_show_fields_from-t.cpptest/tap/tests/admin_show_table_status-t.cpptest/tap/tests/aurora.cpptest/tap/tests/pgsql-copy_from_test-t.cpptest/tap/tests/unit/genai_mcp_thread_unit-t.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- lib/PgSQL_Connection.cpp
- lib/GTID_Server_Data.cpp
- plugins/genai/include/MCP_Thread.h
- lib/ClickHouse_Server.cpp
- include/Admin_ifaces.h
- test/tap/tests/aurora.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: CI-builds / builds (ubuntu22,-tap-mysqlx)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: build
- GitHub Check: run / trigger
- GitHub Check: Gitar
🧰 Additional context used
📓 Path-based instructions (3)
test/tap/tests/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
test/tap/tests/**/*.cpp: Test files intest/tap/tests/must follow the naming patterntest_*.cppor*-t.cpp.
To add a new TAP test, add the<testname>-t.cppfile and register it intest/tap/tests/Makefile/groups.json; no special Makefile target is needed becausemake <testname>-tis generated by pattern rule.
Files:
test/tap/tests/admin_show_fields_from-t.cpptest/tap/tests/unit/genai_mcp_thread_unit-t.cpptest/tap/tests/admin_show_table_status-t.cpptest/tap/tests/pgsql-copy_from_test-t.cpp
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must usePascalCasewith protocol prefixes such asMySQL_,PgSQL_, andProxySQL_.
Member variables must usesnake_case.
Constants and macros must useUPPER_SNAKE_CASE.
Use C++17, and gate conditional code with#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB, and#ifdef PROXYSQLCLICKHOUSE;PROXYSQLGENAImust not guard core code outsideplugins/genai/.
Consider performance implications when changing hot paths or other performance-critical code.
Use RAII for resource management and jemalloc for allocation.
Use pthread mutexes for synchronization andstd::atomic<>for counters.
Files:
test/tap/tests/admin_show_fields_from-t.cpplib/network.cppplugins/genai/src/MCP_Thread.cpptest/tap/tests/unit/genai_mcp_thread_unit-t.cpptest/tap/tests/admin_show_table_status-t.cppplugins/genai/src/tool_handlers/Config_Tool_Handler.cpptest/tap/tests/pgsql-copy_from_test-t.cpp
test/tap/tests/unit/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in
test/tap/tests/unit/must usetest_globals.handtest_init.hwith the custom unit-test harness.
Files:
test/tap/tests/unit/genai_mcp_thread_unit-t.cpp
🧠 Learnings (2)
📚 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/admin_show_fields_from-t.cpptest/tap/tests/unit/genai_mcp_thread_unit-t.cpptest/tap/tests/admin_show_table_status-t.cpptest/tap/tests/pgsql-copy_from_test-t.cpp
📚 Learning: 2026-04-01T21:27:00.297Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:00.297Z
Learning: In ProxySQL unit tests under test/tap/tests/unit/, include test_globals.h and test_init.h only for tests that depend on ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). For “pure” data-structure/utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) that do not require runtime globals/initialization, it is correct to omit test_globals.h and test_init.h and instead include only tap.h plus the relevant project header(s).
Applied to files:
test/tap/tests/unit/genai_mcp_thread_unit-t.cpp
🔇 Additional comments (10)
lib/network.cpp (1)
120-120: LGTM!plugins/genai/src/MCP_Thread.cpp (1)
176-185: LGTM!plugins/genai/src/tool_handlers/Config_Tool_Handler.cpp (1)
423-424: LGTM!Also applies to: 550-551
test/tap/tests/admin_show_fields_from-t.cpp (1)
78-87: LGTM!test/tap/tests/unit/genai_mcp_thread_unit-t.cpp (4)
38-40: LGTM!
365-368: LGTM!
380-396: LGTM!
471-482: LGTM!Also applies to: 499-499
test/tap/tests/admin_show_table_status-t.cpp (1)
78-87: LGTM!test/tap/tests/pgsql-copy_from_test-t.cpp (1)
490-497: LGTM!
Release tokenizer memory only when the input was duplicated on the heap. Short inputs point into tokenizer_t::buffer, so checking the pointer identity prevents free() from being called on that stack-backed storage while preserving cleanup for long inputs.
Use the existing computed query capacities for the config loader's dynamic SQL formatting paths. Bounded snprintf calls prevent oversized configuration values from overrunning query buffers while preserving the generated statements and their existing cleanup flow.
Replace the rand-based replication-lag value used in the Aurora test response with a uniform distribution backed by std::random_device. The generated delay remains in the existing 10-to-39 second range while removing the insecure legacy PRNG flagged by SonarCloud.
Generate the synthetic replication lag with the C++ random facilities instead of rand(). Keep the existing 10-to-39 second range and test-only behavior while removing the SonarCloud vulnerability on the runtime test path.
Limit the diagnostic for rejected Unix socket paths to the bytes already validated by strnlen. This prevents the error path from reading past an unterminated caller buffer while retaining the ENAMETOOLONG rejection.
Write integer payloads in network byte order and encode booleans as one-byte PostgreSQL binary values. Accept the existing t/f test data as well as true/false, reject invalid boolean text, and keep field length prefixes and offsets aligned with the encoded payloads.
Use the allocated query capacity when formatting PostgreSQL user rows so escaped comments and configuration values cannot make sprintf write past the destination buffer. Preserve the existing SQL generation and cleanup behavior.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/SQLite3_Server.cpp (5)
1043-1052: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the matched prefix length for the replication-status boundary.
addr_offsetusesk_select_replica_status_lenforREPLICA STATUS, but the guard still usesk_select_slave_status_len. Compare the query length withaddr_offsetto apply the correct boundary.Use the selected prefix
- if (strlen(query_no_space) > k_select_slave_status_len + 5) { + if (query_no_space_length > addr_offset + 5) {🤖 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 `@src/SQLite3_Server.cpp` around lines 1043 - 1052, Update the replication-status length guard within the TEST_REPLICATIONLAG block to compare strlen(query_no_space) against addr_offset plus 5, using the already selected prefix length for both REPLICA STATUS and SLAVE STATUS queries.
970-973: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winCheck all new allocations before use.
The same null-allocation defect appears across configuration loading, SQLite query emulation, and the TAP test server.
src/SQLite3_Server.cpp#L768-L770,L917-L918,L935-L936,L970-L973,L1019-L1021,L1035-L1038,L1060-L1066,L1073-L1076: check everymallocandl_strdupresult before copying, formatting, or executing.lib/ProxySQL_Config.cpp#L1450-L1451,L1499-L1500,L1559-L1565,L1618-L1619,L1669-L1670,L1730-L1731,L1790-L1791,L1968-L1969,L2240-L2241,L2289-L2290,L2434-L2435: return an error before formatting into a null query buffer.test/tap/tests/aurora.cpp#L475-L476: check the replication-lag response allocation beforesnprintf.🤖 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 `@src/SQLite3_Server.cpp` around lines 970 - 973, Check every new malloc and l_strdup result before copying, formatting, executing, or otherwise using it. In src/SQLite3_Server.cpp at lines 768-770, 917-918, 935-936, 970-973, 1019-1021, 1035-1038, 1060-1066, and 1073-1076, add failure handling around the affected query allocations; in lib/ProxySQL_Config.cpp at lines 1450-1451, 1499-1500, 1559-1565, 1618-1619, 1669-1670, 1730-1731, 1790-1791, 1968-1969, 2240-2241, 2289-2290, and 2434-2435, return an error before formatting into a null query buffer; and in test/tap/tests/aurora.cpp at lines 475-476, validate the replication-lag response allocation before snprintf.Source: Linters/SAST tools
970-974: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftKeep replacement query ownership and size metadata synchronized.
Each listed branch replaces
querybut retains the oldquery_length. The finall_free(query_length, query)then receives stale metadata. Use matchingl_alloc/l_freecalls, set the new length immediately, and release any held mutex on allocation failure.
src/SQLite3_Server.cpp#L970-L974: fix the Aurora replacement.src/SQLite3_Server.cpp#L1005-L1021: fix the Group Replication replacement.src/SQLite3_Server.cpp#L1035-L1039: fix the read-only replacement.src/SQLite3_Server.cpp#L1060-L1068: fix the replication-lag replacement.src/SQLite3_Server.cpp#L1073-L1076: fix the fallback replication-lag replacement.test/tap/tests/aurora.cpp#L473-L476: apply the same allocator and length update.🤖 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 `@src/SQLite3_Server.cpp` around lines 970 - 974, Synchronize replacement-query ownership and length metadata in each listed branch: src/SQLite3_Server.cpp lines 970-974, 1005-1021, 1035-1039, 1060-1068, and 1073-1076, plus test/tap/tests/aurora.cpp lines 473-476. Replace the old buffer using the matching l_free/l_alloc APIs, update query_length immediately to the new allocation size, and release any held mutex if allocation fails so the final l_free(query_length, query) receives valid metadata.
406-416: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRename the new
constexprvariables toUPPER_SNAKE_CASE.The new constants use names such as
k_select_version_len,k_show_tables_len, andtopology_sql_len. Rename them to the required constant format.As per coding guidelines, constants and macros must use
UPPER_SNAKE_CASE.Also applies to: 909-915
🤖 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 `@src/SQLite3_Server.cpp` around lines 406 - 416, Rename the newly added constexpr length variables in the surrounding SQL parsing code, including k_select_version_len, k_select_version_fn_len, k_select_dollar_len, k_show_tables_len, k_show_tables_from_len, k_show_tables_like_len, k_show_databases_len, k_show_schemas_len, k_select_read_only_len, k_select_slave_status_len, k_select_replica_status_len, and topology_sql_len, to UPPER_SNAKE_CASE while updating every reference to each symbol.Source: Coding guidelines
637-648: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEscape
usernamebefore constructing the SQLite query.If
usernamecontains", both implementations can generate invalid or altered SQL. Escape the value or bind it as a parameter insrc/SQLite3_Server.cpp#L637-L648andtest/tap/tests/aurora.cpp#L281-L294.🤖 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 `@src/SQLite3_Server.cpp` around lines 637 - 648, Escape or bind the username before interpolating it into the SQL constructed in the query-building block of src/SQLite3_Server.cpp (lines 637-648), and apply the same protection in test/tap/tests/aurora.cpp (lines 281-294). Ensure embedded double quotes cannot alter or invalidate the generated query while preserving the existing query behavior.lib/ProxySQL_Config.cpp (1)
1565-1569: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReplace the remaining unbounded formatter.
queryis dynamically allocated, butsprintfignoresquery_len. Usesnprintf(query, query_len, ...)and handle truncation. This path still misses the PR's bounded-formatting objective.Bound the write
- sprintf(query, q, + snprintf(query, query_len, q,🤖 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 `@lib/ProxySQL_Config.cpp` around lines 1565 - 1569, Replace the unbounded sprintf call in the query construction path with snprintf using query_len as the buffer size, then detect a truncated result and handle it consistently with the surrounding error path before using query.
🧹 Nitpick comments (1)
src/SQLite3_Server.cpp (1)
234-272: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftKeep temporary interface ownership under RAII.
The new C++ code owns
new_ifacesand eachtoken_copywith raw pointers and manual cleanup across several exit paths. Use scoped owners with custom deleters until*_ifacesis replaced.As per coding guidelines, use RAII for resource management.
🤖 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 `@src/SQLite3_Server.cpp` around lines 234 - 272, Update update_ifaces to manage new_ifaces and each token_copy with scoped RAII owners and custom deleters throughout construction and failure paths. Release the array ownership only when assigning it to *_ifaces, while preserving cleanup of partially built entries and existing old_ifaces replacement behavior.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@lib/ProxySQL_Config.cpp`:
- Around line 1565-1569: Replace the unbounded sprintf call in the query
construction path with snprintf using query_len as the buffer size, then detect
a truncated result and handle it consistently with the surrounding error path
before using query.
In `@src/SQLite3_Server.cpp`:
- Around line 1043-1052: Update the replication-status length guard within the
TEST_REPLICATIONLAG block to compare strlen(query_no_space) against addr_offset
plus 5, using the already selected prefix length for both REPLICA STATUS and
SLAVE STATUS queries.
- Around line 970-973: Check every new malloc and l_strdup result before
copying, formatting, executing, or otherwise using it. In src/SQLite3_Server.cpp
at lines 768-770, 917-918, 935-936, 970-973, 1019-1021, 1035-1038, 1060-1066,
and 1073-1076, add failure handling around the affected query allocations; in
lib/ProxySQL_Config.cpp at lines 1450-1451, 1499-1500, 1559-1565, 1618-1619,
1669-1670, 1730-1731, 1790-1791, 1968-1969, 2240-2241, 2289-2290, and 2434-2435,
return an error before formatting into a null query buffer; and in
test/tap/tests/aurora.cpp at lines 475-476, validate the replication-lag
response allocation before snprintf.
- Around line 970-974: Synchronize replacement-query ownership and length
metadata in each listed branch: src/SQLite3_Server.cpp lines 970-974, 1005-1021,
1035-1039, 1060-1068, and 1073-1076, plus test/tap/tests/aurora.cpp lines
473-476. Replace the old buffer using the matching l_free/l_alloc APIs, update
query_length immediately to the new allocation size, and release any held mutex
if allocation fails so the final l_free(query_length, query) receives valid
metadata.
- Around line 406-416: Rename the newly added constexpr length variables in the
surrounding SQL parsing code, including k_select_version_len,
k_select_version_fn_len, k_select_dollar_len, k_show_tables_len,
k_show_tables_from_len, k_show_tables_like_len, k_show_databases_len,
k_show_schemas_len, k_select_read_only_len, k_select_slave_status_len,
k_select_replica_status_len, and topology_sql_len, to UPPER_SNAKE_CASE while
updating every reference to each symbol.
- Around line 637-648: Escape or bind the username before interpolating it into
the SQL constructed in the query-building block of src/SQLite3_Server.cpp (lines
637-648), and apply the same protection in test/tap/tests/aurora.cpp (lines
281-294). Ensure embedded double quotes cannot alter or invalidate the generated
query while preserving the existing query behavior.
---
Nitpick comments:
In `@src/SQLite3_Server.cpp`:
- Around line 234-272: Update update_ifaces to manage new_ifaces and each
token_copy with scoped RAII owners and custom deleters throughout construction
and failure paths. Release the array ownership only when assigning it to
*_ifaces, while preserving cleanup of partially built entries and existing
old_ifaces replacement behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cb99b422-f90b-4135-807a-c84a8aca1459
📒 Files selected for processing (4)
lib/ProxySQL_Config.cpplib/c_tokenizer.cppsrc/SQLite3_Server.cpptest/tap/tests/aurora.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/c_tokenizer.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (ubuntu22,-tap-mysqlx)
- GitHub Check: build
- GitHub Check: run / trigger
- GitHub Check: Gitar
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must usePascalCasewith protocol prefixes such asMySQL_,PgSQL_, andProxySQL_.
Member variables must usesnake_case.
Constants and macros must useUPPER_SNAKE_CASE.
Use C++17, and gate conditional code with#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB, and#ifdef PROXYSQLCLICKHOUSE;PROXYSQLGENAImust not guard core code outsideplugins/genai/.
Consider performance implications when changing hot paths or other performance-critical code.
Use RAII for resource management and jemalloc for allocation.
Use pthread mutexes for synchronization andstd::atomic<>for counters.
Files:
lib/ProxySQL_Config.cppsrc/SQLite3_Server.cpptest/tap/tests/aurora.cpp
test/tap/tests/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
test/tap/tests/**/*.cpp: Test files intest/tap/tests/must follow the naming patterntest_*.cppor*-t.cpp.
To add a new TAP test, add the<testname>-t.cppfile and register it intest/tap/tests/Makefile/groups.json; no special Makefile target is needed becausemake <testname>-tis generated by pattern rule.
Files:
test/tap/tests/aurora.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/aurora.cpp
🪛 Cppcheck (2.21.0)
lib/ProxySQL_Config.cpp
[warning] 1451-1451: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 1500-1500: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 1619-1619: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 1670-1670: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 1731-1731: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 1791-1791: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 1969-1969: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 2241-2241: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 2290-2290: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
src/SQLite3_Server.cpp
[warning] 46-46: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
test/tap/tests/aurora.cpp
[warning] 46-46: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 476-476: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🔇 Additional comments (3)
lib/ProxySQL_Config.cpp (1)
11-25: LGTM!Also applies to: 68-68, 106-120
src/SQLite3_Server.cpp (1)
30-30: LGTM!Also applies to: 42-49
test/tap/tests/aurora.cpp (1)
27-27: LGTM!Also applies to: 46-50, 267-278, 393-421
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.0 #6026 +/- ##
==========================================
- Coverage 53.40% 53.35% -0.06%
==========================================
Files 487 487
Lines 145258 145651 +393
Branches 36704 36814 +110
==========================================
+ Hits 77579 77716 +137
- Misses 50732 50945 +213
- Partials 16947 16990 +43
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Use consistently initialized and released buffers for listener interface arrays and generated paths. The listener owners now reject accidental copies, and temporary allocations introduced while addressing the security hotspots use the same ownership conventions on success and failure paths.
Replace dynamic printf-family calls in configuration loading and synthetic test queries with literal-safe formatting helpers. Preserve the existing query text and capacity limits while keeping allocation and release paths explicit for Sonar's format-string and unsafe-copy rules.
Use early returns, reentrant tokenization, bounded string views, locale-aware character conversion, and small encoding helpers in the changed code paths. These changes preserve the existing protocol behavior while removing secondary nesting, allocation, and string-handling findings created by the hotspot fixes.
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Flatten GTID and SQLite test control flow, make GTID validation const-correct, and keep configuration cleanup on the ProxySQL allocator path. Replace the Galera tests' unsafe rand calls with standard distributions, reuse reported escaped lengths, and simplify the prepared-statement test loop without changing its test behavior.
Extract repeated configuration-field formatting into a bounded helper so the MySQL and PostgreSQL configuration readers no longer exceed Sonar's nesting threshold. Keep both escaped buffers on the ProxySQL allocator cleanup path and replace the test's raw strlen call with a std::string length before escaping.
Remove stale closing braces left after flattening the read-only and replication-lag checks, and use the selected prefix length for the replication boundary guard. Pair __cxa_demangle with free and retain the correct deallocator for SHOW CREATE TABLE buffers when the quoted-name path uses l_alloc.
Build escaped SQL values with std::string storage instead of allocating temporary and result buffers with malloc. Keep the helper return type unchanged so existing callers continue releasing the duplicated result with free.
Move the lazy replication-lag table check into a dedicated helper so the all-test-mode query handler stays within SonarCloud’s nesting limit. Preserve the existing mutex-protected initialization and selected-prefix lookup behavior.
Document why the demangled symbol buffer is released with free: abi::__cxa_demangle allocates it through malloc, so using ProxySQL’s sized allocator would be an invalid deallocation.
Create a no-op commit so the complete CI matrix runs again for PR #6026 after the self-hosted runner workspace was repaired.
Store the computed PostgreSQL user/database hash after the string-based hash assembly refactor. Without this assignment every connection retained hash zero, allowing the pool to reuse a backend authenticated for another user or database. Extend the PostgreSQL SET-parameter TAP test to seed an unprivileged connection before a privileged one and verify that both retain their expected backend identities.
Code Review ✅ Approved 3 resolved / 3 findingsHardens string handling hotspots and fixes unsafe formatting across protocol and query processing, addressing the set_checksum memcpy over-read, FFTO digest hash length bounds, and allocator pairing mismatches. ✅ 3 resolved✅ Bug: set_checksum: memcpy over-reads source shorter than 20 bytes
✅ Bug: FFTO digest hash length no longer bounded by max_digest_length
✅ Bug: Allocator pairing mismatches introduced by l_alloc/l_free swap
OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
1 participant
Summary
This branch consolidates the ProxySQL-owned SonarCloud security-hotspot cleanup for unsafe string formatting, copying, and repeated length handling.
It also merges the current origin/v3.0 branch into the feature branch. Vendored code is intentionally excluded, including ezOptionParser.hpp, sha256crypt.cpp, and SpookyV2.cpp.
Validation
The debug build completed successfully with PROXYSQL40 enabled, including the MySQLX and GenAI plugins.
Summary by CodeRabbit