Fix SQL_C_GUID parameter binding sending corrupted data on the wire (#295) - #296
Fix SQL_C_GUID parameter binding sending corrupted data on the wire (#295)#296fdcastel wants to merge 6 commits into
Conversation
|
Honestly, I studied this PR and didn't understand a thing. Could you describe it more clearly—where the error was, how to reproduce it, and what the purpose of these changes was?.. |
|
I understand the code switching for string/binary, it's okay. But what's the purpose of total rewriting of the convGuidToString() foo? And why the convGuidToStringW() is left untouched?.. |
a32a022 to
538a720
Compare
|
Thanks for the review — fair feedback. I rewrote the commit so the diff is easier to read; here's the walkthrough. The bug. An ODBC app calls
Issue #295 has the full empirical trace including the two reproducers above ( The fix shape, after the rewrite. The wire-side fix lives in two new functions:
The dispatch in
History is squashed to one commit; force-pushed with |
|
@irodushka Just a friendly ping on this PR when you have a chance. 🙂 This one (ideally together with #292 and #293) feels like a strong candidate for an rc2 release. That would let me finally close out the remaining DuckDB integration issue. After that, we could get back on track with #287. |
|
I'm lost in a tangle of different caliber troubles. Not related to FB or ODBC) Hardly doubt I'll be able to switch to those PRs this week, sorry( |
|
No worries. I know the feeling. 😅 Let me know when we can get back on track. |
|
@fdcastel found some time to throw some |
When an ODBC application called SQLBindParameter with C type SQL_C_GUID and SQL type SQL_GUID, the driver accepted the call but did not convert the 16-byte UUID into Firebird's wire format. Symptoms reported in FirebirdSQL#295: * BINARY(16) / CHAR(16) CHARACTER SET OCTETS targets received the ASCII bytes of the canonical UUID string truncated to 16 chars. * Untyped VARCHAR parameters (e.g. inside CHAR_TO_UUID(?)) received a wide-char buffer interpreted as narrow text, so Firebird raised "Human readable UUID argument for CHAR_TO_UUID must have hex digit at position 2 instead of ''". Add two wire-only conversion functions and route SQL_C_GUID input parameters to them: * convGuidToBinary writes 16 raw bytes in canonical UUID byte order (Data1/2/3 swapped from x86 little-endian to big-endian, Data4 as is). Used for BINARY(16) / CHAR(16) OCTETS / FB4+ BINARY targets. * convGuidToVarString stages the 36-char canonical UUID in the DescRecord local buffer and redirects the wire's sqldata via setSqlData(), matching the idiom transferStringToAllowedType already uses. This sidesteps the SQLDA-allocated buffer being only 2 bytes wide for an untyped `?` (VARCHAR(0)) placeholder. The pre-existing convGuidToString / convGuidToStringW remain unchanged; they handle the (currently unreachable) app-side fetch path that will become live with the column-side SQL_GUID mapping work tracked under FirebirdSQL#287 T5-5. Expose getSqlSubtype() and getSqlLen() as read-only accessors on HeadSqlVar so the dispatch in getAdressFunction can distinguish sqlsubtype == 1 + sqllen == 16 (BINARY/CHAR(16) OCTETS, raw bytes) from text wires of any other charset (canonical UUID string). Add three GuidParamBindingTest acceptance tests covering the issue's exact reproducers: SQL_C_GUID into CHAR(16) OCTETS, into VARCHAR via CHAR_TO_UUID(?), and into BINARY(16) via UUID_TO_CHAR(?). Closes FirebirdSQL#295.
595ffec to
84babbf
Compare
Test coverage: - BindGuidToVarcharOctets16 — binds SQL_C_GUID to VARCHAR(16) CHARACTER SET OCTETS (the wire form of native FB4+ VARBINARY(16)). The existing BindGuidToCharOctets16 only covered the fixed CHAR(16) (SQL_TEXT) slot; this exercises the SQL_VARYING (length-prefixed) binary wire, which was previously untested. Bug the new test exposed (and fixes): - For a SQL_VARYING binary wire, convGuidToBinary wrote 16 raw bytes at offset 0 with no 2-byte length prefix. Firebird then read the first GUID bytes as a VARYING length and over-read the buffer — a hard SEGFAULT on the FB6 snapshot (silently tolerated on FB5). The dispatch now calls setTypeText() for the OCTETS subtype-1/len-16 branch, mirroring the text branch: the varying wire becomes fixed SQL_TEXT (OCTETS charset preserved) so the offset-0 write is correct. A fixed CHAR(16) wire is already SQL_TEXT, so this is a no-op there. - The new test is SKIP_ON_FIREBIRD6-guarded: even with the prefix fix, the current FB6 master snapshot aborts this parameterized OCTETS-VARYING insert with a server-side "Stack overflow" — the same parameterized-query incompatibility already guarded across the suite. Exercised on FB 3/4/5. Dead-code removal: - Drop the wire-side `to->conciseType == SQL_C_BINARY` arm. Native FB4+ BINARY/VARBINARY reach the dispatch as subtype-1 / sqllen-16 OCTETS (caught by the branch above); the only type that yields conciseType SQL_C_BINARY is a binary BLOB, not a sane GUID-bind target. - Drop the app-side `case SQL_C_BINARY` arm. That output direction is unreachable until column-side SQL_GUID mapping (T5-5 / FirebirdSQL#287) lands. convGuidToBinary stays (still used by the OCTETS wire branch). Verified: all 4 GuidParamBindingTest cases pass on FB 5.0.3; on the FB6 snapshot the 3 non-varying cases pass and the VARBINARY case skips; full suite shows no regressions.
|
Took the "cover the code with tests first" advice — and it immediately paid off.
Rebased on current Whenever you have a moment, a review would be much appreciated 🙏 |
|
Hi @fdcastel Okay, but the most of my notes (7 to be exact) are still pending... |
? Sorry... What notes? |
|
These)) didn't commit the review, sorry |
…rror & buffer fixes Per irodushka's review (8 inline notes on PR FirebirdSQL#296): FirebirdSQL#1, dispatch structure - made the wire-side block fall through to an explicit `else` for the app-side switch (the wire-side branches already always-return, so this is purely structural clarity). FirebirdSQL#2/FirebirdSQL#3, "subtype==1 && sqllen==16" indistinct + hardcoded numbers - new inline accessors HeadSqlVar::isBinary() and isVarBinary() bundle the sqltype check (SQL_TEXT for BINARY, SQL_VARYING for VARBINARY) with the OCTETS-charset check, via a named CHARSET_OCTETS = 1 constant that documents the "for SQL_TEXT/SQL_VARYING, sqlsubtype is the CHARACTER SET id (charset 1 = OCTETS, per IscDbc/MultibyteConvert.cpp CODE_CHARSETS(OCTETS, 1, 1))" rule. Dispatch now reads: if ( getSqlLen() == GUID_BINARY_LEN && ( isBinary() || isVarBinary() ) ) GUID_BINARY_LEN (16) and GUID_STRING_LEN (36) named in OdbcConvert.h replace the inline 16/36/37 magic numbers in convGuidToBinary and convGuidToVarString. FirebirdSQL#5, convGuidToBinary silent truncation - changed the `outlen < 16` branch from a silent shortening + SQL_SUCCESS into a hard SQL_ERROR with diagnostic 22001 "String data, right truncation - GUID requires 16 bytes" via parentStmt->postError (same pattern the existing transferStringToAllowedType uses for 01004 truncation). Note in the comment that the dispatcher already enforces sqllen == 16, so this is defensive code for future callers. FirebirdSQL#6, hardcoded char tmp[37] - now char tmp[GUID_STRING_LEN + 1]. FirebirdSQL#7, convGuidToVarString buffer overflow - the previous code only allocated the local buffer when isLocalDataPtr was false, leaving a silent overflow path when a smaller default-sized buffer already existed (an untyped `?` placeholder is described by Firebird as VARCHAR(0), so DescRecord::allocateLocalDataPtr() with no length defaults to getBufferLength() = 1 byte). Dropped the guard and call allocateLocalDataPtr(GUID_STRING_LEN + 1) unconditionally; allocateLocalDataPtr() free()s any existing buffer first, so this is safe and always yields a 37-byte buffer. FirebirdSQL#4, dropped `to->conciseType == SQL_C_BINARY` arm - already done in the previous commit (84babbf) for the same reason irodushka gives; no code change in this commit. FirebirdSQL#8, "what about destination buffer length after setTypeText" - setTypeText() mutates the sqlvar so Sqlda::isExternalOverriden() fires and Sqlda::checkAndRebuild() allocates a fresh execBuffer from the new metadata; on top, convGuidToVarString redirects the wire's sqldata via setSqlData() to our oversized localDataPtr (now guaranteed by FirebirdSQL#7 to be 37 bytes), so the fresh exec buffer is bypassed entirely - the same idiom transferStringToAllowedType uses. No code change needed beyond FirebirdSQL#7. Verified: clean build (0 warnings); FB 5.0.3 runs all 4 GuidParamBindingTest cases green; FB 6 snapshot passes 3 and skips BindGuidToVarcharOctets16 (unchanged from 84babbf); full local suite 225 passed, 0 failed.
|
Pushed Verified locally: clean build (0 warnings), FB 5.0.3 → all 4 |
|
Aha... Now I got it - there was no separate field for charset in the ISC API, so subtype was used. and I think it's much more clear to specify |
…ame, cleanup Per irodushka's 2nd round on PR FirebirdSQL#296 (4 inline + 1 top-level): FirebirdSQL#1+FirebirdSQL#2, drop dead getSqlSubtype() - the only caller (the old dispatch check) is gone since the isBinary() / isVarBinary() refactor in 1571a97; verified by grep that nothing else references it. Drops the virtual decl in Connection.h and the inline impl in IscHeadSqlVar.h. FirebirdSQL#3, unify GUID_BINARY_LEN and GUID_STRING_LEN to int - one was int, the other size_t. Both are small positive byte/char counts used in mixed-signed arithmetic; consistent int matches the short->int promotion at the comparison sites and lets us drop the (int) casts inside convGuidToWireString. FirebirdSQL#4, fix the convGuidToVarString misnomer - rename to convGuidToWireString. The "Var" prefix suggested SQL_VARYING, but the function runs against a SQL_TEXT wire (setTypeText() runs in the dispatch right before the call). Rewrote the leading docstring to make the wire-vs-app distinction with convGuidToString explicit. FirebirdSQL#5 (issue comment 4561787816), switch isBinary() / isVarBinary() to check sqlvar->sqlcharset instead of sqlvar->sqlsubtype - cleaner. SqlProperties (IscDbc/Sqlda.h) carries the charset id in a dedicated sqlcharset field; sqlsubtype is documented as "BLOBs & Text types only" and is just overlaid with sqlcharset by CAttrSqlVar::bindProperties for compatibility. Reading the dedicated field eliminates the "subtype has two meanings" mental gymnastics noted in irodushka's round-1 thread. CHARSET_OCTETS is now `unsigned` to match the sqlcharset field type. Verified: clean build (0 warnings); FB 5.0.3 -> all 4 GuidParamBindingTest cases pass; FB6 snapshot -> 3 pass + BindGuidToVarcharOctets16 skipped (unchanged from 84babbf); full local suite 225 passed, 0 failed.
|
Done in |
|
Hi @fdcastel about #296 (comment) All these conv mechanix (OdbcConvert.cpp) is not mine, but I think I understand the initial idea. The switching is going primarily on This approach, primarily fine and solid, subsequently was repeatedly distorted by countless updates & fixes) I must think about it)) |
|
Agreed. That’s the core issue. You're right that the skeleton is "from-then-to": the outer On the symmetry point, I leaned on the existing pattern:
So the wire-vs-app split ( The reason the GUID wire routing can't be expressed as a pure Where I think your instinct is dead on: the GUID arm is the one place that puts |
|
The worst thing that annoys me is the copypaste in convGuidToString/convGuidToWireString. These foos are 95% identical, except:
Can you please explain me this: ? And the corresponding test is: ? I really wonder if this piece is correct. Will it work if you write it like Well, I can understand when you pass GUID in a text form (as CHAR(36)) to the parameter of SQL_GUID type. It's okay. But pass the 16-byte GUID directly to CHAR(36) column... Hmmm... So as for me - you're trying to make a workaround to serve an incorrect parameter binding. |
Per irodushka's review on PR FirebirdSQL#296: - Extract formatGuidCanonical() — both convGuidToString (app-side) and convGuidToWireString (wire-side) formatted the canonical 36-char UUID with their own copy of the 11-arg snprintf format string. One helper now owns the format; the two functions keep only their distinct destination handling (app buffer vs staged localDataPtr + setSqlData). - Fix the `if ( len == -1 ) len = outlen;` bug in convGuidToString. snprintf returning -1 is an encoding error; setting len to the full buffer size reported uninitialized bytes as valid data. A canonical GUID is always exactly GUID_STRING_LEN (36) chars, so anything else is a hard error now (SQLSTATE HY000), and a destination buffer too small for 36 + NUL raises 01004 truncation with the full length reported via the indicator — matching the ODBC "C to SQL: GUID" conversion contract. - Apply the same principle to convGuidToWireString: drop the clamp-and-continue (`srcLen > GUID_STRING_LEN ? ...`) and treat a non-36 format result as a hard error instead of silently proceeding. No behaviour change on the happy path; convGuidToString remains dormant (app-side fetch, pending T5-5 / FirebirdSQL#287). Verified: clean build (0 warnings); FB 5.0.3 all 4 GuidParamBindingTest pass; FB6 snapshot 3 pass + VARBINARY skip; full suite 225 passed, 0 failed.
|
Great catches! And one I want to (gently, with receipts 😄) push back on. Took them in order: 1. The
2. The copy-paste: also right, dealt with.
3. The macro zoo ( 4. "Workaround to serve an incorrect parameter binding": this is the one where I think the binding is actually correct, and I went and gathered more evidence. Follow on: 4.a) First, your own question "will it work if you write So your binding works, but... all four land in the same
4.b) Second, the spec actually defines exactly this. The ODBC "C to SQL: GUID" conversion table (learn.microsoft.com) lists 4.c) Third, the real-world reason this exists: duckdb/odbc-scanner binds exactly Net: I kept the conversion (the spec sanctions it, your own proposed binding exercises the same path), and folded in the bug fix + dedup you spotted. Totally open to being wrong if you read that conversion table differently 😉. But I think on this one the berries turned out to be cherries. 🍒 |
…idToString The SQL_C_GUID arm now switches on to->conciseType like its neighbours. CHAR/WCHAR targets take the text or raw-bytes path; anything else falls to notYetImplemented, restoring the 07006 for a GUID bound to a numeric or date parameter. convGuidToWireString is folded into convGuidToString, which branches on to->isIndicatorSqlDa only for the destination. Any OCTETS slot receives the 16 raw bytes (22001 when shorter than 16), so getSqlLen() is no longer needed. Both Firebird-side writers use ODBCCONVERT_CHECKNULL_SQLDA so a value bound after a NULL clears the null flag. Comments and tests now describe the actual causes of FirebirdSQL#295: under CHARSET=UTF8 the CHAR_TO_UUID(?) slot is CHAR(36) UTF8, which the driver maps to SQL_C_WCHAR, so the old dispatch wrote UTF-16 onto the wire; on Windows snprintf is _snprintf, which truncated the OCTETS(16) case and returned -1. There is no "VARCHAR(0)" placeholder; the staging buffer is needed because the described slot can be VARYING, shorter than 36 bytes, or wide-mapped. New tests: 07006 for an INTEGER target, 22001 for an undersized OCTETS slot, raw bytes for a VARBINARY(32) slot, and a value bound after a NULL.
|
Sorry for the delay in finishing what you asked for; other professional tasks took my available time. Pushed b303c08. What changed:
The "VARCHAR(0) / 1-byte buffer" explanation I gave earlier was wrong, and the comments now say what actually happens:
Verified locally on FB 5.0.3 with CHARSET=UTF8 and CHARSET=NONE: all 8 |
What this fixes
When an ODBC application calls
SQLBindParameter(SQL_C_GUID, SQL_GUID, …, ptr, 16, &len)and hands the driver a 16-byte UUID, the driver returnsSQL_SUCCESSbut Firebird never sees the 16 UUID bytes:30 33 30 36 43 31 37 36 2D 45 34 30 31 2D 31 31instead of the 16 raw UUID bytes).CHAR_TO_UUID(?)) under CHARSET=UTF8 — Firebird returnsexpression evaluation not supported — Human readable UUID argument for CHAR_TO_UUID must have hex digit at position 2 instead of "".Reproducers, both from issue #295:
Real-world impact: duckdb/odbc-scanner binds DuckDB
::UUIDvalues exactly this way (seesrc/types/uuid_type.cppL33-44). Their Firebird UUID round-trip test (duckdb/odbc-scanner#169) is parked on this.Closes #295.
Root cause
snprintfis_snprintf(seeOdbcJdbc.h), which truncates and returns -1; the legacylen == -1line then reported 16 bytes, so Firebird stored the ASCII of the first 16 chars.?inCHAR_TO_UUID(?)as CHAR(36) UTF8, 144 bytes. The driver maps UTF8 text to JDBC_WCHAR, so the dispatch pickedconvGuidToStringWand wrote UTF-16 onto the wire. Firebird sawA, NUL,0, NUL, … hence "hex digit at position 2". With CHARSET=NONE the slot is CHAR(36) ASCII and the old code passed on Windows by accident (36 chars fit_snprintfexactly, no terminator); C99snprintfwrites 35 chars plus NUL there.How the fix works
OdbcConvert::getAdressFunctionswitches on the target type first, like the neighbouring arms. For an input parameter described as CHAR/VARCHAR:convGuidToBinarywrites the 16 bytes in canonical UUID byte order (Data1/2/3 big-endian, Data4 as is). A slot shorter than 16 bytes is rejected with 22001.convGuidToStringstages the 36-char canonical UUID in the DescRecord local buffer and pointssqldataat it, the idiomtransferStringToAllowedTypeuses. The wire buffer cannot be written directly: it may be VARYING (length prefix), shorter than 36 bytes, or a UTF8 text the driver maps to SQL_C_WCHAR.setTypeText()makes the slot SQL_TEXT andSqlda::checkAndRebuild()rebuilds the message for the new length; Firebird reports the truncation itself when the slot is too small.convGuidToStringalso serves the application-side fetch path (SQLGetData(…, SQL_C_CHAR, …)from a column described as SQL_GUID, dormant until T5-5 / #287 lands); it branches onto->isIndicatorSqlDaonly for the destination.convGuidToStringWis unchanged. Both Firebird-side writers useODBCCONVERT_CHECKNULL_SQLDA, so a value bound after a NULL clears the null flag.IscHeadSqlVar::isBinary()/isVarBinary()read the dedicatedsqlcharsetfield to identify OCTETS slots.Tests
GuidParamBindingTestintests/test_guid_and_binary.cpp:BindGuidToCharOctets16,BindGuidToVarcharOctets16— SQL_C_GUID into CHAR(16) / VARCHAR(16) OCTETS columns, read back viaUUID_TO_CHAR.BindGuidToUuidToCharRoundtrip,BindGuidToVarcharViaCharToUuid— the two reproducers. The text one fails on the old code only under CHARSET=UTF8 (CI's primary run) or on Linux; the comment in the test says why.BindGuidToIntegerParamIsRejected(07006),BindGuidToUndersizedOctetsIsRejected(22001),BindGuidToOversizedOctetsStoresRawBytes(VARBINARY(32) receives 16 bytes),BindGuidValueAfterNull(a value bound after a NULL is sent).The two VARCHAR OCTETS tests skip on the FB6 snapshot, which aborts parameterized OCTETS-varying statements server-side, the same limitation guarded elsewhere in the suite.