fix(security): harden S6069 formatting paths and GenAI ownership - #6036
fix(security): harden S6069 formatting paths and GenAI ownership#6036renecannao wants to merge 12 commits into
Conversation
MCP_Threads_Handler owns and deletes the AI and RAG tool handlers, but this translation unit previously saw only their forward declarations. Include both concrete handler definitions before the delete expressions so their destructors and any class-specific deallocation are invoked during MCP thread teardown.
Replace all 52 cpp:S6069 hotspot call sites in MySQL_Thread.cpp, together with the three ordinary S6069 DNS-cache findings. Each fixed-size destination now passes its capacity to snprintf; the processlist address path duplicates the source string directly instead of routing it through a temporary buffer. The default-variable builder computes the allocated size once and reuses it for formatting, avoiding repeated length scans. Numeric status, port, and audit-message formatting retain their existing output while preventing a write past their local buffers.
Replace the 116 cpp:S6069 formatting hotspots in MySQL_HostGroups_Manager.cpp. Fixed-size diagnostic and statistics buffers now pass their capacity to snprintf, while dynamically allocated SQL buffers retain their allocated size for each subsequent formatting operation. The group-replication, Galera, and Aurora update paths preserve their existing output formats and allocation margins. Recording each dynamic capacity once also avoids recalculating string lengths at the formatting call, while preventing a write beyond the destination buffer.
Replace the 49 cpp:S6069 hotspots in PgSQL_Thread.cpp with bounded formatting. Listener addresses, variable names, host-cache rows, audit metadata, status output, and process-list fields now pass their destination capacity to snprintf. The default-variable list calculates the allocated name size once, including its terminator, and uses that same value for allocation and formatting. The existing strings and status values are preserved while preventing a write past local or heap-backed buffers.
Replace the 42 cpp:S6069 hotspots in MySQL_Session.cpp. JSON pointer fields, SQLSTATE copies, session identifiers, hostgroup errors, and user-facing LDAP, authentication, and charset messages now use bounded formatting. Heap-backed builders record their allocation size once and reuse it for snprintf. The generic SET-variable path also reserves for transaction variable names rewritten for modern MySQL, so its generated statements remain complete while no formatting path can write past its destination buffer.
Replace the 35 cpp:S6069 hotspots in ProxySQL_Statistics.cpp. Interval metric queries record their allocation size for snprintf, while hourly aggregation and retention SQL passes each fixed buffer capacity. The query strings and time-window calculations remain unchanged. Reusing the recorded heap capacity avoids repeated allocation-size calculations and bounds every SQL formatting operation in this source file.
Replace the 27 cpp:S6069 hotspots in PgSQL_HostGroups_Manager.cpp. PostgreSQL error rows, hostgroup status data, connection metadata, and pointer diagnostics now pass their local buffer capacities to snprintf. Heap-built server and replication SQL statements retain their allocated capacity and reuse it for formatting. The existing SQL text, diagnostics, and status values are preserved while every Sonar-reported formatting write is bounded.
Replace the 26 cpp:S6069 hotspots in Admin_Handler.cpp. Administrative error responses, lifecycle timeout values, fast-routing counts, version responses, and checksum output now use the capacity of their local buffers. Config-file errors and temporary SQL messages retain their heap allocation size for snprintf, including the timezone, table-checksum, and admin-error paths. The command responses remain unchanged while each reported formatting write is bounded.
Replace the 22 cpp:S6069 hotspots in ProxySQL_Cluster.cpp. Cluster checksum, metrics, and proxysql-server table rows now use the capacity of their fixed status buffers. Replication-hostgroup and peer-server synchronization statements record their allocated SQL capacity before calling snprintf. Existing SQL content and escaping paths are retained while the reported formatting writes cannot exceed their destination buffers.
Replace the 19 cpp:S6069 hotspots in MySQL_Monitor.cpp. Monitor query builders, connection-error messages, host:port labels, and Galera status diagnostics now pass a recorded destination capacity to snprintf. The stack-buffer host labels retain their existing fast path, while heap-backed SQL and error strings record their allocation size once for both allocation and formatting. Existing monitor queries and messages are preserved without unbounded writes.
Replace the 19 cpp:S6069 hotspots in PgSQL_Session.cpp. Internal-session JSON addresses, SQLSTATE values, client-address fallbacks, and hostgroup errors now pass their fixed-buffer capacity to snprintf. Authentication, SET-variable, tracked-variable, and query-lock messages retain their heap allocation size for formatting. The established PostgreSQL error text and query content are unchanged while each reported write is bounded.
Replace the remaining 106 cpp:S6069 formatting hotspots in 22 ProxySQL-owned source files. Logger, protocol, monitoring, query-processing, configuration, HTTP, support, and admin test code now supply each fixed or heap-backed destination capacity to snprintf. Heap-backed builders retain allocation sizes for formatting, while fixed buffers consistently use sizeof. The generated SQL, diagnostic strings, protocol output, and test messages retain their existing forms while no reported formatting write can exceed its destination buffer. The vendored ezOptionParser.hpp remains intentionally unchanged.
📝 WalkthroughWalkthroughThe pull request replaces unbounded ChangesBounded formatting hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Code Review ✅ ApprovedHardens SonarCloud S6069 formatting paths by replacing unsafe sprintf destinations with bounded snprintf calls across ProxySQL source files and corrects GenAI handler destruction. No issues found. 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 |
|
|
@cubic-dev-ai help |
@renecannao Hi! I'm cubic. Here's how I can help with your PR: Ask questions
Request reviews
Give feedback
Request fixes
|
|
@cubic review this PR |
@renecannao I have started the AI code review. It will take a few minutes to complete. |
|
We've triggered an ultrareview automatically — This large, 33-file hardening sweep across MySQL/PgSQL/ClickHouse, logging, admin, and query processing touches core buffer calculations and fixes an incomplete-delete destructor bug—a subtle miscalculation could cause truncation or corruption in production paths, so a deep review is worthwhile.. I'll post findings when complete. An ultrareview is cubic's deepest review, catching hard-to-find bugs in the most critical PRs. It runs a longer, multi-pass analysis using cubic's most capable review models, and typically takes around 30 minutes. It consumes your team's reviewed-lines allowance at 3× the standard rate. Automated ultrareviews are disabled by default. We triggered this run as part of your trial. Want cubic to do this for every high-risk PR? Enable auto-ultrareview in your settings. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/MySQL_HostGroups_Manager.cpp (1)
4327-4347: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFix the reused query buffer size for the double-hostname CASE statement.
query_sizeis computed once for the "UPDATE OR IGNORE ..." statement, which references_hostnamewith a single%s. The buffer andquery_sizeare then reused for the later "UPDATE mysql_servers_incoming SET status=(CASE ...)" statement, which references_hostnamewith two%splaceholders.The margin left after accounting for the format-string literal is about 75 bytes minus
strlen(_hostname). For hostnames longer than approximately 75 characters (for example, long Kubernetes service DNS names or cloud FQDNs),snprintftruncates this statement silently. The truncatedUPDATEcan leave a Group Replication server in the wrong hostgroup or fail outright, corrupting routing state without any error being logged.Compute a size specific to this statement instead of reusing
query_sizefrom the earlier allocation. The same pattern repeats inupdate_group_replication_set_read_only(lines 4433-4451).🐛 Proposed fix for both functions
q=(char *)"UPDATE mysql_servers_incoming SET status=(CASE " " (SELECT status FROM mysql_servers_incoming WHERE hostname='%s' AND port=%d AND" " hostgroup_id=(SELECT offline_hostgroup FROM mysql_group_replication_hostgroups WHERE writer_hostgroup=%d)) WHEN 2 THEN 2 ELSE 0 END)" " WHERE hostname='%s' AND port=%d AND hostgroup_id=(SELECT offline_hostgroup FROM mysql_group_replication_hostgroups WHERE writer_hostgroup=%d)"; - snprintf(query, query_size, q, _hostname, _port, _writer_hostgroup, _hostname, _port, _writer_hostgroup); + { + size_t q3_size = strlen(q) + 2 * strlen(_hostname) + 64; + if (q3_size > query_size) { + query = (char *)realloc(query, q3_size); + query_size = q3_size; + } + } + snprintf(query, query_size, q, _hostname, _port, _writer_hostgroup, _hostname, _port, _writer_hostgroup);Apply the equivalent change to the matching statement in
update_group_replication_set_read_only.Also applies to: 4433-4451
🤖 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/MySQL_HostGroups_Manager.cpp` around lines 4327 - 4347, In the query-building logic of the current function and update_group_replication_set_read_only, allocate and size the buffer specifically for the final UPDATE ... CASE statement, accounting for both _hostname substitutions, instead of reusing query_size computed for the earlier single-hostname query. Apply the same correction to the matching statement in update_group_replication_set_read_only and preserve the existing SQL and execution flow.
🤖 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/PgSQL_Logger.cpp`:
- Around line 868-874: Update both filename allocation branches in the logger’s
filename construction logic to size filen_size from the actual formatted
events.log_file_id length, while retaining the existing directory and basename
formatting. Ensure nine-digit and larger IDs are not truncated, and add coverage
for a nine-digit log ID.
In `@lib/PgSQL_Thread.cpp`:
- Around line 1625-1626: Update both snprintf calls handling num_threads to use
the unsigned integer format specifier %u instead of %d, preserving the existing
buffer and fallback behavior.
In `@lib/ProxySQL_Statistics.cpp`:
- Around line 563-565: Check the result of every malloc assignment to query in
the affected branches before invoking snprintf. Update the surrounding
query-building flow in the relevant function to return or propagate an error on
allocation failure, ensuring no formatting call dereferences a null buffer;
apply the same guard to all listed allocation sites.
- Around line 555-575: The metric query templates in the interval switch use
`%d` for `time_t` bounds, causing a variadic type mismatch. Update both `query1`
and `query2` to use `%lld`, and cast each `ts-interval` and `ts` argument passed
to `snprintf` to `long long` while preserving the existing query selection and
interval behavior.
---
Outside diff comments:
In `@lib/MySQL_HostGroups_Manager.cpp`:
- Around line 4327-4347: In the query-building logic of the current function and
update_group_replication_set_read_only, allocate and size the buffer
specifically for the final UPDATE ... CASE statement, accounting for both
_hostname substitutions, instead of reusing query_size computed for the earlier
single-hostname query. Apply the same correction to the matching statement in
update_group_replication_set_read_only and preserve the existing SQL and
execution flow.
🪄 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: 37238019-56ed-472a-aa12-c7d81ca0a9db
📒 Files selected for processing (33)
lib/Admin_Handler.cpplib/ClickHouse_Server.cpplib/GTID_Server_Data.cpplib/MySQL_HostGroups_Manager.cpplib/MySQL_Logger.cpplib/MySQL_Monitor.cpplib/MySQL_PreparedStatement.cpplib/MySQL_Protocol.cpplib/MySQL_Query_Processor.cpplib/MySQL_ResultSet.cpplib/MySQL_Session.cpplib/MySQL_Thread.cpplib/MySQL_encode.cpplib/PgSQL_HostGroups_Manager.cpplib/PgSQL_Logger.cpplib/PgSQL_Protocol.cpplib/PgSQL_Query_Processor.cpplib/PgSQL_Session.cpplib/PgSQL_Thread.cpplib/ProxySQL_Admin_Tests.cpplib/ProxySQL_Admin_Tests2.cpplib/ProxySQL_Cluster.cpplib/ProxySQL_Config.cpplib/ProxySQL_HTTP_Server.cpplib/ProxySQL_Statistics.cpplib/QP_query_digest_stats.cpplib/Query_Cache.cpplib/Query_Processor.cpplib/debug.cpplib/mysql_connection.cpplib/proxysql_coredump.cpplib/sqlite3db.cppplugins/genai/src/MCP_Thread.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Gitar
- GitHub Check: lint
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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/GTID_Server_Data.cpplib/debug.cpplib/ProxySQL_Admin_Tests.cpplib/ProxySQL_Config.cpplib/PgSQL_Query_Processor.cpplib/MySQL_Query_Processor.cppplugins/genai/src/MCP_Thread.cpplib/ProxySQL_HTTP_Server.cpplib/QP_query_digest_stats.cpplib/MySQL_PreparedStatement.cpplib/proxysql_coredump.cpplib/MySQL_Protocol.cpplib/MySQL_ResultSet.cpplib/mysql_connection.cpplib/Query_Processor.cpplib/MySQL_Logger.cpplib/ProxySQL_Cluster.cpplib/sqlite3db.cpplib/PgSQL_HostGroups_Manager.cpplib/ProxySQL_Admin_Tests2.cpplib/PgSQL_Logger.cpplib/ProxySQL_Statistics.cpplib/MySQL_encode.cpplib/ClickHouse_Server.cpplib/PgSQL_Protocol.cpplib/Query_Cache.cpplib/Admin_Handler.cpplib/MySQL_Monitor.cpplib/PgSQL_Session.cpplib/MySQL_Session.cpplib/MySQL_HostGroups_Manager.cpplib/MySQL_Thread.cpplib/PgSQL_Thread.cpp
🧠 Learnings (1)
📚 Learning: 2026-07-13T08:28:59.932Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5861
File: lib/ProxySQL_Cluster.cpp:2251-2255
Timestamp: 2026-07-13T08:28:59.932Z
Learning: When reviewing ProxySQL cluster sync code that populates/updates `mysql_servers_v2` (e.g., paths like `pull_mysql_servers_v2_from_peer` and other cluster sync logic), remember that the MySQL server status `SHUNNED_AWS_BGD` is runtime-only: for cluster synchronization it is normalized together with `SHUNNED` to `ONLINE` before values are exposed/checksummed for synchronization. Therefore, during normal cluster sync operation you should not expect case-mismatched or “raw” `SHUNNED`/`SHUNNED_AWS_BGD` strings to reach the `mysql_servers_v2` insert/update path—if they do, treat it as evidence that the normalization step was bypassed or altered (and verify the normalization logic and call flow).
Applied to files:
lib/ProxySQL_Cluster.cpp
🪛 Cppcheck (2.21.0)
lib/debug.cpp
[warning] 384-384: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 385-385: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 387-387: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 388-388: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 390-390: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 392-392: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
lib/MySQL_PreparedStatement.cpp
[warning] 1008-1008: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 1010-1010: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 1012-1012: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 1015-1015: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 1018-1018: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 1020-1020: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 1022-1022: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 1024-1024: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
lib/Query_Processor.cpp
[warning] 736-736: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 2810-2810: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
lib/sqlite3db.cpp
[warning] 551-551: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 581-581: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
lib/PgSQL_HostGroups_Manager.cpp
[warning] 175-175: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 177-177: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 188-188: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 190-190: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 1370-1370: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 1757-1757: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 1796-1796: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 3024-3024: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 3026-3026: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 3027-3027: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 3047-3047: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 3279-3279: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 3280-3280: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
lib/PgSQL_Logger.cpp
[warning] 870-870: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 874-874: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 905-905: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 909-909: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 1017-1017: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 1114-1114: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 1237-1237: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 1275-1275: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
lib/ProxySQL_Statistics.cpp
[warning] 565-565: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 575-575: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 623-623: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 633-633: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 669-669: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 679-679: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 717-717: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 727-727: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 765-765: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 775-775: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
lib/MySQL_encode.cpp
[error] 42-42: If memory allocation fails
(nullPointerArithmeticOutOfMemory)
lib/ClickHouse_Server.cpp
[warning] 701-701: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 733-733: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 864-864: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 878-878: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 946-946: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 959-959: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 1072-1072: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 1302-1302: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
lib/Query_Cache.cpp
[warning] 659-659: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 663-663: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 665-665: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 669-669: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 671-671: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 675-675: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 677-677: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 681-681: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 683-683: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 687-687: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 689-689: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 693-693: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 695-695: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 699-699: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
lib/MySQL_Thread.cpp
[warning] 1526-1526: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 3023-3023: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 3292-3292: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 5420-5420: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 5422-5422: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 5426-5426: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 5428-5428: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 5432-5432: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 5415-5415: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 6041-6041: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🔇 Additional comments (34)
lib/Admin_Handler.cpp (1)
621-625: LGTM!Also applies to: 628-640, 833-841, 843-850, 874-901, 903-917, 926-956, 959-977, 1553-1568, 1620-1631, 1655-1710, 1780-1785, 1795-1800, 1855-1901, 1920-1937, 2251-2267, 2382-2410, 2394-2409, 2517-2534, 2564-2582, 2668-2685, 2796-2813, 3479-3487, 3489-3500, 3769-3792, 3796-3823, 4200-4220, 4560-4582, 5450-5470
lib/ClickHouse_Server.cpp (1)
694-708: LGTM!Also applies to: 730-740, 858-871, 872-885, 940-951, 953-965, 986-1023, 1066-1078, 1295-1306, 1790-1793
lib/GTID_Server_Data.cpp (1)
172-174: LGTM!lib/MySQL_HostGroups_Manager.cpp (1)
1339-1352: LGTM!Also applies to: 1953-1957, 2001-2010, 2438-2449, 3234-3283, 3502-3574, 4297-4326, 4386-4432, 4453-4471, 4523-4693, 4712-4845, 5071-5228, 5230-5310, 5324-5537, 5544-5772, 5820-5862, 5867-5987, 6711-6737, 6896-7131, 7133-7252
lib/MySQL_Logger.cpp (1)
583-594: LGTM!Also applies to: 691-699, 701-705, 1141-1152, 1155-1171, 1381-1397, 1439-1455, 1556-1565, 1838-1849, 2182-2201, 2220-2229
lib/MySQL_Monitor.cpp (1)
1640-1678: LGTM!Also applies to: 1762-1774, 1966-1971, 2177-2189, 2440-2449, 2459-2467, 2571-2583, 2740-2745, 2886-2897, 3407-3414, 3468-3475, 3526-3538, 6288-6300, 6396-6409, 8403-8410, 9296-9303, 9448-9460, 9944-9957, 10116-10118
lib/MySQL_PreparedStatement.cpp (1)
1004-1027: LGTM!lib/MySQL_Protocol.cpp (1)
4202-4206: LGTM!lib/proxysql_coredump.cpp (1)
74-76: LGTM!lib/sqlite3db.cpp (1)
545-551: LGTM!Also applies to: 575-582, 681-688, 1121-1134
plugins/genai/src/MCP_Thread.cpp (1)
11-13: LGTM!lib/MySQL_Query_Processor.cpp (1)
97-97: LGTM!Also applies to: 834-834
lib/MySQL_ResultSet.cpp (1)
450-450: LGTM!lib/MySQL_Session.cpp (1)
1133-1137: LGTM!Also applies to: 1175-1175, 1245-1247, 2780-2780, 2829-2831, 2904-2906, 2960-2960, 3038-3038, 3087-3095, 3106-3108, 3116-3118, 3129-3131, 3224-3224, 3312-3312, 3398-3398, 3755-3755, 3847-3847, 3947-3947, 4036-4038, 4232-4234, 4448-4448, 4496-4496, 5188-5190, 5662-5662, 5687-5687, 6727-6729, 6796-6798, 6824-6826, 7666-7673, 7971-7973, 8012-8012, 8089-8089, 8273-8273, 8420-8422, 8697-8697, 8782-8782, 9185-9187
lib/MySQL_Thread.cpp (1)
1524-1526: LGTM!Also applies to: 1874-1874, 1990-1990, 2078-2078, 2140-2144, 2471-2471, 3020-3023, 3291-3293, 3469-3469, 4712-4712, 4827-4827, 4841-4841, 5001-5001, 5343-5343, 5414-5433, 5519-5519, 5536-5536, 5550-5568, 5593-5671, 6040-6042, 6063-6082, 6095-6095, 6112-6120, 6134-6138, 6214-6214, 6228-6228, 6240-6243
lib/MySQL_encode.cpp (1)
42-42: LGTM!lib/PgSQL_HostGroups_Manager.cpp (1)
174-178: LGTM!Also applies to: 187-191, 1368-1370, 1755-1757, 1793-1796, 2487-2489, 3023-3028, 3046-3051, 3066-3066, 3278-3281, 3305-3339
lib/PgSQL_Logger.cpp (1)
390-390: LGTM!Also applies to: 476-476, 485-485, 675-697, 1015-1017, 1112-1114, 1235-1237, 1273-1275, 1584-1584
lib/PgSQL_Protocol.cpp (1)
994-994: LGTM!Also applies to: 1004-1004, 1185-1187, 1266-1268, 1661-1661, 1670-1670
lib/Query_Processor.cpp (2)
2197-2200: LGTM!Also applies to: 2737-2738, 2804-2810
730-736: 🎯 Functional CorrectnessNo change needed.
keylenincludes 30 bytes of headroom for separators, decimal digits, and the terminating NUL, so the dynamic allocation is sufficient.> Likely an incorrect or invalid review comment.lib/debug.cpp (1)
383-393: LGTM!lib/mysql_connection.cpp (1)
814-814: LGTM!Also applies to: 3267-3267, 3298-3298
lib/PgSQL_Query_Processor.cpp (1)
192-193: LGTM!Also applies to: 452-453
lib/PgSQL_Session.cpp (1)
676-681: LGTM!Also applies to: 768-770, 788-790, 829-831, 1400-1402, 1456-1459, 1734-1736, 2045-2047, 2089-2091, 2507-2510, 4009-4013, 4081-4085, 4097-4101, 4156-4157, 4175-4177, 4628-4630, 5245-5249, 6135-6137, 6545-6548
lib/PgSQL_Thread.cpp (1)
1239-1243: LGTM!Also applies to: 1421-1423, 1488-1490, 1627-1630, 1950-1952, 2399-2403, 2634-2638, 2797-2799, 3907-3908, 3922-3923, 3937-3938, 4114-4117, 4434-4435, 4508-4527, 4640-4647, 4658-4661, 4708-4763, 5164-5167, 5186-5197, 5209-5210, 5222-5233, 5246-5255, 5320-5321, 5334-5335, 5346-5349
lib/ProxySQL_Admin_Tests.cpp (1)
89-90: LGTM!Also applies to: 109-111
lib/ProxySQL_Admin_Tests2.cpp (1)
393-395: LGTM!Also applies to: 428-430, 1256-1258, 1292-1294, 1308-1310, 1382-1384, 1393-1396, 1403-1406, 1422-1424, 1436-1439, 1454-1459
lib/ProxySQL_Cluster.cpp (1)
2291-2293: LGTM!Also applies to: 2324-2326, 2335-2337, 2368-2370, 2379-2381, 2879-2881, 4534-4535, 4562-4571, 4610-4627, 4656-4658
lib/ProxySQL_Config.cpp (1)
156-158: LGTM!lib/ProxySQL_HTTP_Server.cpp (1)
141-148: LGTM!Also applies to: 168-170
lib/ProxySQL_Statistics.cpp (1)
833-852: LGTM!Also applies to: 913-932, 1066-1085, 1251-1270, 1356-1375
lib/QP_query_digest_stats.cpp (1)
164-165: LGTM!Also applies to: 195-197
lib/Query_Cache.cpp (1)
658-700: LGTM!
| size_t filen_size = strlen(events.base_filename) + 11; | ||
| filen=(char *)malloc(filen_size); | ||
| snprintf(filen, filen_size, "%s.%08d",events.base_filename,events.log_file_id); | ||
| } else { // relative path | ||
| filen=(char *)malloc(strlen(events.datadir)+strlen(events.base_filename)+11); | ||
| sprintf(filen,"%s/%s.%08d",events.datadir,events.base_filename,events.log_file_id); | ||
| size_t filen_size = strlen(events.datadir) + strlen(events.base_filename) + 11; | ||
| filen=(char *)malloc(filen_size); | ||
| snprintf(filen, filen_size, "%s/%s.%08d",events.datadir,events.base_filename,events.log_file_id); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Size log filenames for the full file ID.
At Line [868] and Line [903], filen_size reserves space for only eight ID digits. %08d pads short values but does not limit longer values. When a log ID reaches nine digits, snprintf truncates the filename. The logger can then open the wrong path or fail to rotate. Compute the required length from the formatted ID, and add a test for a nine-digit ID.
Also applies to: 903-909
🧰 Tools
🪛 Cppcheck (2.21.0)
[warning] 870-870: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 874-874: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🤖 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/PgSQL_Logger.cpp` around lines 868 - 874, Update both filename allocation
branches in the logger’s filename construction logic to size filen_size from the
actual formatted events.log_file_id length, while retaining the existing
directory and basename formatting. Ensure nine-digit and larger IDs are not
truncated, and add coverage for a nine-digit log ID.
| snprintf(intbuf, sizeof(intbuf), "%d", (num_threads ? num_threads : DEFAULT_NUM_THREADS)); | ||
| return strdup(intbuf); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 --glob '*.{cpp,h,hpp}' '\bnum_threads\b' .Repository: sysown/proxysql
Length of output: 2043
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files '*PgSQL_Thread.cpp' '*.{cpp,h,hpp}' | head -200
printf '%s\n' '--- references to num_threads ---'
rg -n -C 6 --glob '*.{cpp,h,hpp}' '\bnum_threads\b' .
printf '%s\n' '--- target regions ---'
file=$(git ls-files '*PgSQL_Thread.cpp' | head -1)
if [ -n "$file" ]; then
sed -n '1608,1632p;4650,4672p' "$file"
fiRepository: sysown/proxysql
Length of output: 2014
🌐 Web query:
site:github.com/sysown/proxysql "PgSQL_Thread.cpp" "num_threads"
💡 Result:
PgSQL_Thread.cpp is a source file within the ProxySQL codebase that contains the implementation for handling PostgreSQL thread logic, specifically related to the PgSQL_Threads_Handler class [1][2]. In the context of this file, num_threads is typically a member variable or parameter used to represent the number of active PostgreSQL thread workers within the ProxySQL instance [1][3][2]. Key details regarding num_threads in PgSQL_Thread.cpp include: 1. Thread Management: The variable is used extensively by the PgSQL_Threads_Handler to iterate over and manage thread workers, such as in the creation of backend connections, handling of process lists, and aggregation of transaction-poisoned counters [1][4][3]. 2. Global Status: The number of PgSQL thread workers is exposed in ProxySQL statistics via the PgSQL_Thread_Workers gauge, which is populated by the PgSQL_Threads_Handler::SQL3_GlobalStatus method in PgSQL_Thread.cpp [2]. 3. Resource Management: It is also used to balance connection caching, where it determines the distribution of cached connections between local thread caches and the shared host group manager pool [5]. 4. Default Value: The codebase defines a constant DEFAULT_NUM_THREADS (typically set to 4) within the context of these thread operations [1]. The file is integral to how ProxySQL scales and manages PostgreSQL backend connections, ensuring that workloads are distributed across the configured number of threads [1][5].
Citations:
- 1: https://github.com/sysown/proxysql/blob/9cc20a87/lib/PgSQL_Thread.cpp
- 2: https://github.com/sysown/proxysql/blob/v3.0/doc/Stats_Tables.md
- 3: fb690e7
- 4: 5219d4b
- 5: a1ca6a0
🏁 Script executed:
#!/bin/bash
set -euo pipefail
url='https://raw.githubusercontent.com/sysown/proxysql/9cc20a87/lib/PgSQL_Thread.cpp'
tmp=$(mktemp)
trap 'rm -f "$tmp"' EXIT
curl --fail --silent --show-error --location "$url" -o "$tmp"
printf '%s\n' '--- target regions ---'
sed -n '1615,1632p;4655,4672p' "$tmp"
printf '%s\n' '--- declarations, assignments, and uses ---'
rg -n -C 5 '\b(num_threads|DEFAULT_NUM_THREADS)\b' "$tmp"Repository: sysown/proxysql
Length of output: 2014
🌐 Web query:
site:github.com/sysown/proxysql/blob/lib/PgSQL_Thread.cpp "snprintf(intbuf" "num_threads"
💡 Result:
No matching result was found for:
site:github.com/sysown/proxysql/blob/lib/PgSQL_Thread.cpp "snprintf(intbuf" "num_threads"
The ProxySQL repository exists, but that exact file/path or code fragment was not indexed by search. (github.com)
Citations:
🌐 Web query:
"DEFAULT_NUM_THREADS" "num_threads" "PgSQL_Threads_Handler"
💡 Result:
The term DEFAULT_NUM_THREADS in the context of PgSQL_Threads_Handler refers to a hardcoded constant in the ProxySQL source code, defined as 4 [1][2]. PgSQL_Threads_Handler is a class within ProxySQL responsible for managing PostgreSQL-related threads [3][4]. The constant DEFAULT_NUM_THREADS (defined as 4) sets the initial or default number of worker threads created by the handler when managing PostgreSQL connections [1][2]. The handler uses these threads to process queries and handle client sessions [5][6]. In the ProxySQL source code, the PgSQL_Threads_Handler class includes an initialization function (init) that allows for the specification of the number of threads to create, defaulting to this predefined constant when no specific value is provided [3]. The handler also manages thread heartbeats and allows for the addition or deletion of listeners that utilize these threads [5][7]. It is important to note that DEFAULT_NUM_THREADS is a common naming convention for constants in many C++ and Java projects and appears in various software contexts (such as Apache Druid, Solana, and others) with different assigned values [8][9][10][11]; however, specifically for the ProxySQL PgSQL_Threads_Handler, it is defined as 4 [1][2].
Citations:
- 1: https://github.com/sysown/proxysql/blob/9cc20a87/lib/PgSQL_Thread.cpp
- 2: https://fossies.org/linux/proxysql/lib/PgSQL_Thread.cpp
- 3: https://github.com/sysown/proxysql/blob/463a233b/include/PgSQL_Thread.h
- 4: https://github.com/sysown/proxysql/blob/9cc20a87/include/PgSQL_Thread.h
- 5: e4c0f5c
- 6: 4d91452
- 7: https://github.com/sysown/proxysql/blob/463a233b/lib/PgSQL_Thread.cpp
- 8: http://static.druid.io/api/0.12.0/io/druid/java/util/common/concurrent/ExecutorServiceConfig.html
- 9: https://rust.velas.com/solana_runtime/accounts_db/constant.DEFAULT_NUM_THREADS.html
- 10: https://vertx.io/docs/apidocs/io/vertx/micrometer/VertxInfluxDbOptions.html
- 11: http://plinycompute.cs.rice.edu/api/0.5.0/d2/dfb/_configuration_8h_source.html
🌐 Web query:
site:fossies.org/linux/proxysql/lib/PgSQL_Thread.cpp "num_threads"
💡 Result:
In computer programming, num_threads is a common variable or parameter name used to specify the number of execution threads a program or specific task should utilize [1]. It serves as a configuration setting that allows developers to control concurrency, enabling a workload to be distributed across multiple processing units or to manage how a service handles parallel operations [1]. In practice, this identifier is often used when allocating resources or initializing multi-threaded environments, such as setting up socket listeners for network services or defining the parallelism level for computational tasks [1]. Because it is a generic naming convention rather than a language-specific keyword, its exact implementation depends on the specific library or application code where it is defined [1].
Citations:
🌐 Web query:
site:github.com/sysown/proxysql "unsigned int num_threads"
💡 Result:
In programming, the term unsigned int num_threads is not a keyword or built-in standard; it is a common variable declaration used in C and C++ to store the count of threads to be created or managed by an application [1][2][3]. Because the number of threads must always be a non-negative integer, programmers use the unsigned int data type to explicitly enforce this constraint, ensuring the value cannot be negative [1][3]. Key characteristics include: 1. Data Type: unsigned int (an unsigned integer type) ensures the value is 0 or greater. 2. Purpose: It is typically used as a parameter for thread pool initialization [2][4][5], a counter in loops iterating through threads [1][3], or a configuration variable to define concurrency levels [3]. 3. Context: It appears frequently in multi-threaded applications, such as database proxies or performance testing tools, where managing specific thread counts is required to optimize resource usage and workload distribution [1][2][3]. For example, in codebases like ProxySQL, this variable is used to configure listener managers or worker thread handlers, defining how many concurrent execution units the system should spawn [1][4][5].
Citations:
- 1: https://github.com/sysown/proxysql/blob/9cc20a87/lib/MySQL_Thread.cpp
- 2: https://github.com/sysown/proxysql/blob/9cc20a87/include/MySQL_Thread.h
- 3: https://github.com/sysown/proxysql/blob/9cc20a87/test/tap/tests/set_testing-multi-t.cpp
- 4: https://github.com/sysown/proxysql/blob/9cc20a87/include/PgSQL_Thread.h
- 5: https://github.com/sysown/proxysql/blob/9cc20a87/plugins/genai/include/GenAI_Thread.h
🌐 Web query:
"PgSQL_Threads_Handler" "num_threads;" ProxySQL
💡 Result:
In ProxySQL, PgSQL_Threads_Handler is the internal C++ class responsible for managing the pool of worker threads that handle PostgreSQL traffic [1][2]. The member variable num_threads within the PgSQL_Threads_Handler class defines the number of worker threads assigned to process PostgreSQL traffic [1][2][3]. When ProxySQL initializes the PgSQL module, it uses this value to determine how many worker threads to create [2]. These worker threads execute the pgsql_worker_thread_func routine to manage client-to-backend communication and handle queries [2]. It is important to distinguish this internal C++ class member from user-configurable settings: 1. Internal Class Member (num_threads): This is an unsigned integer defined within the PgSQL_Threads_Handler C++ code [1][4]. It is set during the initialization of the PgSQL module based on the ProxySQL global configuration [2]. 2. User-Configurable Settings: While users do not directly modify the num_threads member of the PgSQL_Threads_Handler class, they control the number of threads used for PostgreSQL traffic via ProxySQL global variables (typically set in the main configuration or via the admin interface, similar to mysql-threads for MySQL traffic) [5][6]. Additionally, for the PostgreSQL Monitoring module, users can specifically configure the number of worker threads using the pgsql-monitor_threads variable [7]. In summary, PgSQL_Threads_Handler::num_threads is the internal variable representing the active count of worker threads designated for PostgreSQL traffic processing, which is established at runtime based on the configured thread settings [2][8].
Citations:
- 1: https://github.com/sysown/proxysql/blob/463a233b/include/PgSQL_Thread.h
- 2: https://github.com/sysown/proxysql/blob/9cc20a87/src/main.cpp
- 3: f3ea179
- 4: https://github.com/sysown/proxysql/blob/9cc20a87/include/PgSQL_Thread.h
- 5: https://proxysql.com/documentation/proxysql-threads/
- 6: How can i find the correct number of mysql-threads #1166
- 7: https://proxysql.com/documentation/postgresql-backend-monitoring/
- 8: fb690e7
Use %u for num_threads in both snprintf calls. num_threads is unsigned, but %d requires int; the mismatch causes undefined behavior.
🤖 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/PgSQL_Thread.cpp` around lines 1625 - 1626, Update both snprintf calls
handling num_threads to use the unsigned integer format specifier %u instead of
%d, preserving the existing buffer and fallback behavior.
| size_t query_size = 0; | ||
| char *query1 = (char *)"SELECT SUBSTR(FROM_UNIXTIME(timestamp),0,20) ts, timestamp, Client_Connections_aborted, Client_Connections_connected, Client_Connections_created, Server_Connections_aborted, Server_Connections_connected, Server_Connections_created, ConnPool_get_conn_failure, ConnPool_get_conn_immediate, ConnPool_get_conn_success, Questions, Slow_queries, GTID_consistent_queries FROM mysql_connections WHERE timestamp BETWEEN %d AND %d ORDER BY timestamp"; | ||
| char *query2 = (char *)"SELECT SUBSTR(FROM_UNIXTIME(timestamp),0,20) ts, timestamp, Client_Connections_aborted, Client_Connections_connected, Client_Connections_created, Server_Connections_aborted, Server_Connections_connected, Server_Connections_created, ConnPool_get_conn_failure, ConnPool_get_conn_immediate, ConnPool_get_conn_success, Questions, Slow_queries, GTID_consistent_queries FROM mysql_connections_hour WHERE timestamp BETWEEN %d AND %d ORDER BY timestamp"; | ||
| time_t ts = time(NULL); | ||
| switch (interval) { | ||
| case 1800: | ||
| case 3600: | ||
| case 7200: | ||
| query = (char *)malloc(strlen(query1)+128); | ||
| sprintf(query, query1, ts-interval, ts); | ||
| query_size = strlen(query1) + 128; | ||
| query = (char *)malloc(query_size); | ||
| snprintf(query, query_size, query1, ts-interval, ts); | ||
| break; | ||
| case 28800: | ||
| case 86400: | ||
| case 259200: | ||
| case 604800: | ||
| case 2592000: | ||
| case 7776000: | ||
| query = (char *)malloc(strlen(query2)+128); | ||
| sprintf(query, query2, ts-interval, ts); | ||
| query_size = strlen(query2) + 128; | ||
| query = (char *)malloc(query_size); | ||
| snprintf(query, query_size, query2, ts-interval, ts); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'time_t ts|BETWEEN %d|snprintf\(query' lib/ProxySQL_Statistics.cpp
rg -n -C 3 '\btime_t\b' --glob '*.{cpp,h,hpp}' .Repository: sysown/proxysql
Length of output: 2043
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- target file context ---'
sed -n '520,790p' lib/ProxySQL_Statistics.cpp
printf '%s\n' '--- timestamp declarations and query templates ---'
rg -n -C 4 'time_t|BETWEEN %d|BETWEEN %lld|snprintf\(query|query[12]' lib/ProxySQL_Statistics.cpp
printf '%s\n' '--- related timestamp declarations and schema definitions ---'
rg -n -C 3 '\btimestamp\b' --glob '*.{cpp,h,hpp,sql}' . | head -n 240Repository: sysown/proxysql
Length of output: 2014
🏁 Script executed:
#!/bin/bash
set -euo pipefail
url='https://raw.githubusercontent.com/sysown/proxysql/master/lib/ProxySQL_Statistics.cpp'
tmp=$(mktemp)
trap 'rm -f "$tmp"' EXIT
curl -fsSL "$url" -o "$tmp"
printf '%s\n' '--- matching declarations and calls ---'
rg -n -C 5 'time_t|BETWEEN %d|BETWEEN %lld|snprintf\(query|query[12]' "$tmp"
printf '%s\n' '--- target regions ---'
sed -n '520,790p' "$tmp"
printf '%s\n' '--- timestamp schema references ---'
curl -fsSL 'https://api.github.com/search/code?q=repo%3Asysown%2Fproxysql+mysql_connections+timestamp' |
jq -r '.items[]?.html_url' 2>/dev/null || trueRepository: sysown/proxysql
Length of output: 2014
🌐 Web query:
github sysown proxysql lib/ProxySQL_Statistics.cpp "BETWEEN %d" "time_t ts"
💡 Result:
The code snippet WHERE timestamp BETWEEN %d AND %d found in lib/ProxySQL_Statistics.cpp within the sysown/proxysql repository is used in SQL query templates for retrieving time-series metrics from ProxySQL's on-disk SQLite statistics databases [1][2][3]. In this context, the placeholders %d are filled using sprintf to generate queries dynamically at runtime [1][2]. The values injected into these placeholders are time-based integers calculated using a time_t variable (ts), which represents the current time (time(NULL)), and a specified interval [1][2]. Specifically, the queries typically take the form: sprintf(query, query_template, ts - interval, ts); This effectively constructs a SQL BETWEEN clause that filters database records to those falling within the requested time window (from ts - interval to ts) [1][2][3]. This logic is applied across various statistics tables—such as mysql_connections and system_cpu—to aggregate and retrieve metrics stored in high-resolution and low-resolution (hourly) tables [1][3][4]. The pattern ensures that ProxySQL can efficiently extract historical performance data based on the desired reporting duration [4].
Citations:
- 1: https://github.com/sysown/proxysql/blob/9cc20a87/lib/ProxySQL_Statistics.cpp
- 2: https://github.com/sysown/proxysql/blob/463a233b/lib/ProxySQL_Statistics.cpp
- 3: https://fossies.org/linux/proxysql/lib/ProxySQL_Statistics.cpp
- 4: https://deepwiki.com/sysown/proxysql/7.2-statistics-and-metrics-collection
Use a matching format for time_t in the metric query templates.
The five metric getters pass time_t values to %d. This variadic type mismatch can produce incorrect BETWEEN bounds. Cast the timestamps to long long and use %lld in each query template.
🧰 Tools
🪛 Cppcheck (2.21.0)
[warning] 565-565: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 575-575: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🤖 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_Statistics.cpp` around lines 555 - 575, The metric query
templates in the interval switch use `%d` for `time_t` bounds, causing a
variadic type mismatch. Update both `query1` and `query2` to use `%lld`, and
cast each `ts-interval` and `ts` argument passed to `snprintf` to `long long`
while preserving the existing query selection and interval behavior.
| query_size = strlen(query1) + 128; | ||
| query = (char *)malloc(query_size); | ||
| snprintf(query, query_size, query1, ts-interval, ts); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Check each query-buffer allocation before calling snprintf.
Each branch calls snprintf immediately after malloc(query_size). If allocation fails, query is NULL and the write can crash ProxySQL. Use a checked allocation helper or return an error before formatting.
query = (char *)malloc(query_size);
+if (query == nullptr) {
+ proxy_error("Unable to allocate metrics query buffer\n");
+ return nullptr;
+}
snprintf(query, query_size, query1, ...);Cppcheck reports possible null dereferences at these formatting calls.
Also applies to: 573-575, 621-623, 631-633, 667-669, 677-679, 715-717, 725-727, 763-765, 773-775
🧰 Tools
🪛 Cppcheck (2.21.0)
[warning] 565-565: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🤖 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_Statistics.cpp` around lines 563 - 565, Check the result of
every malloc assignment to query in the affected branches before invoking
snprintf. Update the surrounding query-building flow in the relevant function to
return or propagate an error on allocation failure, ensuring no formatting call
dereferences a null buffer; apply the same guard to all listed allocation sites.
Source: Linters/SAST tools
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ultrareview completed in 22m 40s
7 issues found across 33 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/ProxySQL_Statistics.cpp">
<violation number="1" location="lib/ProxySQL_Statistics.cpp:565">
P2: The metrics range queries can eventually select the wrong timestamp window because the updated `snprintf` calls pass `time_t` values into SQL templates that still use `%d`. `%d` is an `int` specifier, so this introduces a varargs type mismatch and risks truncated timestamps as epoch values grow. It would be safer to align the format specifiers with `time_t` (for example `%ld` with matching casts, or an explicit 64-bit format) across these query templates.</violation>
</file>
<file name="lib/PgSQL_Logger.cpp">
<violation number="1" location="lib/PgSQL_Logger.cpp:868">
P2: Log rotation filename construction can silently truncate once `log_file_id` grows beyond 8 digits. The new `snprintf` calls are bounded by a fixed `+ 11` allocation, but `%08d` allows more than 8 digits for large values, so the path can be cut off without any error handling. Consider sizing from `snprintf(nullptr, 0, ...) + 1` (or otherwise handling variable digit length) and checking the return value so rotation never reuses/truncates filenames.</violation>
</file>
<file name="lib/ProxySQL_Cluster.cpp">
<violation number="1" location="lib/ProxySQL_Cluster.cpp:2880">
P2: The cluster pull path can crash under memory pressure because the new `malloc(query_size)` result is passed to `snprintf` without a null check. If allocation fails, this loop dereferences a null pointer instead of marking the sync as failed and exiting cleanly.</violation>
</file>
<file name="lib/ClickHouse_Server.cpp">
<violation number="1" location="lib/ClickHouse_Server.cpp:864">
P2: The USER()/DATABASE() rewrite still inserts `userinfo->username` into SQL without escaping, so usernames containing quote characters can produce malformed or altered SQL in the generated query. `snprintf` prevents overflow, but it does not make the SQL literal safe. Escaping the username (or using a bound/typed value path) before formatting would keep these rewrites correct for all usernames.</violation>
<violation number="2" location="lib/ClickHouse_Server.cpp:1302">
P2: The startup `USE` query is still assembled from raw `schemaname` text, which can fail for valid names requiring quoting and can alter parsing when special characters are present. Quoting/escaping the schema identifier before building `use_query` would make first-query schema initialization more robust.</violation>
</file>
<file name="lib/MySQL_HostGroups_Manager.cpp">
<violation number="1" location="lib/MySQL_HostGroups_Manager.cpp:4346">
P2: Group-replication status updates can be silently malformed for long hostnames because the SQL buffer is sized for a query shape with one `%s`, then reused for a later query that injects the hostname twice. In this path, `snprintf` will truncate the statement when the hostname is long enough, and the truncated SQL is still passed to `mydb->execute`, which can make OFFLINE/READ_ONLY transitions fail unexpectedly. Recomputing `query_size` for the final status query (or checking `snprintf`’s return value and resizing) would keep this path reliable.</violation>
</file>
<file name="lib/MySQL_Logger.cpp">
<violation number="1" location="lib/MySQL_Logger.cpp:1394">
P2: The relative log filename buffer is sized for only an 8-digit file id. Once `log_file_id` reaches 9 digits, `snprintf` truncates the generated path, which can break log open/rotation behavior. Consider sizing from the formatted id length instead of a fixed `+11` constant.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| sprintf(query, query1, ts-interval, ts); | ||
| query_size = strlen(query1) + 128; | ||
| query = (char *)malloc(query_size); | ||
| snprintf(query, query_size, query1, ts-interval, ts); |
There was a problem hiding this comment.
P2: The metrics range queries can eventually select the wrong timestamp window because the updated snprintf calls pass time_t values into SQL templates that still use %d. %d is an int specifier, so this introduces a varargs type mismatch and risks truncated timestamps as epoch values grow. It would be safer to align the format specifiers with time_t (for example %ld with matching casts, or an explicit 64-bit format) across these query templates.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/ProxySQL_Statistics.cpp, line 565:
<comment>The metrics range queries can eventually select the wrong timestamp window because the updated `snprintf` calls pass `time_t` values into SQL templates that still use `%d`. `%d` is an `int` specifier, so this introduces a varargs type mismatch and risks truncated timestamps as epoch values grow. It would be safer to align the format specifiers with `time_t` (for example `%ld` with matching casts, or an explicit 64-bit format) across these query templates.</comment>
<file context>
@@ -552,24 +552,27 @@ SQLite3_result * ProxySQL_Statistics::get_mysql_metrics(int interval) {
- sprintf(query, query1, ts-interval, ts);
+ query_size = strlen(query1) + 128;
+ query = (char *)malloc(query_size);
+ snprintf(query, query_size, query1, ts-interval, ts);
break;
case 28800:
</file context>
| if (events.base_filename[0]=='/') { // absolute path | ||
| filen=(char *)malloc(strlen(events.base_filename)+11); | ||
| sprintf(filen,"%s.%08d",events.base_filename,events.log_file_id); | ||
| size_t filen_size = strlen(events.base_filename) + 11; |
There was a problem hiding this comment.
P2: Log rotation filename construction can silently truncate once log_file_id grows beyond 8 digits. The new snprintf calls are bounded by a fixed + 11 allocation, but %08d allows more than 8 digits for large values, so the path can be cut off without any error handling. Consider sizing from snprintf(nullptr, 0, ...) + 1 (or otherwise handling variable digit length) and checking the return value so rotation never reuses/truncates filenames.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/PgSQL_Logger.cpp, line 868:
<comment>Log rotation filename construction can silently truncate once `log_file_id` grows beyond 8 digits. The new `snprintf` calls are bounded by a fixed `+ 11` allocation, but `%08d` allows more than 8 digits for large values, so the path can be cut off without any error handling. Consider sizing from `snprintf(nullptr, 0, ...) + 1` (or otherwise handling variable digit length) and checking the return value so rotation never reuses/truncates filenames.</comment>
<file context>
@@ -865,11 +865,13 @@ void PgSQL_Logger::events_open_log_unlocked() {
if (events.base_filename[0]=='/') { // absolute path
- filen=(char *)malloc(strlen(events.base_filename)+11);
- sprintf(filen,"%s.%08d",events.base_filename,events.log_file_id);
+ size_t filen_size = strlen(events.base_filename) + 11;
+ filen=(char *)malloc(filen_size);
+ snprintf(filen, filen_size, "%s.%08d",events.base_filename,events.log_file_id);
</file context>
| char *query = (char *)malloc(strlen(q)+l+strlen(o)+64); | ||
| sprintf(query,q,row[0],row[1],row[2],o); | ||
| size_t query_size = strlen(q) + l + strlen(o) + 64; | ||
| char *query = (char *)malloc(query_size); |
There was a problem hiding this comment.
P2: The cluster pull path can crash under memory pressure because the new malloc(query_size) result is passed to snprintf without a null check. If allocation fails, this loop dereferences a null pointer instead of marking the sync as failed and exiting cleanly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/ProxySQL_Cluster.cpp, line 2880:
<comment>The cluster pull path can crash under memory pressure because the new `malloc(query_size)` result is passed to `snprintf` without a null check. If allocation fails, this loop dereferences a null pointer instead of marking the sync as failed and exiting cleanly.</comment>
<file context>
@@ -2871,8 +2876,9 @@ void ProxySQL_Cluster::pull_proxysql_servers_from_peer(const std::string& expect
- char *query = (char *)malloc(strlen(q)+l+strlen(o)+64);
- sprintf(query,q,row[0],row[1],row[2],o);
+ size_t query_size = strlen(q) + l + strlen(o) + 64;
+ char *query = (char *)malloc(query_size);
+ snprintf(query, query_size, q, row[0], row[1], row[2], o);
if (o!=row[3]) { // there was a copy
</file context>
| sprintf(use_query,"USE %s", sn); | ||
| size_t use_query_size = strlen(sn) + 8; | ||
| use_query = (char *)malloc(use_query_size); | ||
| snprintf(use_query, use_query_size, "USE %s", sn); |
There was a problem hiding this comment.
P2: The startup USE query is still assembled from raw schemaname text, which can fail for valid names requiring quoting and can alter parsing when special characters are present. Quoting/escaping the schema identifier before building use_query would make first-query schema initialization more robust.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/ClickHouse_Server.cpp, line 1302:
<comment>The startup `USE` query is still assembled from raw `schemaname` text, which can fail for valid names requiring quoting and can alter parsing when special characters are present. Quoting/escaping the schema identifier before building `use_query` would make first-query schema initialization more robust.</comment>
<file context>
@@ -1290,8 +1297,9 @@ void ClickHouse_Server_session_handler(MySQL_Session* sess, void *_pa, PtrSize_t
- sprintf(use_query,"USE %s", sn);
+ size_t use_query_size = strlen(sn) + 8;
+ use_query = (char *)malloc(use_query_size);
+ snprintf(use_query, use_query_size, "USE %s", sn);
clickhouse::Query myq(use_query);
clickhouse_sess->client->Execute(myq);
</file context>
| char *query1=(char *)"SELECT \"%s\" AS 'CURRENT_USER()'"; | ||
| size_t query2_size = strlen(query1) + strlen(sess->client_myds->myconn->userinfo->username) + 10; | ||
| char *query2=(char *)malloc(query2_size); | ||
| snprintf(query2, query2_size, query1,sess->client_myds->myconn->userinfo->username); |
There was a problem hiding this comment.
P2: The USER()/DATABASE() rewrite still inserts userinfo->username into SQL without escaping, so usernames containing quote characters can produce malformed or altered SQL in the generated query. snprintf prevents overflow, but it does not make the SQL literal safe. Escaping the username (or using a bound/typed value path) before formatting would keep these rewrites correct for all usernames.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/ClickHouse_Server.cpp, line 864:
<comment>The USER()/DATABASE() rewrite still inserts `userinfo->username` into SQL without escaping, so usernames containing quote characters can produce malformed or altered SQL in the generated query. `snprintf` prevents overflow, but it does not make the SQL literal safe. Escaping the username (or using a bound/typed value path) before formatting would keep these rewrites correct for all usernames.</comment>
<file context>
@@ -855,10 +857,11 @@ void ClickHouse_Server_session_handler(MySQL_Session* sess, void *_pa, PtrSize_t
+ char *query1=(char *)"SELECT \"%s\" AS 'CURRENT_USER()'";
+ size_t query2_size = strlen(query1) + strlen(sess->client_myds->myconn->userinfo->username) + 10;
+ char *query2=(char *)malloc(query2_size);
+ snprintf(query2, query2_size, query1,sess->client_myds->myconn->userinfo->username);
query=l_strdup(query2);
query_length=strlen(query2)+1;
</file context>
| " hostgroup_id=(SELECT offline_hostgroup FROM mysql_group_replication_hostgroups WHERE writer_hostgroup=%d)) WHEN 2 THEN 2 ELSE 0 END)" | ||
| " WHERE hostname='%s' AND port=%d AND hostgroup_id=(SELECT offline_hostgroup FROM mysql_group_replication_hostgroups WHERE writer_hostgroup=%d)"; | ||
| sprintf(query,q,_hostname,_port,_writer_hostgroup,_hostname,_port,_writer_hostgroup); | ||
| snprintf(query, query_size, q, _hostname, _port, _writer_hostgroup, _hostname, _port, _writer_hostgroup); |
There was a problem hiding this comment.
P2: Group-replication status updates can be silently malformed for long hostnames because the SQL buffer is sized for a query shape with one %s, then reused for a later query that injects the hostname twice. In this path, snprintf will truncate the statement when the hostname is long enough, and the truncated SQL is still passed to mydb->execute, which can make OFFLINE/READ_ONLY transitions fail unexpectedly. Recomputing query_size for the final status query (or checking snprintf’s return value and resizing) would keep this path reliable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/MySQL_HostGroups_Manager.cpp, line 4346:
<comment>Group-replication status updates can be silently malformed for long hostnames because the SQL buffer is sized for a query shape with one `%s`, then reused for a later query that injects the hostname twice. In this path, `snprintf` will truncate the statement when the hostname is long enough, and the truncated SQL is still passed to `mydb->execute`, which can make OFFLINE/READ_ONLY transitions fail unexpectedly. Recomputing `query_size` for the final status query (or checking `snprintf`’s return value and resizing) would keep this path reliable.</comment>
<file context>
@@ -4338,7 +4343,7 @@ void MySQL_HostGroups_Manager::update_group_replication_set_offline(char *_hostn
" hostgroup_id=(SELECT offline_hostgroup FROM mysql_group_replication_hostgroups WHERE writer_hostgroup=%d)) WHEN 2 THEN 2 ELSE 0 END)"
" WHERE hostname='%s' AND port=%d AND hostgroup_id=(SELECT offline_hostgroup FROM mysql_group_replication_hostgroups WHERE writer_hostgroup=%d)";
- sprintf(query,q,_hostname,_port,_writer_hostgroup,_hostname,_port,_writer_hostgroup);
+ snprintf(query, query_size, q, _hostname, _port, _writer_hostgroup, _hostname, _port, _writer_hostgroup);
mydb->execute(query);
//free(query);
</file context>
| } else { // relative path | ||
| filen=(char *)malloc(strlen(events.datadir)+strlen(events.base_filename)+11); | ||
| sprintf(filen,"%s/%s.%08d",events.datadir,events.base_filename,events.log_file_id); | ||
| size_t filen_size = strlen(events.datadir) + strlen(events.base_filename) + 11; |
There was a problem hiding this comment.
P2: The relative log filename buffer is sized for only an 8-digit file id. Once log_file_id reaches 9 digits, snprintf truncates the generated path, which can break log open/rotation behavior. Consider sizing from the formatted id length instead of a fixed +11 constant.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/MySQL_Logger.cpp, line 1394:
<comment>The relative log filename buffer is sized for only an 8-digit file id. Once `log_file_id` reaches 9 digits, `snprintf` truncates the generated path, which can break log open/rotation behavior. Consider sizing from the formatted id length instead of a fixed `+11` constant.</comment>
<file context>
@@ -1387,11 +1387,13 @@ void MySQL_Logger::events_open_log_unlocked() {
} else { // relative path
- filen=(char *)malloc(strlen(events.datadir)+strlen(events.base_filename)+11);
- sprintf(filen,"%s/%s.%08d",events.datadir,events.base_filename,events.log_file_id);
+ size_t filen_size = strlen(events.datadir) + strlen(events.base_filename) + 11;
+ filen=(char *)malloc(filen_size);
+ snprintf(filen, filen_size, "%s/%s.%08d",events.datadir,events.base_filename,events.log_file_id);
</file context>
|
Superseded by #6038, which preserves the independent GenAI complete-type destruction fix as a one-file, one-commit change. This PR also contained the bulk S6069 refactoring. SonarCloud reported 485 new code smells on that broader approach, so it is being restarted separately with a Sonar-validated pattern rather than merged here. |
1 participant
Summary
This branch hardens SonarCloud
cpp:S6069formatting hotspots in ProxySQL-owned code and corrects GenAI handler destruction where forward declarations previously allowed incomplete-type deletes.sprintfdestinations withsnprintfusingsizeofor the recorded allocation size.ezOptionParser.hpp, unchanged.Why
The affected calls did not communicate their destination capacity to the formatting function. Recording or deriving the existing buffer size makes the bound explicit while preserving the generated SQL, protocol output, diagnostics, and test messages.
Validation
PROXYSQL40=1 make -j16 debugSummary by cubic
Hardened all cpp:S6069 formatting paths by bounding string writes and updated GenAI MCP teardown to delete AI/RAG handlers with complete types. This reduces overflow risk and fixes destructor calls without changing runtime behavior.
sprintfwithsnprintfusingsizeofor recorded heap sizes across MySQL, PostgreSQL, ClickHouse, logging, admin, tests, and support code.ezOptionParser.hpp) untouched.AI_Tool_Handler.handRAG_Tool_Handler.hin MCP thread to ensure proper handler destruction.Written for commit 0836f1b. Summary will update on new commits.
Summary by CodeRabbit