From 79dcbef51db8db7ed45b392b65d19de71e2c32af Mon Sep 17 00:00:00 2001 From: takaidohigasi Date: Wed, 27 May 2026 09:23:12 +0900 Subject: [PATCH] Admin: strip leading SQL block comments before connect-setup matchers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two connect-setup statement accept blocks in admin_session_handler — SET SQL_SAFE_UPDATES=1 (fix bug #442) and the BEGIN/COMMIT/ROLLBACK/ SET NAMES/SET AUTOCOMMIT/... block (fix bug #1047) — use strncmp/ strncasecmp prefix matching against the start of query_no_space. Clients that prepend SQL tracing comments to every query — SQLCommenter, Datadog Agent's mysql integration CommenterCursor, Sequelize, Hibernate, etc. — defeat the prefix match. The query falls through to the SQLite engine, which rejects it with `near "SET": syntax error` (wrapped as MySQL error code 1045). The most visible manifestation is Datadog Agent autodiscovery on the admin port failing during connection setup, before any custom_queries run. Add a small helper `skip_leading_sql_comments` that returns a pointer past leading whitespace and /* ... */ block comments, then apply it to both accept blocks via a single local pointer. The original query buffer is unchanged, so audit / digest / error-echo paths see the original text. Only block comments are handled here on purpose. MySQL `-- ` / `#` line comments can't be parsed safely in this code path: remove_spaces() runs before this matching and collapses '\\n' and '\\r' to a single space, so the line-comment terminator no longer exists in the buffer. Block-comment recognition is unaffected because `*/` survives intact — and block comments are the form SQL-commenter clients actually emit. Test: test/tap/tests/mysql-reg_test_5786_admin_strip_leading_sql_comments-t.cpp covers SET AUTOCOMMIT/SET NAMES/SET character_set_results/SET SQL_AUTO_IS_NULL/ SET SQL_SAFE_UPDATES/BEGIN/START TRANSACTION/COMMIT/ROLLBACK each with a leading block comment, the exact Datadog Agent CommenterCursor payload, stacked comments, leading+trailing comments, and a keyword-in-comment- text negative case (must not false-positive). Fixes #5786. --- lib/Admin_Handler.cpp | 118 +++++++++++++---- test/tap/groups/groups.json | 1 + ...786_admin_strip_leading_sql_comments-t.cpp | 119 ++++++++++++++++++ 3 files changed, 211 insertions(+), 27 deletions(-) create mode 100644 test/tap/tests/mysql-reg_test_5786_admin_strip_leading_sql_comments-t.cpp diff --git a/lib/Admin_Handler.cpp b/lib/Admin_Handler.cpp index 9cfedf44c..b04eb3cdc 100644 --- a/lib/Admin_Handler.cpp +++ b/lib/Admin_Handler.cpp @@ -429,6 +429,56 @@ static void convert_regex_to_like(char* dst, const char* src, size_t dst_size) { *dst = '\0'; } +// Return a pointer into `q` past leading whitespace and /* ... */ block +// comments. `len` bounds the scan; the returned pointer is within +// [q, q+len]. The buffer is not mutated; an unterminated /* consumes +// the rest. +// +// Used so the connect-setup matchers below recognise queries prefixed +// by SQL tracing comments emitted by SQLCommenter, Datadog Agent's +// mysql integration CommenterCursor, Sequelize, Hibernate, etc. — all +// of which use the /* ... */ form. See sysown/proxysql#5786. +// +// Only block comments are handled here on purpose: by the time this +// helper runs, remove_spaces() above has already collapsed runs of +// whitespace (including '\n' and '\r') to a single space, so MySQL +// `-- ` / `#` line-comment terminators no longer exist in the buffer +// and cannot be parsed unambiguously. Block-comment recognition is +// unaffected by remove_spaces because `*/` survives intact. +static const char* skip_leading_sql_comments(const char* q, size_t len) { + // SQL whitespace is a fixed ASCII set; using an inline check rather + // than isspace() avoids any C-locale dependency. + auto is_sql_ws = [](char c) -> bool { + return c == ' ' || c == '\t' || c == '\n' + || c == '\r' || c == '\v' || c == '\f'; + }; + const char* p = q; + size_t n = len; + while (n > 0) { + while (n > 0 && is_sql_ws(*p)) { + p++; n--; + } + if (n == 0) break; + + // /* ... */ block comment (SQL: no nesting) + if (n >= 2 && p[0] == '/' && p[1] == '*') { + p += 2; n -= 2; + while (n >= 2 && !(p[0] == '*' && p[1] == '/')) { + p++; n--; + } + if (n >= 2) { + p += 2; n -= 2; // consume closing */ + } else { + p += n; n = 0; // unterminated /* — consume rest + } + continue; + } + + break; + } + return p; +} + // Unified handler for \dt, \di, \dv commands // sqlite_type: "table", "index", or "view" // columns: columns to SELECT (e.g., "name" or "name, tbl_name") @@ -3854,34 +3904,48 @@ void admin_session_handler(S* sess, void *_pa, PtrSize_t *pkt) { goto __run_query; } - // fix bug #442 - if (!strncmp("SET SQL_SAFE_UPDATES=1", query_no_space, strlen("SET SQL_SAFE_UPDATES=1"))) { - SPA->send_ok_msg_to_client(sess, NULL, 0, query_no_space); - run_query=false; - goto __run_query; - } + // Strip leading SQL comments once for the two connect-setup accept + // blocks below, so clients that prepend SQL tracing comments + // (SQLCommenter, Datadog Agent's mysql integration CommenterCursor, + // Sequelize, Hibernate, …) are recognised. The original + // `query_no_space` is unchanged and is what gets passed to + // `send_ok_msg_to_client` for audit/digest fidelity. The local + // `mb` is wrapped in its own block scope so the existing `goto + // __run_query` statements earlier in this function do not jump + // past its initialiser (cpp:S1036). + // See sysown/proxysql#5786. + { + const char *mb = skip_leading_sql_comments(query_no_space, query_no_space_length); - // fix bug #1047 - if ( - (!strncasecmp("BEGIN", query_no_space, strlen("BEGIN"))) - || - (!strncasecmp("START TRANSACTION", query_no_space, strlen("START TRANSACTION"))) - || - (!strncasecmp("COMMIT", query_no_space, strlen("COMMIT"))) - || - (!strncasecmp("ROLLBACK", query_no_space, strlen("ROLLBACK"))) - || - (!strncasecmp("SET character_set_results", query_no_space, strlen("SET character_set_results"))) - || - (!strncasecmp("SET SQL_AUTO_IS_NULL", query_no_space, strlen("SET SQL_AUTO_IS_NULL"))) - || - (!strncasecmp("SET NAMES", query_no_space, strlen("SET NAMES"))) - || - (!strncasecmp("SET AUTOCOMMIT", query_no_space, strlen("SET AUTOCOMMIT"))) - ) { - SPA->send_ok_msg_to_client(sess, NULL, 0, query_no_space); - run_query=false; - goto __run_query; + // fix bug #442 + if (!strncmp("SET SQL_SAFE_UPDATES=1", mb, strlen("SET SQL_SAFE_UPDATES=1"))) { + SPA->send_ok_msg_to_client(sess, NULL, 0, query_no_space); + run_query=false; + goto __run_query; + } + + // fix bug #1047 + if ( + (!strncasecmp("BEGIN", mb, strlen("BEGIN"))) + || + (!strncasecmp("START TRANSACTION", mb, strlen("START TRANSACTION"))) + || + (!strncasecmp("COMMIT", mb, strlen("COMMIT"))) + || + (!strncasecmp("ROLLBACK", mb, strlen("ROLLBACK"))) + || + (!strncasecmp("SET character_set_results", mb, strlen("SET character_set_results"))) + || + (!strncasecmp("SET SQL_AUTO_IS_NULL", mb, strlen("SET SQL_AUTO_IS_NULL"))) + || + (!strncasecmp("SET NAMES", mb, strlen("SET NAMES"))) + || + (!strncasecmp("SET AUTOCOMMIT", mb, strlen("SET AUTOCOMMIT"))) + ) { + SPA->send_ok_msg_to_client(sess, NULL, 0, query_no_space); + run_query=false; + goto __run_query; + } } // MySQL client check command for dollars quote support, starting at version '8.1.0'. See #4300. diff --git a/test/tap/groups/groups.json b/test/tap/groups/groups.json index 99d41e652..d1e8d5b2c 100644 --- a/test/tap/groups/groups.json +++ b/test/tap/groups/groups.json @@ -90,6 +90,7 @@ "mysql-reg_test_4716_single_semicolon-t" : [ "legacy-g1","mysql-auto_increment_delay_multiplex=0-g1","mysql-multiplexing=false-g1","mysql-query_digests=0-g1","mysql-query_digests_keep_comment=1-g1","mysql84-g1","mysql90-g1","mysql95-g1" ], "mysql-reg_test_4723_query_cache_stores_empty_result-t" : [ "legacy-g1","mysql-auto_increment_delay_multiplex=0-g1","mysql-multiplexing=false-g1","mysql-query_digests=0-g1","mysql-query_digests_keep_comment=1-g1","mysql84-g1","mysql90-g1","mysql95-g1" ], "mysql-reg_test_4867_query_rules-t" : [ "legacy-g1","mysql-auto_increment_delay_multiplex=0-g1","mysql-multiplexing=false-g1","mysql-query_digests=0-g1","mysql-query_digests_keep_comment=1-g1","mysql84-g1","mysql90-g1","mysql95-g1" ], + "mysql-reg_test_5786_admin_strip_leading_sql_comments-t" : [ "legacy-g1","mysql-auto_increment_delay_multiplex=0-g1","mysql-multiplexing=false-g1","mysql-query_digests=0-g1","mysql-query_digests_keep_comment=1-g1","mysql84-g1","mysql90-g1","mysql95-g1" ], "mysql-select_version_without_backend-t" : [ "legacy-g1","mysql-auto_increment_delay_multiplex=0-g1","mysql-multiplexing=false-g1","mysql-query_digests=0-g1","mysql-query_digests_keep_comment=1-g1","mysql84-g1","mysql90-g1","mysql95-g1" ], "mysql-session_track_variables_enforced-t" : [ "legacy-g4","mysql-auto_increment_delay_multiplex=0-g4","mysql-multiplexing=false-g4","mysql-query_digests=0-g4","mysql-query_digests_keep_comment=1-g4","mysql56-single-g1" ], "mysql-session_track_variables_ff-t" : [ "legacy-g4","mysql-auto_increment_delay_multiplex=0-g4","mysql-multiplexing=false-g4","mysql-query_digests=0-g4","mysql-query_digests_keep_comment=1-g4" ], diff --git a/test/tap/tests/mysql-reg_test_5786_admin_strip_leading_sql_comments-t.cpp b/test/tap/tests/mysql-reg_test_5786_admin_strip_leading_sql_comments-t.cpp new file mode 100644 index 000000000..6fca63d12 --- /dev/null +++ b/test/tap/tests/mysql-reg_test_5786_admin_strip_leading_sql_comments-t.cpp @@ -0,0 +1,119 @@ +/** + * @file mysql-reg_test_5786_admin_strip_leading_sql_comments-t.cpp + * @brief Regression test for sysown/proxysql#5786 — the Admin interface + * connect-setup accept blocks must recognise queries prefixed by + * /* ... *\/ SQL tracing comments. + * + * SQL-commenter-style clients (SQLCommenter, Datadog Agent's mysql + * integration CommenterCursor, Sequelize, Hibernate, ...) prepend a + * tracing block comment to every query, e.g.: + * + * /*service='datadog-agent'*\/ SET AUTOCOMMIT=1 + * + * Without the fix, the leading "/*" defeats the strncasecmp prefix + * match in admin_session_handler, the query falls through to the + * SQLite engine, and the client sees: + * ERROR 1045 (28000): ProxySQL Admin Error: near "SET": syntax error + * + * This test connects to the admin interface and issues a representative + * mix of comment-prefixed connect-setup statements; each should be + * silently accepted (mysql_errno() == 0). It also exercises the + * negative case (comment text containing the keyword but no real + * connect-setup statement after) to make sure we don't false-positive. + */ + +#include +#include +#include + +#include "mysql.h" +#include "command_line.h" +#include "tap.h" +#include "utils.h" + +CommandLine cl; + +// Accept-as-no-op queries: the connect-setup matchers should silently +// consume these and the client should see rc == 0 with NO result set +// (send_ok_msg_to_client emits an OK packet, not a result set). +static const char* ACCEPT_CASES[] = { + "SET AUTOCOMMIT=1", // baseline (no comment) + "/*svc=dd*/ SET AUTOCOMMIT=1", + "/*service='datadog-agent'*/ SET AUTOCOMMIT=1", // exact DD Agent payload + "/* a *//* b */ SET AUTOCOMMIT=1", // stacked comments + "/*svc=dd*/ SET NAMES utf8", + "/*svc=dd*/ SET character_set_results=utf8", + "/*svc=dd*/ SET SQL_AUTO_IS_NULL=0", + "/*svc=dd*/ SET SQL_SAFE_UPDATES=1", // covers fix bug #442 + "/*svc=dd*/ BEGIN", + "/*svc=dd*/ START TRANSACTION", + "/*svc=dd*/ COMMIT", + "/*svc=dd*/ ROLLBACK", + "/*svc=dd*/ SET AUTOCOMMIT=1 /* trailing */", // leading + trailing +}; + +int main() { + const int n_accept = static_cast(std::size(ACCEPT_CASES)); + // Each accept case contributes 2 ok()'s (query OK + no resultset). + // The negative case contributes 3 (query OK, resultset present, row == "1"). + plan(n_accept * 2 + 3); + + if (cl.getEnv()) + return exit_status(); + + MYSQL* proxysql_admin = mysql_init(NULL); + if (!proxysql_admin) { + BAIL_OUT("mysql_init failed"); + } + if (!mysql_real_connect(proxysql_admin, cl.host, cl.admin_username, + cl.admin_password, NULL, cl.admin_port, NULL, 0)) { + fprintf(stderr, "Failed to connect to ProxySQL admin: %s\n", + mysql_error(proxysql_admin)); + mysql_close(proxysql_admin); + return exit_status(); + } + + // ---- accept cases: rc == 0 AND no result set (send_ok_msg_to_client) ---- + for (const char* q : ACCEPT_CASES) { + int rc = mysql_query(proxysql_admin, q); + int errno_ = static_cast(mysql_errno(proxysql_admin)); + ok(rc == 0 && errno_ == 0, + "accept | query=%-65s | rc=%d errno=%d msg=%s", + q, rc, errno_, errno_ ? mysql_error(proxysql_admin) : "(none)"); + + MYSQL_RES* res = mysql_store_result(proxysql_admin); + ok(res == nullptr && mysql_field_count(proxysql_admin) == 0, + "accept | query=%-65s | no result set returned", q); + if (res) mysql_free_result(res); + } + + // ---- negative case: keyword inside a comment must NOT be silently + // accepted. The query after the comment is `SELECT 1` against + // the admin schema; SQLite must execute it and return one row + // with value "1". If the helper false-positives and the accept + // block fires, we'd get an OK packet with NO result set, and + // the row check below would fail. + { + const char* q = "/* SET AUTOCOMMIT=1 inside comment */ SELECT 1"; + int rc = mysql_query(proxysql_admin, q); + int errno_ = static_cast(mysql_errno(proxysql_admin)); + ok(rc == 0 && errno_ == 0, + "negative | query=%s | rc=%d errno=%d msg=%s", + q, rc, errno_, errno_ ? mysql_error(proxysql_admin) : "(none)"); + + MYSQL_RES* res = mysql_store_result(proxysql_admin); + ok(res != nullptr, "negative | %s | result set present", q); + if (res) { + MYSQL_ROW row = mysql_fetch_row(res); + bool row_ok = (row != nullptr) && (row[0] != nullptr) && (strcmp(row[0], "1") == 0); + ok(row_ok, "negative | %s | first row value is \"1\" (got=%s)", + q, (row && row[0]) ? row[0] : "(null)"); + mysql_free_result(res); + } else { + ok(false, "negative | %s | no row to check (false-positive accept?)", q); + } + } + + mysql_close(proxysql_admin); + return exit_status(); +}