Fix SQL_C_GUID parameter binding sending corrupted data on the wire (#295)#296
Open
fdcastel wants to merge 2 commits intoFirebirdSQL:masterfrom
Open
Fix SQL_C_GUID parameter binding sending corrupted data on the wire (#295)#296fdcastel wants to merge 2 commits intoFirebirdSQL:masterfrom
fdcastel wants to merge 2 commits intoFirebirdSQL:masterfrom
Conversation
SQLBindParameter with SQL_C_GUID was accepted, but the driver did not convert the C-side 16-byte GUID into Firebird's wire format. The data Firebird saw was either ASCII bytes of the canonical UUID truncated to 16 chars (BINARY(16) target), or wide-char UTF-16 interpreted as narrow text (VARCHAR target). Add convGuidToBinary that emits 16 bytes in canonical UUID byte order (Data1/2/3 swapped from x86 little-endian to big-endian, Data4 as-is) and rework the SQL_C_GUID dispatch in getAdressFunction to choose between binary and text wire formats based on the IPD's sqlsubtype/ sqllen rather than the application-side conciseType. For text wire, setTypeText() is now applied so SQL_VARYING is converted to SQL_TEXT before the canonical UUID is written without a length prefix; canonical UUIDs are pure ASCII so the same 36 narrow bytes work for UTF8 wires. Expose getSqlSubtype()/getSqlLen() on HeadSqlVar so the dispatch can detect CHAR(16) CHARACTER SET OCTETS / BINARY(16) targets. Add three GuidParamBindingTest acceptance tests that exercise the issue's 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.
Untyped `?` placeholders are described by Firebird as VARCHAR(0), so the wire-side SQLDA buffer holds at most 2 bytes (the length prefix) before the parameter type is bound. Writing 36 bytes of canonical UUID into that buffer overflowed on Linux/master builds (the failure surfaced as CHAR_TO_UUID seeing an empty argument). Switch convGuidToString in wire mode to allocate the DescRecord's local buffer, write into it, and redirect the wire's sqldata via setSqlData(). Same pattern transferStringToAllowedType already uses for app→wire string transfers, so checkAndRebuild copies our 36 bytes into the freshly-sized execBuffer regardless of the original SQLDA precision.
3 tasks
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
1 participant
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.
Summary
Fixes #295. When an ODBC application called
SQLBindParameter(SQL_C_GUID, SQL_GUID, ...)and provided a 16-byte UUID, the driver accepted the call but silently sent corrupt data on the wire — either ASCII bytes of the canonical UUID truncated to 16 chars (BINARY(16) target) or wide-char UTF-16 interpreted as narrow text (VARCHAR target).The fix:
convGuidToBinarythat emits 16 bytes in canonical UUID byte order (Data1/Data2/Data3 swapped from x86 little-endian to big-endian, Data4 unchanged).SQL_C_GUIDdispatch inOdbcConvert::getAdressFunctionto choose between binary and text wire formats based on the IPD's sqlsubtype/sqllen rather than the application-side conciseType.setTypeText()so SQL_VARYING is converted to SQL_TEXT, then writes the canonical UUID into the DescRecord's local buffer and redirects the wire'ssqldataviasetSqlData()— matching the patterntransferStringToAllowedTypealready uses, so we don't depend on the SQLDA-allocated buffer being large enough (an untyped?placeholder is described by Firebird as VARCHAR(0), so its wire buffer is essentially nothing).getSqlSubtype()/getSqlLen()onHeadSqlVarso the dispatch can detect CHAR(16) CHARACTER SET OCTETS / BINARY(16) targets reliably.This is the parameter-binding piece of #287 T5-5; the column-side SQL_GUID type mapping (SQLDescribeCol returning SQL_GUID for CHAR(16) OCTETS columns) is still open and out of scope here.
Tests
Three new acceptance tests in
tests/test_guid_and_binary.cppexercise the issue's exact reproducers:GuidParamBindingTest.BindGuidToCharOctets16— SQL_C_GUID into CHAR(16) CHARACTER SET OCTETS, round-tripped viaUUID_TO_CHAR.GuidParamBindingTest.BindGuidToVarcharViaCharToUuid— SQL_C_GUID into VARCHAR viaCHAR_TO_UUID(?)(reproducer 2 from the issue).GuidParamBindingTest.BindGuidToUuidToCharRoundtrip— SQL_C_GUID into BINARY(16) viaUUID_TO_CHAR(?)(reproducer 1).All three tests pass on the FB master matrix jobs (Windows x64/x86/ARM64, Ubuntu x64/ARM64). Existing test suite (202 active tests) shows zero regressions locally.