vikrantpuppala opened a new pull request, #50853:
URL: https://github.com/apache/arrow/pull/50853
### Rationale for this change
Two string attributes in the ODBC driver are decoded with the byte-wise
decoder even when the value arrives through a wide (Unicode / `*W`) entry
point,
so the wide buffer is misread and corrupted.
`ODBCConnection::SetConnectAttr(SQL_ATTR_CURRENT_CATALOG)` had its two decode
branches swapped:
```cpp
if (is_unicode) {
SetAttributeUTF8(value, string_length, catalog); // byte-wise
} else {
SetAttributeSQLWCHAR(value, string_length, catalog); // wide
}
```
`is_unicode` selects the buffer **width**: a unicode (`*W`) call passes a
wide
`SQLWCHAR` buffer that must be decoded with `SetAttributeSQLWCHAR`; a
non-unicode
call passes a byte string decoded with `SetAttributeUTF8`. With the branches
swapped, a wide catalog name (e.g. UTF-16 `"my_catalog"`) is reinterpreted
byte-wise and stored with the wide encoding's embedded NULs
(`"m\0y\0_\0c\0a\0t\0a\0l\0o\0g\0"`), so catalog scoping operates on a
corrupt
name. This is the inverse of the mapping the getter already uses:
`GetStringAttribute` maps `is_unicode -> GetAttributeSQLWCHAR`.
`ODBCDescriptor::SetField(SQL_DESC_NAME)` is the same class of bug: it
unconditionally used the byte-wise `SetAttributeUTF8`, even though the
matching
getter (`GetField` / `SQL_DESC_NAME`) reads the field back with
`GetAttributeSQLWCHAR`.
**How this happened (history):**
- The `SQL_ATTR_CURRENT_CATALOG` branches were swapped from the start, in the
original driver import (GH-46522, #40939). Note the getter side was written
against the shared `GetStringAttribute` helper (which decides width in one
place and got it right), while the setter open-coded the branch inline and
inverted the polarity — there is no matching `SetStringAttribute` helper.
- The descriptor `SQL_DESC_NAME` case was originally *consistent* (getter and
setter both byte-wise). GH-47721 (#48050) later migrated the descriptor
string getters to the wide `GetAttributeSQLWCHAR` for correct Unicode
column
attributes, but left the `SQL_DESC_NAME` setter on the byte-wise decoder,
creating the asymmetry.
### What changes are included in this PR?
- `odbc_connection.cc`: swap the `SQL_ATTR_CURRENT_CATALOG` decode branches
so a
unicode call uses `SetAttributeSQLWCHAR` and a non-unicode call uses
`SetAttributeUTF8`, matching `GetStringAttribute`.
- `odbc_descriptor.cc`: decode `SQL_DESC_NAME` with `SetAttributeSQLWCHAR` to
match its getter.
- `connection_attr_test.cc`: add
`TestSQLSetGetConnectAttrCurrentCatalogWide`, a
`TYPED_TEST` (mock + remote fixtures) that sets a multi-character catalog
through the wide entry point and asserts the full name round-trips back.
### Are these changes tested?
Yes. The new test is a set-then-get round-trip through the real driver stack
(`SQLSetConnectAttr` / `SQLGetConnectAttr`), placed next to the other
connection
attribute tests, and it distinguishes the fixed and unfixed driver:
| check | result |
|---|---|
| new test, **with** fix (mock fixture) | PASS — `out_catalog ==
"my_catalog"` |
| new test, **without** fix (mock fixture) | FAIL — `out_catalog ==
"m\0y\0_\0c\0a\0t\0a\0l\0o\0g\0"` |
| `ConnectionAttributeTest/0.*` (full mock suite) | 25/25 pass |
| clang-format | clean on all three files |
The mock fixture (`FlightSQLODBCMockTestBase`) runs against an in-process
SQLite
Flight SQL server; the remote variant runs when `ARROW_FLIGHT_SQL_ODBC_CONN`
is
set. Verified locally against the mock fixture.
### Are there any user-facing changes?
Yes. A catalog name (and descriptor `SQL_DESC_NAME`) set through the wide
entry
point is now decoded correctly instead of being corrupted. Applications that
set
a multi-character catalog through `SQLSetConnectAttrW` now scope to the
intended
catalog.
### AI usage
Per the [AI-generated code
guidance](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code):
the diagnosis, the fix, and the test were produced with the assistance of an
AI
coding agent and reviewed and verified by me. Correctness was checked by
(1) confirming the getter path (`GetStringAttribute`) maps `is_unicode` to
the
wide decoder, establishing the intended mapping the setters violated, and
(2) running the new round-trip test against both the fixed and unfixed
driver to
confirm it distinguishes them (clean name vs. embedded-NUL corruption).
---
_🤖 Drafted by Claude Code (an AI agent) and reviewed & verified by
vikrantpuppala._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
