fix(core): validate_charset must replace collation 255 on MariaDB backends (#5790)#5807
Conversation
…kends (#5790) Commit 7c6f42b ("fix: MySQL 9.x charset handling and log_last_insert_id test race") changed the version guard in lib/MySQL_Variables.cpp from testing the first character of server_version to using atoi(): - if (charset >= 255 && myconn->mysql->server_version[0] != '8') { + if (charset >= 255 && atoi(myconn->mysql->server_version) < 8) { The new form correctly extends the "supports collation id 255" treatment to MySQL 9.x and later, but it silently broke MariaDB: server_version old check new check ---------------------------- --------- --------- "5.7.x" replace replace "8.0.x" skip skip "9.x.y" replace[*] skip "10.11.13-MariaDB" replace skip <-- BUG "11.x.x-MariaDB" replace skip <-- BUG [*] wrong for MySQL 9.x; that is the case the original commit fixed. MariaDB does not implement utf8mb4_0900_ai_ci (collation id 255), regardless of its numeric major version. With the regression, ProxySQL no longer rewrites the unsupported collation before sending SET NAMES to the MariaDB backend, and the backend rejects it with: ERROR 1273 (HY000): Unknown collation: 'utf8mb4_0900_ai_ci' …breaking clients such as Dovecot that pick collation 255 from libmariadb's charset table at connect time. Fix: also treat MariaDB (detected by the "MariaDB" suffix in server_version) as a backend that cannot accept collation id >= 255, so handle_unknown_charset replacement applies there as well. The regression is present on v3.0 HEAD (and on the v3.1.8 / v4.0.8 tags), but NOT in the v3.0.7 / v3.0.8 release tags themselves; users hitting this in production are on post-v3.0.8 "v3.0-head" CI packages (`3.0.8-NN-gHASH`). Reported-by: @FoZo (#5790) Also-confirmed-by: @akrus Adds reg_test_5790-mariadb_collation_255-t, registered in mariadb10-galera-g1, which: 1. Skips when the backend is not MariaDB. 2. Sets mysql-handle_unknown_charset=1 and mysql-default_collation_connection=utf8mb4_general_ci. 3. Issues `SET NAMES utf8mb4 COLLATE utf8mb4_0900_ai_ci`. 4. Runs a SELECT that forces backend SET NAMES via handler_again___status_CHANGING_CHARSET. 5. Verifies the SELECT succeeds and that the backend's @@collation_connection is the configured default, not utf8mb4_0900_ai_ci. This test fails without the fix (errno 1273 from the backend) and passes with it.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (1)**/*.{cpp,h,hpp}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🔇 Additional comments (1)
📝 WalkthroughWalkthroughDetect MariaDB from server_version and treat collation IDs >=255 as unsupported for MariaDB or MySQL major <8; adjust Galera monitor and session SET-variable rewrites accordingly. Add a regression test that ensures ProxySQL does not forward collation 255 to MariaDB backends. ChangesMariaDB Collation 255 Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![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 pull request fixes a regression where MariaDB backends were incorrectly identified as supporting MySQL 8.0+ collation IDs, leading to errors when using collations like utf8mb4_0900_ai_ci. It also introduces a new regression test. Feedback points out a potential crash in the version check logic if the server version string is null and identifies several memory leaks in the test code's error handling paths.
| // major version (10.x, 11.x would otherwise pass an atoi() < 8 check). | ||
| const char *sv = myconn->mysql->server_version; | ||
| bool is_mariadb = (sv != NULL && strstr(sv, "MariaDB") != NULL); | ||
| if (charset >= 255 && (is_mariadb || atoi(sv) < 8)) { |
There was a problem hiding this comment.
This line can cause a crash if sv is NULL. If is_mariadb is false, the expression short-circuits to atoi(sv), which would be atoi(NULL). This is undefined behavior.
Please guard the atoi call. The suggestion below fixes the crash.
Note that with this fix, if sv is NULL, the condition will be false and no replacement will happen. This is safer, but you may want to consider if replacement should be the default when the version is unknown (e.g., by checking !sv first).
if (charset >= 255 && (is_mariadb || (sv && atoi(sv) < 8))) {| if (!mysql_real_connect(mysql_probe, cl.host, cl.username, cl.password, | ||
| NULL, cl.port, NULL, 0)) { | ||
| diag("Frontend connect for probe failed: %s", mysql_error(mysql_probe)); | ||
| return exit_status(); | ||
| } |
There was a problem hiding this comment.
There's a memory leak here. If mysql_real_connect fails, the function returns without calling mysql_close(mysql_probe), leaking the MYSQL object allocated by mysql_init.
You should call mysql_close(mysql_probe) before returning on error.
This same issue exists for the admin connection (line 74) and the mysql connection (line 95).
| if (!mysql_real_connect(mysql_probe, cl.host, cl.username, cl.password, | |
| NULL, cl.port, NULL, 0)) { | |
| diag("Frontend connect for probe failed: %s", mysql_error(mysql_probe)); | |
| return exit_status(); | |
| } | |
| if (!mysql_real_connect(mysql_probe, cl.host, cl.username, cl.password, NULL, cl.port, NULL, 0)) { | |
| diag("Frontend connect for probe failed: %s", mysql_error(mysql_probe)); | |
| mysql_close(mysql_probe); | |
| return exit_status(); | |
| } |
Address @gemini-code-assist review on PR #5807: the previous patch unconditionally evaluated `atoi(sv)` in the disjunction `(is_mariadb || atoi(sv) < 8)`. If `sv` was NULL, the short-circuit on `is_mariadb` (false) would still reach `atoi(NULL)`, which is undefined behavior per the C standard. In practice `myconn->mysql->server_version` is non-NULL by the time validate_charset() runs (CHANGING_CHARSET is only reachable after the backend handshake has populated server_version), so this is defensive, not a real crash path. Still essentially free, and keeps the guard consistent with the existing NULL check on `strstr(sv, "MariaDB")`. No behavior change for any reachable input.
The same mental defect fixed in lib/MySQL_Variables.cpp:validate_charset()
(commit before this one) exists at four other call sites in core. All
four use a first-character or 3-byte string-prefix comparison against
server_version and consequently misclassify either MariaDB or MySQL 9.x
(or both). Two are latent (MariaDB happens to land on the still-working
branch); the two in MySQL_Monitor.cpp are an active break on MySQL 9.x
Galera/PXC, because `INFORMATION_SCHEMA.GLOBAL_STATUS` was removed in
MySQL 8.4 and MySQL 9.x falls into the else branch that targets it.
Audited sites and fixes:
lib/MySQL_Session.cpp:2607-2615 -- tx_isolation / tx_read_only
Pre-fix: `strncmp(sv,"8",1)==0` chose `transaction_isolation` /
`transaction_read_only`; everything else got `tx_isolation` /
`tx_read_only`. MySQL 9.x fell to the else branch, but `tx_isolation`
was removed in MySQL 8.4; on MySQL 9.x the SET fails. MariaDB also
fell to the else branch -- coincidentally correct, since MariaDB
keeps `tx_isolation` across all versions.
Post-fix: use the modern name only for non-MariaDB MySQL >= 8.0
(any major), so MySQL 8.0/8.4/9.x all get `transaction_isolation`
and MariaDB 10/11 keeps `tx_isolation`.
lib/MySQL_Monitor.cpp:749 (sync MON_GALERA setup)
lib/MySQL_Monitor.cpp:2297 (async MON_GALERA dispatch)
Pre-fix: `strncmp(sv,"5.7",3)==0 || strncmp(sv,"8",1)==0` picked
performance_schema.global_status; everything else used
INFORMATION_SCHEMA.GLOBAL_STATUS. MySQL 9.x fell to the else branch
and queries INFORMATION_SCHEMA.GLOBAL_STATUS, which was removed in
MySQL 8.4 -- monitor query fails on MySQL 9.x Galera/PXC. MariaDB
Galera correctly fell to the else branch and uses
information_schema, which is where MariaDB keeps GLOBAL_STATUS.
Post-fix: use performance_schema for non-MariaDB MySQL >= 5.7 (so
MySQL 5.7, 8.0, 8.4, 9.x all share that path); MariaDB Galera and
legacy MySQL/PXC < 5.7 retain the information_schema path.
Pattern used at all sites (matching the existing fix in
validate_charset):
const char *sv = ...->mysql->server_version;
bool is_mariadb = (sv != NULL && strstr(sv, "MariaDB") != NULL);
int sv_major = (sv != NULL) ? atoi(sv) : 0;
No dedicated test added for the Monitor / SET TRANSACTION paths in this
commit: the Monitor change is exercised by any mariadb10-galera or
mysql{8,9}x-* CI group with Galera/PXC backends, and the
transaction_isolation change by SET-isolation-level coverage in
existing TAP groups. The validate_charset regression test added in
the prior commit on this branch still covers the original #5790 path.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/MySQL_Session.cpp`:
- Around line 2607-2617: The temp buffer `query` (allocated from
strlen(var_name)) is too small when you rewrite short `tx_*` names to longer
`transaction_*` names in MySQL_Session.cpp; update the code that prepares
`query` (used by sprintf when writing "transaction_isolation" /
"transaction_read_only") to allocate a safe size or use a bounded/size-aware
API: compute needed_size = strlen(q) + max_replacement_extra + 1 (or switch to
std::string) and replace sprintf(...) with snprintf(..., needed_size, ...) so
the buffer cannot overflow; reference symbols: query, var_name,
sprintf/snprintf, mybe->server_myds->myconn->mysql->server_version, is_mariadb,
sv_major, and the branches that write
"transaction_isolation"/"transaction_read_only".
🪄 Autofix (Beta)
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
Run ID: 0b7e8e7f-972d-451d-97ae-42b0425f098f
📒 Files selected for processing (3)
lib/MySQL_Monitor.cpplib/MySQL_Session.cpplib/MySQL_Variables.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names should use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables should use snake_case
Constants and macros should use UPPER_SNAKE_CASE
Use C++17 features; conditional compilation should use#ifdefPROXYSQL31,#ifdefPROXYSQL40,#ifdefPROXYSQLFFTO,#ifdefPROXYSQLTSDB,#ifdefPROXYSQLCLICKHOUSE (PROXYSQLGENAI no longer guards core code)
Use RAII for resource management and jemalloc for memory allocation
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
lib/MySQL_Session.cpplib/MySQL_Variables.cpplib/MySQL_Monitor.cpp
🔇 Additional comments (2)
lib/MySQL_Variables.cpp (1)
346-352: LGTM!lib/MySQL_Monitor.cpp (1)
749-762: LGTM!Also applies to: 769-770, 2308-2317, 2324-2325
| // MySQL 8.0+ uses `transaction_isolation`; `tx_isolation` was | ||
| // deprecated in 8.0 and removed in 8.4 (so MySQL 9.x also | ||
| // requires the modern name). MariaDB (any version, including | ||
| // 11.x where atoi(server_version) >= 11) keeps `tx_isolation`, | ||
| // so the major-version check must exclude MariaDB explicitly. | ||
| const char *sv = mybe->server_myds->myconn->mysql->server_version; | ||
| bool is_mariadb = (sv != NULL && strstr(sv, "MariaDB") != NULL); | ||
| int sv_major = (sv != NULL) ? atoi(sv) : 0; | ||
| if (!is_mariadb && sv_major >= 8) { | ||
| sprintf(query,q,"transaction_isolation", var_value); | ||
| } else { |
There was a problem hiding this comment.
Buffer size is too small when rewriting tx_* names to transaction_*.
query is allocated using strlen(var_name) (Line 2605), but these branches may write longer names (transaction_isolation / transaction_read_only) into sprintf, which can overflow the heap buffer on MySQL 8+/9+ non-MariaDB paths.
💡 Suggested fix
- query=(char *)malloc(strlen(q)+strlen(var_name)+strlen(var_value));
+ size_t effective_var_name_len = strlen(var_name);
+ if (strncasecmp("tx_isolation", var_name, 12) == 0) {
+ effective_var_name_len = std::max(effective_var_name_len, strlen("transaction_isolation"));
+ } else if (strncasecmp("tx_read_only", var_name, 12) == 0) {
+ effective_var_name_len = std::max(effective_var_name_len, strlen("transaction_read_only"));
+ }
+ query=(char *)malloc(strlen(q) + effective_var_name_len + strlen(var_value));Also applies to: 2621-2626
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/MySQL_Session.cpp` around lines 2607 - 2617, The temp buffer `query`
(allocated from strlen(var_name)) is too small when you rewrite short `tx_*`
names to longer `transaction_*` names in MySQL_Session.cpp; update the code that
prepares `query` (used by sprintf when writing "transaction_isolation" /
"transaction_read_only") to allocate a safe size or use a bounded/size-aware
API: compute needed_size = strlen(q) + max_replacement_extra + 1 (or switch to
std::string) and replace sprintf(...) with snprintf(..., needed_size, ...) so
the buffer cannot overflow; reference symbols: query, var_name,
sprintf/snprintf, mybe->server_myds->myconn->mysql->server_version, is_mariadb,
sv_major, and the branches that write
"transaction_isolation"/"transaction_read_only".
The previous commit introduced local variable declarations with
initializers (`const char *sv = …`, `bool is_mariadb = …`, etc.)
directly inside `case MON_GALERA:` of the `switch` in
`init_async()` at lib/MySQL_Monitor.cpp:742. The sibling
`case MON_CLOSE_CONNECTION:` / `MON_CONNECT:` / `MON_AWS_AURORA:`
labels then jump past these initializations, which GCC rejects:
MySQL_Monitor.cpp:779:14: error: jump to case label
MySQL_Monitor.cpp:781:14: error: jump to case label
MySQL_Monitor.cpp:783:14: error: jump to case label
Wrap the `#else` body of the MON_GALERA case in `{ ... }` so the
declarations live in their own scope and no sibling case can jump
over them. No behavior change.
The async path at lib/MySQL_Monitor.cpp:2299 was already inside its
own `{ ... }` block (added with the original code, not by this
patch), so it compiled fine and needs no change. The
`MySQL_Session.cpp` edits live inside `if / else if` chains with
their own braces, also unaffected.
|
The admin connection used cl.host, which is the frontend endpoint. In infrastructures where the admin interface is on a different host the test would false-fail on the admin connect, unrelated to collation behavior. Use cl.admin_host. Addresses CodeRabbit review feedback on PR #5807.
1 participant
Summary
Fix a regression on
v3.0HEAD whereSET NAMES utf8mb4 COLLATE utf8mb4_0900_ai_ciis forwarded unmodified to MariaDB backends and fails withERROR 1273: Unknown collation: 'utf8mb4_0900_ai_ci'. The same bug is present onv3.1/v4.0HEAD and shipped in tagsv3.1.8/v4.0.8(not inv3.0.7/v3.0.8themselves).Root cause: commit
7c6f42bcd("fix: MySQL 9.x charset handling…") changed the version guard inlib/MySQL_Variables.cpp:validate_charset()fromserver_version[0] != '8'toatoi(server_version) < 8. The new form correctly extends id-255 support to MySQL 9.x but silently treats MariaDB 10.x / 11.x (atoi() == 10 / 11) as if they were MySQL ≥ 8, so the unsupported collation is no longer rewritten beforeSET NAMESis sent to the backend.server_version[0] != '8'atoi(...) < 85.7.x8.0.x9.x.y10.11.13-MariaDB11.x.x-MariaDBPrimary fix: detect MariaDB by the
"MariaDB"suffix inserver_versionand apply the samehandle_unknown_charsetreplacement as for pre-8.0 MySQL.Closes #5790. No backport — ships in 3.0.9 / 3.1.9 / 4.0.9.
Expanded scope — sibling sites with the same class of bug
Independent review (subagents) surfaced four other sites in core that do version detection by indexing or string-prefix-matching
server_version. Each misclassifies MariaDB, MySQL 9.x, or both. The twoMySQL_Monitor.cppsites are an active break on MySQL 9.x Galera/PXC (becauseINFORMATION_SCHEMA.GLOBAL_STATUSwas removed in MySQL 8.4); the twoMySQL_Session.cppsites are latent (MariaDB happens to fall on a still-working branch).All four are fixed in this PR using the same
is_mariadb + sv_majorpattern asvalidate_charset:lib/MySQL_Session.cpp:2607-2615strncmp(sv,"8",1)==0→transaction_isolation/transaction_read_only, elsetx_isolation/tx_read_onlytx_isolationremoved in 8.4 → SET fails!is_mariadb && sv_major >= 8→ modern name on MySQL 8.0+ (incl. 9.x); MariaDB keepstx_isolationlib/MySQL_Monitor.cpp:749(sync MON_GALERA)→performance_schema, elseINFORMATION_SCHEMA`lib/MySQL_Monitor.cpp:2297(async MON_GALERA)Commits
d8e51349e— primary fix invalidate_charset+ TAP regressionreg_test_5790-mariadb_collation_255-tregistered inmariadb10-galera-g1.79414c10f— Gemini review follow-up: null-safety guard onatoi(server_version)invalidate_charset.81bcc802d— extend the MariaDB / MySQL 9.x detection pattern to the four sibling sites above.Test plan
mariadb10-galera-g1runsreg_test_5790-mariadb_collation_255-tand passes.lib/MySQL_Variables.cppchange → confirmreg_test_5790-…-tfails with errno 1273 on MariaDB backend.test_utf8mb4_as_ci-4841-t(MySQL 8+ id-255 path) orcharset_unsigned_int-t.mariadb10-galera-g*) continue to pass — confirmsMySQL_Monitor.cppfix did not break the MariaDB Galera path that previously fell through the buggy condition by accident.mysql95-*/mysql93-*group with Galera orSET SESSION TRANSACTION ISOLATION LEVEL …coverage exercises theMySQL_Session.cpp/MySQL_Monitor.cppfixes on MySQL 9.x.Summary by CodeRabbit
Bug Fixes
Tests