fix: use complete GenAI handler types and bounded formatting - #6038
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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe changes add AI and RAG tool handler includes to ChangesFormatting and handler updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 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 |
The GTID connection setup passes a fixed-size service-name buffer to getaddrinfo(). Format the uint16_t port with snprintf() and the buffer's actual size instead of an unbounded sprintf() call. NI_MAXSERV is sufficient for the decimal port representation, and the bounded conversion preserves the existing connection behavior while keeping the write tied to str_port's capacity.
renecannao
changed the title
|
Added |
PgSQL_Thread.cpp contains 49 cpp:S6069 reports. Convert the 47 calls that write to fixed local arrays to snprintf() with each destination's sizeof(), covering configuration names, status values, process-list fields, and audit messages. Leave the two dynamically allocated destinations unchanged: their allocation formulas need separate capacity reasoning. This commit is intentionally restricted to literal formats and statically sized buffers, the pattern SonarCloud accepted in the preceding pilot.
renecannao
changed the title
|
Added |
![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
🤖 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_Thread.cpp`:
- Line 1421: Update the backend-address construction near the pgsql_servers
handling to avoid fixed-buffer truncation before strdup(): copy
mc->parent->address directly, or validate snprintf()’s return value and handle
oversized results without truncation. Preserve the full hostname/address for
inputs exceeding 1023 bytes.
- Line 2634: Update the formatting call in the surrounding update-value
serialization logic to use type-correct format specifiers: format the unsigned
int value with %u, and format last_updated using a conversion matching time_t on
the target platform or cast it to a fixed-width integer type before using the
corresponding specifier.
🪄 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: 8413856d-23a0-4db8-8747-d08fba2e3009
📒 Files selected for processing (1)
lib/PgSQL_Thread.cpp
📜 Review details
🧰 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/PgSQL_Thread.cpp
🪛 Cppcheck (2.21.0)
lib/PgSQL_Thread.cpp
[warning] 2633-2633: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4506-4506: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4511-4511: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4513-4513: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4517-4517: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4519-4519: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4523-4523: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4638-4638: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4642-4642: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4656-4656: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4660-4660: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4706-4706: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4710-4710: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4712-4712: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4716-4716: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4718-4718: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4722-4722: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4724-4724: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4728-4728: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4730-4730: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4734-4734: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4736-4736: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4740-4740: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4742-4742: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4746-4746: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4748-4748: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4752-4752: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4754-4754: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4758-4758: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 5162-5162: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🔇 Additional comments (1)
lib/PgSQL_Thread.cpp (1)
1488-1488: LGTM!Also applies to: 1624-1628, 2632-2633, 2795-2795, 3904-3904, 3919-3919, 3934-3934, 4431-4431, 4505-4524, 4637-4643, 4655-4661, 4705-4759, 5163-5163, 5185-5193, 5206-5206, 5221-5229, 5245-5251, 5331-5331, 5343-5346
SQL3_getStats() writes cache counters into a fixed 256-byte buffer before handing the strings to SQLite result rows. Replace each unbounded sprintf() call with snprintf() using the buffer's actual capacity. The values remain the same numeric representations, while the write now stays within the local status buffer even if its declaration changes in the future.
PS_global_stats::get_row() serializes statement identifiers, digest values, and reference counters through a fixed 128-byte buffer. Use snprintf() with sizeof(buf) for the six reported conversions. This keeps the existing result-row representation and ensures every temporary numeric string is bounded by the storage that owns it.
ProxySQL_messages_stats::get_row() formats message identifiers and timing counters through a fixed 128-byte buffer. Bound each reported conversion with snprintf() and sizeof(buf). The generated statistics rows are unchanged for valid values, while formatting cannot exceed the temporary row buffer.
Connection attributes and JSON diagnostics format hostgroup identifiers and pointer addresses into fixed local buffers. Replace the three reported sprintf() calls with capacity-aware snprintf() calls. The output values preserve their current representation while remaining constrained by __buffer and buff at each call site.
MySQL query-rule rendering formats 64-bit digests into fixed 20-byte local buffers. Use snprintf() with sizeof(buf) at both current Sonar-reported sites. The hexadecimal digest text keeps its existing 0x-prefixed format and the formatting operation is now explicitly limited to the buffer capacity.
PostgreSQL query-rule rendering uses fixed 20-byte buffers for 0x-prefixed 64-bit digest strings. Replace both unbounded sprintf() calls with snprintf() using sizeof(buf). The serialized digest format is unchanged, and the conversion now records the storage bound at the write site.
QP_query_digest_stats writes digest and hostgroup values into fixed arrays owned by query_digest_stats_pointers_t. Use snprintf() with the sizes of qdsp->digest and qdsp->hid. This retains the result-row values while ensuring both conversions remain within their dedicated 24-byte fields.
MySQL_ResultSet::add_err() copies the server SQLSTATE into a fixed local array before creating an error packet. Format it with snprintf() and sizeof(sqlstate) instead of an unbounded sprintf(). The destination remains the same ten-byte SQLSTATE buffer, and packet construction continues to receive the existing text value.
proxy_coredump_generate() constructs the generated core filename in a fixed 128-byte local buffer. Replace sprintf() with snprintf() using the buffer's capacity. The core.<pid>.<counter> naming scheme is preserved while filename generation can no longer overrun its local storage.
MySQL_Thread.cpp has 52 cpp:S6069 reports. Convert the 50 calls that target fixed local arrays to snprintf() with each destination's sizeof(), covering configuration, status, process-list, and audit formatting. Leave the two dynamically allocated destinations unchanged because their allocation formulas need separate capacity analysis. This commit stays within the literal-format, fixed-buffer pattern already accepted by SonarCloud.
|
Added ten file-specific S6069 commits ( |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 10 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Process-list rows previously copied each backend address through a fixed 1024-byte buffer before duplicating it. Duplicate the owned address directly so valid server names longer than that temporary buffer are retained without truncation. Also align the two adjacent numeric conversions with their declared types: render the thread index as unsigned and convert time_t through an explicit long long representation. The remaining bounded conversions continue to use snprintf with the destination capacity.
Process-list rows previously copied each backend address through a fixed 1024-byte buffer before duplicating it. Duplicate the owned address directly so valid server names longer than that temporary buffer are retained without truncation. Also align the two adjacent numeric conversions with their declared types: render the thread index as unsigned and convert time_t through an explicit long long representation. The remaining bounded conversions continue to use snprintf with the destination capacity.
The signed hostgroup conversion must use formatted output because my_itoa loses the sign, as documented by issue #2285. The code already uses bounded snprintf, but its comment still referred to the earlier sprintf implementation. Describe the required conversion behavior rather than a retired API name so the rationale remains accurate without changing runtime behavior.
The HTTP status page renders uptime and worker counts into fixed local arrays. Pass the actual capacity of each destination to snprintf so those values cannot write beyond their presentation buffers. The output format is unchanged and the buffers remain comfortably sized for their bounded numeric representations. This change intentionally covers only the fixed-array formatting reported by SonarCloud.
Configuration group names are allocated from the measured prefix and the _variables suffix. Preserve that exact allocation size in a named value and pass it to snprintf while constructing the group name. The name and allocation policy are unchanged; the conversion only makes the existing capacity explicit to the formatter and static analysis.
The administrative digest generator formats synthetic queries into its dedicated 1024-byte work buffer. Use that known capacity when emitting each test query. The generated query template and test coverage remain unchanged; this only preserves the buffer boundary during test-data construction.
The temporary statistics response has a fixed 1000-byte backing array. Supply its capacity to the formatter when generating the uptime and query counters. The protocol text and packet construction are unchanged, while formatting now has an explicit destination limit.
SHA-1 bytes are rendered as two hexadecimal characters into a pre-sized output buffer. Limit every two-character write to its three-byte character-and-terminator window. The loop retains its original offsets and output layout, including the final terminator reserved by the allocation, while making each write boundary explicit.
Checksum and plugin SHA-1 paths render binary values into fixed local hexadecimal buffers. Give those formatters the full buffer size or the three-byte window for each encoded byte. The byte order, loop limits, and checksum data passed to SQLite remain unchanged; only the destination limits are made explicit.
Firewall diagnostics print either a fixed-width digest or the unknown marker into a 32-byte local array. Use the array capacity for both output paths. This retains the warning text and digest representation exactly while preventing formatting from exceeding its diagnostic buffer.
ClickHouse compatibility responses render a thread session identifier and configured port into fixed local buffers. Pass each buffer capacity to the formatter. The response values and configuration API stay unchanged, while their numeric conversions gain explicit bounds.
PostgreSQL authentication encodes digest bytes into fixed slices of a local buffer, and OK packet generation formats known command tags into a 128-byte array. Limit the hexadecimal writes to their three-byte slices and use the complete array size for tags. Authentication bytes, PostgreSQL command tags, and packet semantics remain unchanged; the conversion makes each already-known destination boundary explicit.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/mysql_connection.cpp`:
- Line 3267: Update the snprintf call around the mysql pointer formatting to
pass mysql as static_cast<void*> explicitly for the %p conversion, and apply the
same cast to this wherever it is formatted with %p in the surrounding code.
In `@lib/MySQL_Thread.cpp`:
- Line 2077: Update the snprintf formatting of variables.server_capabilities to
use an unsigned 32-bit format specifier, such as PRIu32, so values with the high
bit set are represented correctly; include the required format macro header if
needed.
🪄 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: a49fe6ce-4aa5-49d3-bce9-2d3070dd7e52
📒 Files selected for processing (11)
lib/MySQL_PreparedStatement.cpplib/MySQL_Query_Processor.cpplib/MySQL_ResultSet.cpplib/MySQL_Thread.cpplib/PgSQL_Query_Processor.cpplib/PgSQL_Thread.cpplib/QP_query_digest_stats.cpplib/Query_Cache.cpplib/debug.cpplib/mysql_connection.cpplib/proxysql_coredump.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/PgSQL_Thread.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: cubic · AI code reviewer
🧰 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/MySQL_Query_Processor.cpplib/proxysql_coredump.cpplib/MySQL_ResultSet.cpplib/QP_query_digest_stats.cpplib/PgSQL_Query_Processor.cpplib/debug.cpplib/MySQL_PreparedStatement.cpplib/mysql_connection.cpplib/Query_Cache.cpplib/MySQL_Thread.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_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] 3289-3289: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 5417-5417: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 5419-5419: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 5423-5423: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 5425-5425: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 5429-5429: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 5412-5412: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 6038-6038: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🔇 Additional comments (10)
lib/MySQL_PreparedStatement.cpp (1)
1007-1025: LGTM!lib/MySQL_Query_Processor.cpp (1)
97-97: LGTM!Also applies to: 834-834
lib/MySQL_ResultSet.cpp (1)
450-450: LGTM!lib/PgSQL_Query_Processor.cpp (1)
192-192: LGTM!Also applies to: 452-452
lib/Query_Cache.cpp (1)
658-700: LGTM!lib/debug.cpp (1)
383-393: LGTM!lib/MySQL_Thread.cpp (1)
1873-1873: LGTM!Also applies to: 1989-1989, 2139-2143, 2470-2470, 3288-3290, 3466-3466, 4709-4709, 4824-4824, 4838-4838, 4998-4998, 5340-5340, 5411-5430, 5516-5516, 5533-5533, 5547-5547, 5559-5565, 5590-5650, 6037-6039, 6060-6079, 6092-6092, 6109-6117, 6131-6135, 6211-6211, 6225-6225, 6237-6240
lib/mysql_connection.cpp (1)
814-814: LGTM!lib/proxysql_coredump.cpp (1)
75-75: LGTM!lib/QP_query_digest_stats.cpp (1)
164-164: 🗄️ Data Integrity & IntegrationNo change required for these
snprintfcalls.digestandhidare fixedchar[24]arrays, sosizeof(...)provides their actual capacities.> Likely an incorrect or invalid review comment.
The %p conversion requires a void pointer argument, but the JSON diagnostics passed typed MYSQL and MySQL_Connection pointers through the variadic formatter. Cast both explicitly to void* before formatting. The emitted addresses stay identical on current platforms while the calls become standards-conforming and portable across pointer representations.
The server_capabilities variable is uint32_t, yet the administrative getter rendered it with the signed %d conversion. Values with the high capability bit set could therefore be exposed as negative numbers. Use PRIu32 with the matching header so the formatting follows the declared width and unsignedness without relying on a platform-specific underlying type.
Administrative stress helpers build synthetic host names and status replies in fixed 32-byte and 256-byte arrays. Use the corresponding capacity for every selected hostname, timing, and monitor-state conversion. The generated values are unchanged; the certificate duration is also rendered through an explicit unsigned long long representation so its uint64_t type is not tied to the platform's unsigned long format.
PostgreSQL error rows, connection-pool snapshots, and hostgroup diagnostics format fixed numeric and pointer values into local arrays. Use each destination capacity and pass explicit void-pointer types for address rendering. The underlying metrics and row contents are unchanged. Timestamps are converted through long long explicitly so their time_t representation is not assumed while reporting them.
Session diagnostics serialize addresses, SQLSTATE placeholders, hostgroup errors, and fallback client names through fixed local buffers. Route those paths through capacity-aware snprintf and use const void pointers for read-only backend connection addresses. The JSON and error-packet values retain their established representation while their formatting and pointer constness are made explicit.
Both Galera monitoring execution paths render wsrep_local_state into fixed 80-byte diagnostic buffers. Supply the capacity in each conversion before reporting a server offline. The state text and health-check decisions remain unchanged while the monitor message construction becomes bounded.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/sqlite3db.cpp`:
- Line 1132: Update the plugin checksum formatting loop around the snprintf call
to iterate while i < SHA_DIGEST_LENGTH, ensuring all 20 SHA-1 bytes produce 40
hexadecimal digits. Increase the destination buf allocation by one byte to
accommodate the terminating NUL character.
🪄 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: 75138907-7b99-4acd-b1d1-5ffcaef02cca
📒 Files selected for processing (9)
lib/ClickHouse_Server.cpplib/MySQL_Protocol.cpplib/MySQL_encode.cpplib/PgSQL_Protocol.cpplib/ProxySQL_Admin_Tests.cpplib/ProxySQL_Config.cpplib/ProxySQL_HTTP_Server.cpplib/Query_Processor.cpplib/sqlite3db.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: cubic · AI code reviewer
🧰 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/ProxySQL_Admin_Tests.cpplib/ProxySQL_Config.cpplib/ClickHouse_Server.cpplib/Query_Processor.cpplib/PgSQL_Protocol.cpplib/MySQL_Protocol.cpplib/ProxySQL_HTTP_Server.cpplib/sqlite3db.cpplib/MySQL_encode.cpp
🪛 Cppcheck (2.21.0)
lib/ProxySQL_Config.cpp
[warning] 158-158: 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)
🔇 Additional comments (9)
lib/ClickHouse_Server.cpp (1)
984-984: LGTM!Also applies to: 1783-1783
lib/MySQL_Protocol.cpp (1)
291-291: LGTM!lib/MySQL_encode.cpp (1)
42-42: LGTM!Also applies to: 246-247
lib/PgSQL_Protocol.cpp (1)
994-994: LGTM!Also applies to: 1004-1004, 1659-1659, 1668-1668
lib/ProxySQL_Admin_Tests.cpp (1)
108-108: LGTM!lib/ProxySQL_Config.cpp (1)
156-158: LGTM!lib/ProxySQL_HTTP_Server.cpp (1)
147-147: LGTM!Also applies to: 169-169
lib/Query_Processor.cpp (1)
2195-2197: LGTM!lib/sqlite3db.cpp (1)
687-687: LGTM!Also applies to: 1253-1253
Plugin loading converted only 19 of the 20 SHA-1 bytes to hexadecimal, then copied an uninitialized final byte into the checksum buffer. This produced an incomplete checksum whenever plugin verification was enabled. Allocate room for all 40 hexadecimal characters plus the terminator, encode every digest byte, and copy the complete string into the plugin checksum buffer.
Internal-session JSON, SQLSTATE propagation, localhost fallbacks, and protocol replies render into fixed local buffers. Pass each buffer capacity to snprintf and use explicit void-pointer conversions for address serialization. The produced JSON fields, error packets, and numeric responses retain their existing representations. Allocation-backed query and error-string construction is intentionally unchanged because its capacity derives from runtime input rather than a fixed local buffer.
Connection-pool rows, GTID statistics, error statistics, and verbose hostgroup diagnostics write to fixed local buffers. Supply each buffer capacity to snprintf and serialize diagnostic pointers through explicit void-pointer conversions. The generated result-set fields and diagnostic values keep their existing formats. Dynamically allocated SQL statement construction is deliberately excluded, because truncating those runtime-sized queries would change database behavior.
Cluster checksum, health-metric, and node-table rows render port, version, timestamp, weight, and counter values through fixed local buffers. Use snprintf with the declared 32-byte capacity for each of these result-set fields. The database-facing cluster configuration strings continue using their existing dynamically allocated paths and are intentionally excluded. This preserves query construction while hardening the independent fixed-buffer reporting code.
Administrative control commands, generated count queries, and checksum responses format fixed local buffers. Use snprintf consistently and serialize the uint64_t global checksum through an explicit unsigned long long conversion. This preserves the external admin response values while removing the platform-dependent checksum format assumption. Dynamically allocated queries and runtime error text remain unchanged to avoid silently truncating caller-controlled content.
Hourly CPU, memory, connection-pool, and MySQL connection aggregation queries format only timestamp fields into fixed 256-, 512-, and 1024-byte buffers. Use snprintf for the selected static query templates. Worst-case signed 64-bit timestamp expansions are 203/256, 264/512, 330/1024, and 550/1024 bytes respectively, so the generated SQL remains complete. Dynamically allocated query construction remains unchanged.
MySQL event logging renders fixed-width timestamps, durations, and 64-bit hexadecimal query digests through local 20-, 36-, and 64-byte arrays. Pass each destination capacity to snprintf in JSON generation and SQLite event persistence. The timestamp layout and digest representation remain unchanged. Filename and network-address formatting use dynamically allocated destinations and are intentionally excluded from this fixed-buffer commit.
PostgreSQL event logging renders fixed-width timestamps, durations, and 64-bit hexadecimal query digests through local 20-, 36-, and 64-byte arrays. Use snprintf with each destination capacity in JSON generation and SQLite event persistence. Timestamp and digest output formats are preserved. File names and connection addresses continue using dynamically allocated strings and are deliberately left outside this fixed-buffer hardening change.
SQL3_GlobalStatus renders the three MySQL monitor DNS-cache counters through its fixed 256-byte local buffer. Replace their remaining sprintf calls with snprintf calls that state that buffer capacity explicitly. The status variable names, unsigned 64-bit counter formatting, and result-row ownership are unchanged. This clears the remaining SonarCloud cpp:S6069 findings in MySQL_Thread.cpp.
The prior #6038 CI-builds run used the cleanup from #6040, which invoked the unavailable standalone docker-compose executable. Its Debian build completed but failed in post-build cleanup, and the Ubuntu 24 leg failed during pre-build cleanup. PR #6041 is now merged into GH-Actions and switches both cleanup loops to Docker Compose v2. This empty commit changes no ProxySQL source; it triggers #6038 again so the corrected reusable workflow is exercised on the self-hosted runners.
Code Review ✅ ApprovedIncludes complete GenAI handler types for proper destructor invocation during thread teardown and hardens buffer formatting project-wide using snprintf. 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 |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3.0 #6038 +/- ##
==========================================
- Coverage 53.34% 53.33% -0.01%
==========================================
Files 492 492
Lines 146700 146699 -1
Branches 37082 37080 -2
==========================================
- Hits 78256 78245 -11
+ Misses 51227 51222 -5
- Partials 17217 17232 +15
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:
|
1 participant
Summary
Include the concrete AI and RAG tool-handler definitions in
MCP_Thread.cpp, the translation unit that owns and deletes those handlers.Why
MCP_Threads_Handlerpreviously deleted forward-declared handler types. C++ requires the complete type at a delete expression so the destructor and any class-specific deallocation function are correctly invoked.Impact
GenAI MCP thread teardown now performs complete destruction of the owned AI and RAG handlers. The change is limited to the necessary includes; it does not alter handler ownership or runtime control flow.
Validation
PROXYSQL40=1 make -j16 debugSummary by cubic
Includes concrete
AI_Tool_HandlerandRAG_Tool_Handlerinplugins/genai/src/MCP_Thread.cppso owned handlers are destroyed with complete types. Replaces sprintf with snprintf across core paths, tightens pointer/format usage, and completes the hardening by bounding MySQL DNS-cache status strings.GLOBAL_CHECKSUM()as unsigned long long.void*for%pin MySQL/PgSQL connections and sessions.PRIu32forserver_capabilities.MySQL_Thread.cpp.Written for commit 62c9375. Summary will update on new commits.
Summary by CodeRabbit