Fix compiler warnings and migrate prepare_v2 SIMPLE cases to RAII #5357
+407
−570
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix use-after-free vulnerability where the function frees the 'arg' pointer at line 2306 but then returns it at line 2323. Change the return value to NULL instead of the freed pointer. This fixes a -Wuse-after-free compiler warning and prevents potential memory corruption. Found during compiler warning analysis with 'make debug'.
Fix two HIGH priority signed/unsigned comparison warnings: 1. MySQL_HostGroups_Manager.cpp:1792 - Change loop variable from 'int j' to 'unsigned int j' to match the unsigned comparison with servers->len. 2. MySQL_Thread.cpp:4357 - Cast int to size_t when comparing with getBufferSize() which returns size_t. These changes prevent potential logic errors from negative values being interpreted as large unsigned values. Found during compiler warning analysis with 'make debug'.
1. Fix write-strings warnings (12 occurrences):
- MySQL_HostGroups_Manager.cpp: Change char* to const char*
for src_status and dst_status variables
- MySQL_Monitor.cpp: Change ConsumerThread constructor
parameter from char thread_name[16] to const char *thread_name
2. Fix unused-but-set-variable warnings (6 occurrences) by
wrapping PROXYSQLGENAI-specific variables in #ifdef blocks:
- stats_mcp_query_tools_counters
- stats_mcp_query_tools_counters_reset
- stats_mcp_query_digest
- stats_mcp_query_digest_reset
- stats_mcp_query_rules
- runtime_mcp_query_rules
These variables were assigned values but appeared unused when
PROXYSQLGENAI was not defined. Now they are only defined and
assigned when PROXYSQLGENAI is enabled.
This eliminates 18 compiler warnings: 12 write-strings and 6
unused-but-set-variable warnings.
Found during compiler warning analysis with 'make debug'.
Migrate 11 functions in MySQL_HostGroups_Manager.cpp from the deprecated prepare_v2() API to the new RAII-based API. Uses raw pointer aliases to avoid .get() verbosity throughout the code. Changes per function: - servers_add() - All mysql_* table generation functions - Group replication, Galera, AWS Aurora hostgroups functions Pattern used: auto [rc, statement_unique] = mydb->prepare_v2(query); sqlite3_stmt *statement = statement_unique.get(); // ... use statement directly, no .get() needed ... // No manual finalize - RAII handles it Benefits: - Automatic cleanup via RAII (no manual finalize needed) - Exception-safe cleanup on early returns - Reduced memory leak risk This addresses ~11 of the ~150 deprecated prepare_v2 warnings. Found during compiler warning analysis with 'make debug'.
Migrate insertMysqlEventsIntoDb() function from deprecated prepare_v2() to the new RAII-based API using raw pointer aliases. Pattern used: auto [rc, statement_unique] = db->prepare_v2(query); sqlite3_stmt *statement = statement_unique.get(); // ... use statement directly ... // No manual finalize - RAII handles it This addresses 2 of the ~150 deprecated prepare_v2 warnings.
Migrate 4 functions from deprecated prepare_v2() to the new RAII-based API: - flush_pgsql_variables___runtime_to_database() - flush_genai_variables___runtime_to_database() - flush_mysql_variables___runtime_to_database() - flush_mcp_variables___runtime_to_database() All early returns happen before prepare_v2(), making these simple cases. Uses raw pointer aliases to avoid .get() verbosity. Pattern used: auto [rc, statement_unique] = db->prepare_v2(query); sqlite3_stmt *statement = statement_unique.get(); // ... use statement directly ... // No manual finalize - RAII handles it This addresses 4 of the ~150 deprecated prepare_v2 warnings.
Migrate 2 test functions from deprecated prepare_v2() to the new RAII-based API: - ProxySQL_Test___GenerateRandom_mysql_query_rules_fast_routing() - ProxySQL_Test___MySQL_HostGroups_Manager_generate_many_clusters() Both are test functions with clear structure - simple cases. Uses raw pointer aliases to avoid .get() verbosity. Pattern used: auto [rc, statement_unique] = admindb->prepare_v2(query); sqlite3_stmt *statement = statement_unique.get(); // ... use statement directly ... // No manual finalize - RAII handles it This addresses 3 of the ~150 deprecated prepare_v2 warnings.
- Changed from raw pointer to smart pointer-based prepare_v2 - Use raw pointer aliases to avoid .get() verbosity - Remove manual finalize - RAII handles cleanup automatically
… prepare_v2 - Changed from raw pointer to smart pointer-based prepare_v2 - Fixes existing memory leak where statement was never finalized - RAII ensures automatic cleanup when function exits
|
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:
📝 WalkthroughWalkthroughRefactors prepared-statement handling to return (rc, unique_statement) via structured bindings across multiple modules, replacing manual Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @renecannao, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves code quality and stability by addressing various compiler warnings and a critical use-after-free vulnerability. It modernizes SQLite statement handling through a comprehensive migration to a RAII-based API, ensuring automatic resource cleanup and preventing memory leaks. These changes contribute to a more robust and maintainable codebase. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent pull request that addresses several important issues. The fix for the use-after-free vulnerability in child_telnet is critical and correctly implemented. The changes to fix compiler warnings, such as signed/unsigned comparisons and -Wwrite-strings, improve code quality and correctness. The migration of prepare_v2 calls to a modern, RAII-based API is a significant improvement in terms of code safety and readability, and it's great that this migration also fixed a memory leak in generate_pgsql_hostgroup_attributes_table. The code is well-structured and the changes are implemented consistently across multiple files. I have no further suggestions for improvement. Great work!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@lib/Admin_FlushVariables.cpp`:
- Around line 1141-1151: The function
flush_mysql_variables___runtime_to_database contains a use-after-free:
statement2_unique is a local unique_ptr created inside the if (runtime) block
and goes out of scope, leaving statement2 dangling when later used; fix by
extending the lifetime of the prepared statement by moving the declaration of
statement2_unique (the unique_ptr returned by db->prepare_v2) to the outer scope
(same scope as statement1_unique) or by storing the unique_ptr in a member/local
variable that survives until after the later uses (ensure statement2 is obtained
from that persistent unique_ptr), keeping the ASSERT_SQLITE_OK check for rc2 and
not creating a raw dangling sqlite3_stmt* from a freed owner.
- Around line 1008-1017: The raw pointer statement2 is left dangling because
statement2_unique is declared inside the if (runtime) block in
flush_genai_variables___runtime_to_database; move the unique_ptr that owns
statement2 to function scope (declare e.g. sqlite3_stmt_unique_ptr
statement2_unique; alongside the existing statement1_unique), then inside the if
assign it from db->prepare_v2(b) and call ASSERT_SQLITE_OK on the returned rc
before setting statement2 = statement2_unique.get(); this ensures the owner
outlives the block and prevents the use-after-free when statement2 is later
used.
- Around line 336-345: The raw pointer statement2 is extracted from a unique_ptr
that is declared inside the if (runtime) block and gets destroyed at block exit,
causing a use-after-free; fix by moving/hoisting the unique_ptr that owns the
sqlite3_stmt (the variable currently declared as statement2_unique) to the
surrounding function scope (declare it before the if), then inside the if only
assign to that unique_ptr (do not re-declare a new local unique_ptr), keep
setting statement2 = statement2_unique.get() and leave ASSERT_SQLITE_OK(rc2, db)
as-is so the owning unique_ptr outlives later uses and finalizes only when the
function scope ends.
Comment on lines
+336
to
+345
| auto [rc1, statement1_unique] = db->prepare_v2(a); | ||
| ASSERT_SQLITE_OK(rc1, db); | ||
| sqlite3_stmt* statement1 = statement1_unique.get(); | ||
| sqlite3_stmt* statement2 = NULL; | ||
| if (runtime) { | ||
| db->execute("DELETE FROM runtime_global_variables WHERE variable_name LIKE 'pgsql-%'"); | ||
| b = (char*)"INSERT INTO runtime_global_variables(variable_name, variable_value) VALUES(?1, ?2)"; | ||
| //rc=(*proxy_sqlite3_prepare_v2)(mydb3, b, -1, &statement2, 0); | ||
| rc = db->prepare_v2(b, &statement2); | ||
| ASSERT_SQLITE_OK(rc, db); | ||
| auto [rc2, statement2_unique] = db->prepare_v2(b); | ||
| ASSERT_SQLITE_OK(rc2, db); | ||
| statement2 = statement2_unique.get(); |
There was a problem hiding this comment.
Critical: statement2 becomes a dangling pointer — use-after-free.
statement2_unique is declared inside the if (runtime) block (line 343), so it is destroyed when the block exits. However, statement2 (the raw pointer extracted at line 345) is used later at lines 362–366, which is a use-after-free. The sqlite3_finalize is called by the unique_ptr destructor at the closing brace of the if block, invalidating the statement handle.
🐛 Proposed fix: hoist unique_ptr to function scope
One approach is to declare the unique_ptr at function scope so it outlives all usages:
auto [rc1, statement1_unique] = db->prepare_v2(a);
ASSERT_SQLITE_OK(rc1, db);
sqlite3_stmt* statement1 = statement1_unique.get();
- sqlite3_stmt* statement2 = NULL;
+ sqlite3_stmt* statement2 = nullptr;
+ // Declare at function scope so the unique_ptr outlives statement2 usage
+ decltype(statement1_unique) statement2_unique;
if (runtime) {
db->execute("DELETE FROM runtime_global_variables WHERE variable_name LIKE 'pgsql-%'");
b = (char*)"INSERT INTO runtime_global_variables(variable_name, variable_value) VALUES(?1, ?2)";
- auto [rc2, statement2_unique] = db->prepare_v2(b);
+ auto [rc2, stmt2_tmp] = db->prepare_v2(b);
ASSERT_SQLITE_OK(rc2, db);
- statement2 = statement2_unique.get();
+ statement2_unique = std::move(stmt2_tmp);
+ statement2 = statement2_unique.get();
}🤖 Prompt for AI Agents
In `@lib/Admin_FlushVariables.cpp` around lines 336 - 345, The raw pointer
statement2 is extracted from a unique_ptr that is declared inside the if
(runtime) block and gets destroyed at block exit, causing a use-after-free; fix
by moving/hoisting the unique_ptr that owns the sqlite3_stmt (the variable
currently declared as statement2_unique) to the surrounding function scope
(declare it before the if), then inside the if only assign to that unique_ptr
(do not re-declare a new local unique_ptr), keep setting statement2 =
statement2_unique.get() and leave ASSERT_SQLITE_OK(rc2, db) as-is so the owning
unique_ptr outlives later uses and finalizes only when the function scope ends.
Comment on lines
+1008
to
+1017
| auto [rc1, statement1_unique] = db->prepare_v2(a); | ||
| ASSERT_SQLITE_OK(rc1, db); | ||
| sqlite3_stmt* statement1 = statement1_unique.get(); | ||
| sqlite3_stmt* statement2 = NULL; | ||
| rc = db->prepare_v2(a, &statement1); | ||
| ASSERT_SQLITE_OK(rc, db); | ||
| if (runtime) { | ||
| db->execute("DELETE FROM runtime_global_variables WHERE variable_name LIKE 'genai-%'"); | ||
| b = (char*)"INSERT INTO runtime_global_variables(variable_name, variable_value) VALUES(?1, ?2)"; | ||
| rc = db->prepare_v2(b, &statement2); | ||
| ASSERT_SQLITE_OK(rc, db); | ||
| auto [rc2, statement2_unique] = db->prepare_v2(b); | ||
| ASSERT_SQLITE_OK(rc2, db); | ||
| statement2 = statement2_unique.get(); |
There was a problem hiding this comment.
Critical: Same use-after-free bug in flush_genai_variables___runtime_to_database.
statement2_unique (line 1015) goes out of scope at the end of the if (runtime) block, but the raw statement2 pointer is used at lines 1033–1038. Same fix needed: hoist the unique_ptr to function scope.
🤖 Prompt for AI Agents
In `@lib/Admin_FlushVariables.cpp` around lines 1008 - 1017, The raw pointer
statement2 is left dangling because statement2_unique is declared inside the if
(runtime) block in flush_genai_variables___runtime_to_database; move the
unique_ptr that owns statement2 to function scope (declare e.g.
sqlite3_stmt_unique_ptr statement2_unique; alongside the existing
statement1_unique), then inside the if assign it from db->prepare_v2(b) and call
ASSERT_SQLITE_OK on the returned rc before setting statement2 =
statement2_unique.get(); this ensures the owner outlives the block and prevents
the use-after-free when statement2 is later used.
Comment on lines
+1141
to
+1151
| auto [rc1, statement1_unique] = db->prepare_v2(a); | ||
| ASSERT_SQLITE_OK(rc1, db); | ||
| sqlite3_stmt *statement1 = statement1_unique.get(); | ||
| sqlite3_stmt *statement2 = NULL; | ||
| if (runtime) { | ||
| db->execute("DELETE FROM runtime_global_variables WHERE variable_name LIKE 'mysql-%'"); | ||
| b=(char *)"INSERT INTO runtime_global_variables(variable_name, variable_value) VALUES(?1, ?2)"; | ||
|
|
||
| rc=db->prepare_v2(b, &statement2); | ||
| ASSERT_SQLITE_OK(rc, db); | ||
| auto [rc2, statement2_unique] = db->prepare_v2(b); | ||
| ASSERT_SQLITE_OK(rc2, db); | ||
| statement2 = statement2_unique.get(); |
There was a problem hiding this comment.
Critical: Same use-after-free bug in flush_mysql_variables___runtime_to_database.
statement2_unique (line 1149) goes out of scope at the end of the if (runtime) block, but statement2 is used at lines 1167–1172. Same dangling pointer issue.
🤖 Prompt for AI Agents
In `@lib/Admin_FlushVariables.cpp` around lines 1141 - 1151, The function
flush_mysql_variables___runtime_to_database contains a use-after-free:
statement2_unique is a local unique_ptr created inside the if (runtime) block
and goes out of scope, leaving statement2 dangling when later used; fix by
extending the lifetime of the prepared statement by moving the declaration of
statement2_unique (the unique_ptr returned by db->prepare_v2) to the outer scope
(same scope as statement1_unique) or by storing the unique_ptr in a member/local
variable that survives until after the later uses (ensure statement2 is obtained
from that persistent unique_ptr), keeping the ASSERT_SQLITE_OK check for rc2 and
not creating a raw dangling sqlite3_stmt* from a freed owner.
…nitor Migrated the following functions: - update_monitor_mysql_servers() - update_monitor_proxysql_servers() - monitor_connect_thread() - monitor_ping_thread() - monitor_read_only_thread() - monitor_replication_lag_thread() - monitor_connect() cleanup - monitor_ping() cleanup - monitor_read_only() cleanup - monitor_replication_lag() cleanup - populate_monitor_mysql_server_group_replication_log() - populate_monitor_mysql_server_galera_log() All statements are now automatically managed by RAII smart pointers, eliminating manual finalize calls and potential memory leaks.
Migrated the following functions: - populate_monitor_mysql_server_aws_aurora_log() - populate_monitor_mysql_server_aws_aurora_check_status() - monitor_ping_process_ready_task_thread() - monitor_read_only_process_ready_tasks() (with if/else branches for replication_lag) All statements are now automatically managed by RAII smart pointers, eliminating manual finalize calls and potential memory leaks.
…_Admin Migrated the following functions: - save_mysql_query_rules_fast_routing_from_runtime() - save_pgsql_query_rules_fast_routing_from_runtime() - save_mysql_firewall_whitelist_sqli_fingerprints_from_runtime() - save_pgsql_firewall_whitelist_sqli_fingerprints_from_runtime() - save_mysql_firewall_whitelist_users_from_runtime() - save_pgsql_firewall_whitelist_users_from_runtime() All statements are now automatically managed by RAII smart pointers, eliminating manual finalize calls and potential memory leaks.
Migrated: - save_mysql_firewall_whitelist_rules_from_runtime() - save_pgsql_firewall_whitelist_rules_from_runtime() All statements are now automatically managed by RAII smart pointers.
Migrated: - dump_checksums_values_table() - save_mysql_users_runtime_to_database() All statements are now automatically managed by RAII smart pointers.
Migrated: - save_pgsql_users_runtime_to_database() - save_mysql_ldap_mapping_runtime_to_database() All statements are now automatically managed by RAII smart pointers.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/ProxySQL_Admin.cpp (2)
6286-6300:⚠️ Potential issue | 🔴 CriticalFix duplicate
rcdeclaration (compile error).
rcis already declared earlier in this scope (Line 6286); redeclaring it at Line 6299 will fail to compile.Suggested fix
auto [rc1, statement1_unique] = admindb->prepare_v2(q); ASSERT_SQLITE_OK(rc1, admindb); sqlite3_stmt *statement1 = statement1_unique.get(); - int rc;
6421-6448:⚠️ Potential issue | 🔴 CriticalFix duplicate
rcdeclaration (compile error).
rcis already declared at Line 6421 and is re-declared at Line 6448 in the same scope.Suggested fix
auto [rc1, f_statement1_unique] = admindb->prepare_v2(q_stmt1_f); ASSERT_SQLITE_OK(rc1, admindb); auto [rc2, b_statement1_unique] = admindb->prepare_v2(q_stmt1_b); ASSERT_SQLITE_OK(rc2, admindb); sqlite3_stmt *f_statement1 = f_statement1_unique.get(); sqlite3_stmt *b_statement1 = b_statement1_unique.get(); - int rc;
Migrated: - save_pgsql_ldap_mapping_runtime_to_database() - save_mysql_servers_runtime_to_database() All statements are now automatically managed by RAII smart pointers.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@lib/ProxySQL_Admin.cpp`:
- Around line 6296-6299: The duplicate declaration of rc after preparing the
statement causes a compile error; remove the redundant "int rc;" line so the
code uses the existing rc variable already declared earlier in the function (the
block with admindb->prepare_v2, statement1_unique, and sqlite3_stmt *statement1
should not re-declare rc).
- Around line 6442-6448: The build breaks due to a duplicate declaration of rc
in the block after preparing statements; remove the redundant "int rc;" and
reuse the existing rc variable already declared at the top of the function so
the results from admindb->prepare_v2 (used for f_statement1_unique and
b_statement1_unique) and subsequent sqlite operations use the single rc
variable; update any subsequent assignments/uses in this block to refer to the
existing rc rather than a newly declared one.
- Around line 6523-6529: The code redeclares int rc after preparing statements,
causing a compile error; remove the duplicate declaration and reuse the existing
rc variable when checking return codes from admindb->prepare_v2 (keep using the
prepared objects f_statement1_unique, b_statement1_unique and their raw pointers
f_statement1, b_statement1) so that rc refers to the previously defined variable
rather than declaring a new one.
lib/ProxySQL_Admin.cpp
Outdated
Comment on lines
6296
to
6299
| auto [rc1, statement1_unique] = admindb->prepare_v2(q); | ||
| ASSERT_SQLITE_OK(rc1, admindb); | ||
| sqlite3_stmt *statement1 = statement1_unique.get(); | ||
| int rc; |
There was a problem hiding this comment.
Remove duplicate rc declaration to fix a compile error.
rc is already declared earlier in this function; redeclaring it here will fail to compile.
🛠️ Suggested fix
- sqlite3_stmt *statement1 = statement1_unique.get();
- int rc;
+ sqlite3_stmt *statement1 = statement1_unique.get();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| auto [rc1, statement1_unique] = admindb->prepare_v2(q); | |
| ASSERT_SQLITE_OK(rc1, admindb); | |
| sqlite3_stmt *statement1 = statement1_unique.get(); | |
| int rc; | |
| auto [rc1, statement1_unique] = admindb->prepare_v2(q); | |
| ASSERT_SQLITE_OK(rc1, admindb); | |
| sqlite3_stmt *statement1 = statement1_unique.get(); |
🤖 Prompt for AI Agents
In `@lib/ProxySQL_Admin.cpp` around lines 6296 - 6299, The duplicate declaration
of rc after preparing the statement causes a compile error; remove the redundant
"int rc;" line so the code uses the existing rc variable already declared
earlier in the function (the block with admindb->prepare_v2, statement1_unique,
and sqlite3_stmt *statement1 should not re-declare rc).
lib/ProxySQL_Admin.cpp
Outdated
Comment on lines
6442
to
6448
| auto [rc1, f_statement1_unique] = admindb->prepare_v2(q_stmt1_f); | ||
| ASSERT_SQLITE_OK(rc1, admindb); | ||
| auto [rc2, b_statement1_unique] = admindb->prepare_v2(q_stmt1_b); | ||
| ASSERT_SQLITE_OK(rc2, admindb); | ||
| sqlite3_stmt *f_statement1 = f_statement1_unique.get(); | ||
| sqlite3_stmt *b_statement1 = b_statement1_unique.get(); | ||
| int rc; |
There was a problem hiding this comment.
Duplicate rc redeclaration breaks the build.
rc is already declared at the top of this function; remove the second declaration.
🛠️ Suggested fix
- sqlite3_stmt *f_statement1 = f_statement1_unique.get();
- sqlite3_stmt *b_statement1 = b_statement1_unique.get();
- int rc;
+ sqlite3_stmt *f_statement1 = f_statement1_unique.get();
+ sqlite3_stmt *b_statement1 = b_statement1_unique.get();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| auto [rc1, f_statement1_unique] = admindb->prepare_v2(q_stmt1_f); | |
| ASSERT_SQLITE_OK(rc1, admindb); | |
| auto [rc2, b_statement1_unique] = admindb->prepare_v2(q_stmt1_b); | |
| ASSERT_SQLITE_OK(rc2, admindb); | |
| sqlite3_stmt *f_statement1 = f_statement1_unique.get(); | |
| sqlite3_stmt *b_statement1 = b_statement1_unique.get(); | |
| int rc; | |
| auto [rc1, f_statement1_unique] = admindb->prepare_v2(q_stmt1_f); | |
| ASSERT_SQLITE_OK(rc1, admindb); | |
| auto [rc2, b_statement1_unique] = admindb->prepare_v2(q_stmt1_b); | |
| ASSERT_SQLITE_OK(rc2, admindb); | |
| sqlite3_stmt *f_statement1 = f_statement1_unique.get(); | |
| sqlite3_stmt *b_statement1 = b_statement1_unique.get(); |
🤖 Prompt for AI Agents
In `@lib/ProxySQL_Admin.cpp` around lines 6442 - 6448, The build breaks due to a
duplicate declaration of rc in the block after preparing statements; remove the
redundant "int rc;" and reuse the existing rc variable already declared at the
top of the function so the results from admindb->prepare_v2 (used for
f_statement1_unique and b_statement1_unique) and subsequent sqlite operations
use the single rc variable; update any subsequent assignments/uses in this block
to refer to the existing rc rather than a newly declared one.
lib/ProxySQL_Admin.cpp
Outdated
Comment on lines
6523
to
6529
| auto [rc1, f_statement1_unique] = admindb->prepare_v2(q_stmt1_f); | ||
| ASSERT_SQLITE_OK(rc1, admindb); | ||
| auto [rc2, b_statement1_unique] = admindb->prepare_v2(q_stmt1_b); | ||
| ASSERT_SQLITE_OK(rc2, admindb); | ||
| sqlite3_stmt* f_statement1 = f_statement1_unique.get(); | ||
| sqlite3_stmt* b_statement1 = b_statement1_unique.get(); | ||
| int rc; |
There was a problem hiding this comment.
Duplicate rc declaration here also causes a compile error.
Use the existing rc instead of redeclaring it.
🛠️ Suggested fix
- sqlite3_stmt* f_statement1 = f_statement1_unique.get();
- sqlite3_stmt* b_statement1 = b_statement1_unique.get();
- int rc;
+ sqlite3_stmt* f_statement1 = f_statement1_unique.get();
+ sqlite3_stmt* b_statement1 = b_statement1_unique.get();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| auto [rc1, f_statement1_unique] = admindb->prepare_v2(q_stmt1_f); | |
| ASSERT_SQLITE_OK(rc1, admindb); | |
| auto [rc2, b_statement1_unique] = admindb->prepare_v2(q_stmt1_b); | |
| ASSERT_SQLITE_OK(rc2, admindb); | |
| sqlite3_stmt* f_statement1 = f_statement1_unique.get(); | |
| sqlite3_stmt* b_statement1 = b_statement1_unique.get(); | |
| int rc; | |
| auto [rc1, f_statement1_unique] = admindb->prepare_v2(q_stmt1_f); | |
| ASSERT_SQLITE_OK(rc1, admindb); | |
| auto [rc2, b_statement1_unique] = admindb->prepare_v2(q_stmt1_b); | |
| ASSERT_SQLITE_OK(rc2, admindb); | |
| sqlite3_stmt* f_statement1 = f_statement1_unique.get(); | |
| sqlite3_stmt* b_statement1 = b_statement1_unique.get(); |
🤖 Prompt for AI Agents
In `@lib/ProxySQL_Admin.cpp` around lines 6523 - 6529, The code redeclares int rc
after preparing statements, causing a compile error; remove the duplicate
declaration and reuse the existing rc variable when checking return codes from
admindb->prepare_v2 (keep using the prepared objects f_statement1_unique,
b_statement1_unique and their raw pointers f_statement1, b_statement1) so that
rc refers to the previously defined variable rather than declaring a new one.
…nctions The RAII-based prepare_v2 migration introduced duplicate variable declarations that caused compilation errors. Removed duplicate 'int rc;' declarations in: - dump_checksums_values_table() - save_mysql_users_runtime_to_database() - save_pgsql_users_runtime_to_database() The 'int rc' variable was already declared at function scope, so the duplicate declarations after the structured bindings were redundant.
Migrated 4 prepare_v2 calls to RAII-based pattern in: - pull_mysql_query_rules_from_peer(): 3 statements (statement1, statement1fr, statement32fr) - update_mysql_users(): 1 statement (also fixes memory leak - missing finalize) - update_ldap_mappings(): 1 statement (also fixes memory leak - missing finalize) The RAII pattern ensures automatic cleanup and eliminates manual (*proxy_sqlite3_finalize) calls, reducing the risk of resource leaks.
…mplate Migrated the template function FlushDigestTableToDisk to use RAII-based prepare_v2. This function is instantiated for both MySQL and PostgreSQL, eliminating 8 deprecation warnings (4 instantiations x 2 prepare_v2 calls). The RAII pattern ensures automatic cleanup and eliminates manual (*proxy_sqlite3_finalize) calls.
Migrated 5 prepare_v2 calls to RAII-based pattern in: - update_monitor_pgsql_servers(): 2 statements (stmt1, stmt32) - update_connect_table(): 1 statement (stmt) - update_ping_table(): 1 statement (stmt) - update_readonly_table(): 1 statement (stmt) - maint_monitor_table(): 1 statement (stmt) Note: Functions shunn_non_resp_srv() and update_srv_latency() were NOT migrated because they use global thread_local cached statements, which is a valid use case for the old API. The RAII pattern ensures automatic cleanup and eliminates manual sqlite_finalize_statement() calls.
Migrated 1 function with 2 prepare_v2 calls to RAII-based pattern in: - generate_pgsql_hostgroup_attributes_table(): 2 statements (statement1, statement2) The RAII pattern ensures automatic cleanup and eliminates manual (*proxy_sqlite3_finalize) calls.
Migrated all prepare_v2 calls to RAII-based pattern in 5 functions: - stats___mysql_processlist(): 2 statements (statement1, statement32) - stats___pgsql_processlist(): 2 statements - stats___mysql_free_connections(): 2 statements - stats___mysql_query_rules(): 2 statements - stats___mysql_query_digest(): 2 statements And removed manual finalization calls for all functions. The RAII pattern ensures automatic cleanup and eliminates manual (*proxy_sqlite3_finalize) calls.
…clarations - Fixed SAFE_SQLITE3_STEP2 macro errors: renamed 'rc1' to 'rc' in PgSQL_Monitor.cpp functions (update_connect_table, update_ping_table, update_readonly_table, maint_monitor_table) to match the macro's expected variable name - Removed redundant NULL declarations in ProxySQL_Admin_Stats.cpp functions (stats___pgsql_processlist, stats___pgsql_free_connections, stats___pgsql_errors, stats___save_pgsql_query_digest_to_sqlite) - Completed migration of stats___pgsql_errors and stats___save_pgsql_query_digest_to_sqlite to RAII pattern
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR addresses compiler warnings found during
make debugbuild, focusing on:Critical Bug Fix
lib/ProxySQL_Admin.cpp:2323 - Fixed use-after-free where freed pointer was being returned from child_telnet function.
Warning Fixes
HIGH Priority (2 fixes)
MySQL_HostGroups_Manager.cpp:1792- Changed loop variable tounsigned intMySQL_Thread.cpp:4357- Added cast for size_t comparisonMEDIUM Priority (12 fixes)
char thread_name[16]toconst char *thread_namechar*toconst char*for status variables#ifdef PROXYSQLGENAIblocksDeprecation Warnings (prepare_v2)
Migrated all SIMPLE prepare_v2 cases (where statements are used entirely within function scope) to the new RAII-based API:
Bonus fix:
generate_pgsql_hostgroup_attributes_table()had a memory leak (statement never finalized) - RAII migration fixed this automatically.Migration Pattern
Test plan
make clean && make debug- 0 warnings, 0 errorsSummary by CodeRabbit
Refactor
Build/Compile
Behavior
Tests