Client#prepare computes the connection-encoding conversion and then discards it — statements are sent in the wrong encoding - #1455
Merged
Conversation
rb_mysql_stmt_new converts the SQL to the connection encoding and then takes its pointer and length from the ORIGINAL string, so the conversion is computed and discarded. client.c:877-879 does the same thing correctly on the query path; this makes prepare match it.
Client#prepare computes the connection-encoding conversion and then discards it — statements are sent in the wrong encoding
Collaborator
|
Wow, that's been there a while! Nice catch! |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is the two-line change; the evidence is below so it can be checked rather than taken on trust.
Summary
rb_mysql_stmt_newtranscodes the SQL string to the connection encoding, then handsmysql_stmt_preparethe pointer and length of the original, un-transcoded string. Theconverted string is computed, stored in
args.sql, and never used. So when the SQL string'sencoding differs from the connection encoding,
Client#preparesends the wrong bytes whilecharacter_set_clientsays otherwise — andClient#query, given the identical string on theidentical connection, sends the right ones.
The result is silent: no error, no warning, just different data.
client.query("SELECT 'é'…")returns
éandclient.prepare("SELECT 'é'…").executereturnsé.This is a correctness bug, not a memory-safety bug —
sql_ptrandsql_lenare both takenfrom the original string, so they are consistent with each other and there is no over-read. I
checked that by execution rather than assuming it; details below.
It is reachable, not latent:
Client#prepareis public API, and ActiveRecord's mysql2 adapterreaches it for any query carrying binds. But it only bites when the statement text contains
non-ASCII and the string's encoding differs from the connection's — pure-ASCII SQL is
byte-identical in every ASCII-compatible encoding, which is presumably why this has gone unnoticed
since 2012.
Reproduction
Standalone; needs only a MySQL server. The control is
Client#queryon the identical string,so the two lines that differ are the whole finding.
HEX()is evaluated server-side, so it reports what MySQL received, not what Ruby sent.3/3 on every run. Applying the fix below makes both lines read
41E9, 3/3.Confirmed independently against the server's general log, which records the statement byte-exact:
Query41 E9Prepare41 C3 A9Prepare, patched41 E9The prepared statement's length is the original string's
bytesize, confirming both pointer andlength come from the pre-conversion string.
It can change SQL parse structure, not only produce mojibake
Worth knowing before deciding severity. In the UTF-8-string/latin1-connection direction above the
damage is confined to wrong characters — every byte of a UTF-8 multibyte sequence is ≥ 0x80 and
latin1 maps each to a printable character, so
'and\can never be synthesised and quotingstays intact.
But it is not confined in the other direction. Shift_JIS has characters whose trail byte is an
ASCII backslash — U+8868 「表」 is
95 5C. Withencoding: "utf8mb4"and a Shift_JIS SQL string:prepareships the raw95 5C; the utf8mb4 parser reads the5Cas an escape and swallows theclosing quote, so the literal does not terminate where the author wrote it. Here that lands on a
syntax error, but the mechanism is a quote-context change, i.e. the classic charset-mismatch shape.
I do not think this makes it a security issue, and I have not filed it as one: what selects the
mismatch is the statement text's encoding plus the connection option, both constants in
application source/config, and bound parameter values — where untrusted data actually lives —
go through
rb_mysql_stmt_execute, which converts and uses the converted string correctly.Flagging the reasoning explicitly so you can disagree with it rather than reconstruct it.
Cause
ext/mysql2/statement.c,rb_mysql_stmt_new—:168-171. The comment states theintent that the next two lines defeat:
rb_str_export_to_encreturns a new String when a conversion is needed, leavingsqluntouched — so
args.sqlis the only place the converted bytes exist, and it is the one thing notpassed to
mysql_stmt_prepare.The sibling query path in the same repo does it correctly —
ext/mysql2/client.c:865-867:Provenance
Introduced in
162e8e7ae("utf8 fieldnames", 2012-07-29), the commit that split the singleargs.sqlfield intosql+sql_ptr/sql_len:The intent is legible in the diff itself:
args.sqlwas introduced to be the buffer. It lookslike a straightforward slip that pure-ASCII SQL has hidden ever since.
git log --all -S "args.sql_ptr" -- ext/mysql2/statement.creturns exactly that one commit — theline has not been touched since.
Not a memory-safety bug — checked, not assumed
sql_ptrandsql_lenboth come fromsql, so they agree with each other andmysql_stmt_preparereads exactlyRSTRING_LEN(sql)live bytes. I verified this with theserver's general log as the detector, using a deliberately-broken build as a positive control
(
ptrfrom the converted string,lenfrom the original, over a statement padded to a 200-byteover-read):
Mysql2::Errorsyntax error — 200 bytes of heap the app never wrote arrive at the serverbytesizebytes received, zero past the end of the SQLSo the detector demonstrably can see an over-read, and this defect does not produce one.
I also checked the lifetime question —
args.sqlholds the converted string, so nothing in thestruct anchors the original across the GVL-releasing call. 3,000 prepares under
GC.stress, plus240 prepares racing 39,291
GC.compacts from a second thread, across both embedded andnon-embedded String slot classes: 0 anomalies,
GC.verify_compaction_referencesclean. As apositive control that the harness can actually fail, the known
fieldTypesunmarked-VALUEbug(#1453) aborts 3/3 through it on the same build.
sqlis a method argument live on the VM stackand the nogvl thread's machine stack is scanned conservatively, so it is pinned — the code is
correct here by luck of the calling convention rather than by design, but it is correct.
Affected versions
Every release that has prepared statements is affected — 0.4.0 through 0.5.7, and
master.I checked each tag mechanically rather than bisecting:
master@ 2026-08-06args.sql_ptr = RSTRING_PTR(sql)at 0.4.0:145)statement.c— prepared statements did not existNo version in the affected range is safe, incidentally or otherwise. Anything whose connection
encoding happens to equal the Ruby string encoding is spared by configuration, not by code —
the defect is still present.
Suggested fix
Two lines, matching the idiom already used at
client.c:866-867:args.sqlmust keep holding the converted String — it is what keeps it reachable for the GCacross
rb_thread_call_without_gvl.Yes, I ran it. Rebuilt from source with only this change and re-ran the reproducer:
prepareand
queryagree, 3/3, on both 0.5.6 and a 0.5.4-based tree. The general log shows both pathssending byte-identical statements.
A regression test would be the reproducer above: connect with
encoding: "latin1", prepare astatement whose text contains a non-ASCII character, and assert the round-trip matches
Client#query. Asserting the conversion is real first (sql.encode(...) != sql) matters — with amatching connection encoding the test passes vacuously.
Environment
Printed from the running process, not from memory:
The verdict depends only on
encoding:differing from the Ruby string's encoding — that is thewhole precondition. It does not depend on the server version, the client library, or the
platform: the divergence is created entirely on the Ruby side, in
rb_mysql_stmt_new, before anybytes reach the wire. I note the MariaDB connector explicitly because it is what was linked, not
because it is implicated —
mysql_stmt_prepareis handed a pointer and a length and faithfullysends what it is given, which is the whole problem.