aherman
/
proxysql
mirror of https://github.com/sysown/proxysql
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix: address code review feedback from AI reviewers (#5566)

Browse Source 
Critical fixes:
- Admin_Handler: Gate IMPORT PGBOUNCER CONFIG to admin sessions only
  (stats connections can no longer mutate config)
- Admin_Handler: Wrap import execution in BEGIN/ROLLBACK for atomicity
  (partial failures no longer leave inconsistent state)
- Admin_Handler: Parse DRY RUN/IGNORE WARNINGS as trailing tokens after
  quoted path (paths containing those strings no longer break parsing)
- ProxySQL_CLI: Never write SQL to stdout on failed conversion
  (prevents partial SQL from being piped to mysql in error cases)

Important fixes:
- ConfigParser: Add INT_MIN/INT_MAX range checking in parse_int()
  and UINT_MAX checking in parse_uint() to prevent overflow
- ConfigParser: Detect unterminated single quotes in connection string
  pairs and report parse error instead of silently accepting
- ConfigParser: Include <climits> for range check constants
- ConfigParser: Set file field in "cannot open" error message
- ConfigConverter: DELETE FROM pgsql_firewall_whitelist_rules before
  importing HBA rules (prevents stale entries)
- ShowCommands: Fix SHOW POOLS query to avoid cross-join that produced
  nondeterministic results with multiple pool rows
- ProxySQL_CLI: Remove duplicate semicolons in non-dry-run output
- Admin_Handler: Fix admindb->execute() return type (bool not int)
feature/pgbouncer-compat
Rene Cannao 4 days ago
parent 4871accb74
commit 3f1027b7bf
5 changed files with 102 additions and 52 deletions
Show Stats Download Patch File Download Diff File

107
lib/Admin_Handler.cpp
Unescape View File

@ -5294,42 +5294,74 @@ __end_show_commands:
if ((query_no_space_length > 30) && !strncasecmp("IMPORT PGBOUNCER CONFIG FROM ", query_no_space, 29)) {
proxy_info("Received %s command\n", query_no_space);
// Parse command: extract file path and flags
// Only allow from admin sessions, not stats
if (sess->session_type != PROXYSQL_SESSION_ADMIN) {
SPA->send_error_msg_to_client(sess, (char *)"IMPORT PGBOUNCER CONFIG requires admin privileges");
run_query = false;
goto __run_query;
}
// Parse flags as trailing tokens (after the quoted path).
// Syntax: IMPORT PGBOUNCER CONFIG FROM '/path' [DRY RUN] [IGNORE WARNINGS]
// Extract the quoted path first, then check remaining tokens for flags.
std::string cmd_rest = std::string(query_no_space + 29);
bool dry_run = false;
bool ignore_warnings = false;
// Check for DRY RUN and IGNORE WARNINGS flags (case-insensitive)
// Find the file path (first quoted or unquoted token)
std::string file_path_str;
std::string remaining;
{
std::string upper_rest = cmd_rest;
std::transform(upper_rest.begin(), upper_rest.end(), upper_rest.begin(), ::toupper);
if (upper_rest.find("DRY RUN") != std::string::npos) {
size_t start = cmd_rest.find_first_not_of(" \t");
if (start == std::string::npos) {
SPA->send_error_msg_to_client(sess, (char *)"Missing file path");
run_query = false;
goto __run_query;
}
if (cmd_rest[start] == '\'' || cmd_rest[start] == '"') {
char quote = cmd_rest[start];
size_t end = cmd_rest.find(quote, start + 1);
if (end == std::string::npos) {
SPA->send_error_msg_to_client(sess, (char *)"Unterminated quote in file path");
run_query = false;
goto __run_query;
}
file_path_str = cmd_rest.substr(start + 1, end - start - 1);
remaining = cmd_rest.substr(end + 1);
} else {
size_t end = cmd_rest.find_first_of(" \t", start);
if (end == std::string::npos) {
file_path_str = cmd_rest.substr(start);
} else {
file_path_str = cmd_rest.substr(start, end - start);
remaining = cmd_rest.substr(end);
}
}
}
// Parse remaining tokens for flags
{
std::string upper_rem = remaining;
std::transform(upper_rem.begin(), upper_rem.end(), upper_rem.begin(), ::toupper);
// Remove extra whitespace for matching
// Check for "DRY RUN" and "IGNORE WARNINGS" as whole tokens
if (upper_rem.find("DRY RUN") != std::string::npos) {
dry_run = true;
auto pos = upper_rest.find("DRY RUN");
cmd_rest.erase(pos, 7);
}
upper_rest = cmd_rest;
std::transform(upper_rest.begin(), upper_rest.end(), upper_rest.begin(), ::toupper);
if (upper_rest.find("IGNORE WARNINGS") != std::string::npos) {
if (upper_rem.find("IGNORE WARNINGS") != std::string::npos) {
ignore_warnings = true;
auto pos = upper_rest.find("IGNORE WARNINGS");
cmd_rest.erase(pos, 15);
}
}
char *path_buf = strdup(cmd_rest.c_str());
char *file_path = trim_spaces_and_quotes_in_place(path_buf);
// Stage 1: Parse PgBouncer config
PgBouncer::Config pgb_config;
bool parse_ok = PgBouncer::parse_config_file(std::string(file_path), pgb_config);
bool parse_ok = PgBouncer::parse_config_file(file_path_str, pgb_config);
if (!parse_ok) {
std::string err_msg = "Failed to parse PgBouncer config: " + std::string(file_path);
std::string err_msg = "Failed to parse PgBouncer config: " + file_path_str;
for (const auto& e : pgb_config.errors) {
err_msg += "\n " + e.file + ":" + std::to_string(e.line) + ": " + e.message;
}
SPA->send_error_msg_to_client(sess, (char *)err_msg.c_str());
free(path_buf);
run_query = false;
goto __run_query;
}
@ -5345,16 +5377,15 @@ __end_show_commands:
err_msg += "\n ERROR: " + e.message;
}
if (dry_run) {
err_msg += "\n\n" + PgBouncer::ConfigConverter::format_dry_run(result, file_path, strict);
err_msg += "\n\n" + PgBouncer::ConfigConverter::format_dry_run(result, file_path_str.c_str(), strict);
}
SPA->send_error_msg_to_client(sess, (char *)err_msg.c_str());
free(path_buf);
run_query = false;
goto __run_query;
}
if (dry_run) {
std::string dry_output = PgBouncer::ConfigConverter::format_dry_run(result, file_path, strict);
std::string dry_output = PgBouncer::ConfigConverter::format_dry_run(result, file_path_str.c_str(), strict);
std::string escaped = dry_output;
size_t pos = 0;
while ((pos = escaped.find('\'', pos)) != std::string::npos) {
@ -5365,27 +5396,35 @@ __end_show_commands:
std::string select_q = "SELECT '" + escaped + "' AS dry_run_output";
query = l_strdup(select_q.c_str());
query_length = strlen(query) + 1;
free(path_buf);
goto __run_query;
}
// Execute: apply the conversion
// Execute atomically: wrap in transaction so partial failure rolls back
SPA->admindb->execute("BEGIN");
bool exec_ok = true;
for (const auto& entry : result.entries) {
if (!entry.sql.empty()) {
SPA->admindb->execute(entry.sql.c_str());
bool rc = SPA->admindb->execute(entry.sql.c_str());
if (!rc) {
exec_ok = false;
SPA->admindb->execute("ROLLBACK");
SPA->send_error_msg_to_client(sess, (char *)"Import failed during SQL execution; changes rolled back");
break;
}
}
}
std::string ok_msg = "PgBouncer config imported: " +
std::to_string(result.server_count) + " servers, " +
std::to_string(result.user_count) + " users, " +
std::to_string(result.rule_count) + " query rules, " +
std::to_string(result.variable_count) + " variables";
if (!result.warnings.empty()) {
ok_msg += " (" + std::to_string(result.warnings.size()) + " warnings)";
if (exec_ok) {
SPA->admindb->execute("COMMIT");
std::string ok_msg = "PgBouncer config imported: " +
std::to_string(result.server_count) + " servers, " +
std::to_string(result.user_count) + " users, " +
std::to_string(result.rule_count) + " query rules, " +
std::to_string(result.variable_count) + " variables";
if (!result.warnings.empty()) {
ok_msg += " (" + std::to_string(result.warnings.size()) + " warnings)";
}
SPA->send_ok_msg_to_client(sess, (char *)ok_msg.c_str(), 0, query_no_space);
}
SPA->send_ok_msg_to_client(sess, (char *)ok_msg.c_str(), 0, query_no_space);
free(path_buf);
run_query = false;
goto __run_query;
}

7
lib/pgbouncer_compat/PgBouncer_ConfigConverter.cpp
Unescape View File

@ -375,6 +375,13 @@ void ConfigConverter::convert_hba_rules(const Config& config,
bool any_converted = false;
if (!config.hba_rules.empty()) {
result.entries.push_back({
"DELETE FROM pgsql_firewall_whitelist_rules;",
"Remove existing firewall whitelist rules before importing HBA rules"
});
}
for (const auto& rule : config.hba_rules) {
// Unsupported connection types
if (rule.conn_type == "local") {

18
lib/pgbouncer_compat/PgBouncer_ConfigParser.cpp
Unescape View File

@ -4,6 +4,7 @@
#include <sstream>
#include <algorithm>
#include <cctype>
#include <climits>
namespace PgBouncer {
@ -57,6 +58,7 @@ bool ConfigParser::parse_int(const std::string& value, int& result) {
size_t pos = 0;
long v = std::stol(value, &pos);
if (pos != value.size()) return false;
if (v < INT_MIN || v > INT_MAX) return false;
result = static_cast<int>(v);
return true;
} catch (...) {
@ -70,6 +72,7 @@ bool ConfigParser::parse_uint(const std::string& value, unsigned int& result) {
size_t pos = 0;
unsigned long v = std::stoul(value, &pos);
if (pos != value.size()) return false;
if (v > UINT_MAX) return false;
result = static_cast<unsigned int>(v);
return true;
} catch (...) {
@ -117,24 +120,27 @@ bool ConfigParser::parse_connstr_pairs(
// Quoted value: collect until unescaped closing quote
// PgBouncer escapes single quotes by doubling: ''
++i; // skip opening quote
std::string raw = "'";
bool closed = false;
while (i < len) {
if (connstr[i] == '\'') {
if (i + 1 < len && connstr[i + 1] == '\'') {
raw += "''";
value += '\'';
i += 2;
} else {
// closing quote
++i;
closed = true;
break;
}
} else {
raw += connstr[i];
value += connstr[i];
++i;
}
}
raw += "'";
value = unquote(raw);
if (!closed) {
errors.push_back({file, line, "unterminated single quote in connection string value for key '" + key + "'"});
return false;
}
} else {
// Unquoted value: read until whitespace
size_t val_start = i;
@ -503,7 +509,7 @@ bool ConfigParser::parse_ini(
{
std::ifstream ifs(filepath);
if (!ifs.is_open()) {
config.errors.push_back({"", 0, "cannot open file: " + filepath});
config.errors.push_back({filepath, 0, "cannot open file: " + filepath});
return false;
}

14
lib/pgbouncer_compat/PgBouncer_ShowCommands.cpp
Unescape View File

@ -71,8 +71,8 @@ static std::vector<std::string> tokenize_upper(const std::string& s) {
static std::string query_pools(bool extended) {
std::string q =
"SELECT "
"'default' AS database, "
"su.username AS user, "
"cp.srv_host AS database, "
"'-' AS user, "
"0 AS cl_active, "
"0 AS cl_waiting, "
"0 AS cl_cancel_req, "
@ -83,20 +83,16 @@ static std::string query_pools(bool extended) {
"0 AS sv_login, "
"0 AS maxwait, "
"0 AS maxwait_us, "
"CASE WHEN su.fast_forward=1 THEN 'session' "
"WHEN su.transaction_persistent=1 THEN 'transaction' "
"ELSE 'statement' END AS pool_mode";
"'statement' AS pool_mode";
if (extended) {
q += ", cp.hostgroup AS hostgroup_id"
", cp.ConnUsed AS multiplex"
", 1 AS multiplex"
", cp.Latency_us AS latency_us"
", cp.Queries AS Queries"
", cp.Bytes_data_sent AS Bytes_data_sent"
", cp.Bytes_data_recv AS Bytes_data_recv";
}
q += " FROM stats_pgsql_connection_pool cp"
" JOIN runtime_pgsql_users su ON 1=1"
" GROUP BY su.username";
q += " FROM stats_pgsql_connection_pool cp";
return q;
}

8
lib/pgbouncer_compat/ProxySQL_CLI.cpp
Unescape View File

@ -68,8 +68,10 @@ static int cmd_import_pgbouncer(int argc, const char* argv[]) {
for (const auto& err : result.errors) {
std::cerr << " ERROR: " << err.message << "\n";
}
// Still show the dry-run output so the user can see what would have been converted
std::cout << PgBouncer::ConfigConverter::format_dry_run(result, config_path, strict);
// Show dry-run output on stderr only (never stdout — it could be piped to mysql)
if (dry_run) {
std::cerr << "\n" << PgBouncer::ConfigConverter::format_dry_run(result, config_path, strict);
}
return 1;
}
@ -98,7 +100,7 @@ static int cmd_import_pgbouncer(int argc, const char* argv[]) {
if (!entry.comment.empty()) {
std::cout << "-- " << entry.comment << "\n";
}
std::cout << entry.sql << ";\n";
std::cout << entry.sql << "\n";
}
if (!result.warnings.empty()) {

Write Preview
Loading…
Cancel
Save