This is an automated email from the ASF dual-hosted git repository.
lidavidm pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push:
new 53416d1d713 GH-50852: [C++][FlightRPC][ODBC] Decode wide
connection/descriptor string attributes with the wide decoder (#50853)
53416d1d713 is described below
commit 53416d1d713acba688951486c9b72cc58792c3a1
Author: Vikrant Puppala <[email protected]>
AuthorDate: Fri Oct 16 06:34:35 2026 +0530
GH-50852: [C++][FlightRPC][ODBC] Decode wide connection/descriptor string
attributes with the wide decoder (#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.
- `odbc_descriptor_test.cc` (new): add
`ODBCDescriptorTest.SetGetNameWideRoundTrips`,
a `SetField`/`GetField` round-trip on `SQL_DESC_NAME` with a
multi-character
wide name. There is no `SQLSetDescField` entry point in this tree, so this
covers the descriptor change with a direct unit test on `ODBCDescriptor`.
### Are these changes tested?
Yes. Each change has a round-trip test that distinguishes the fixed and
unfixed
code:
| check | result |
|---|---|
| catalog test, **with** fix (mock fixture) | PASS — `out_catalog ==
"my_catalog"` |
| catalog test, **without** fix (mock fixture) | FAIL — `out_catalog ==
"m\0y\0_\0c\0a\0t\0a\0l\0o\0g\0"` |
| descriptor test, **with** fix | PASS — name reads back as `"my_column"` |
| descriptor test, **without** fix | FAIL — byte-wise decode corrupts the
stored UTF-16 |
| `arrow-odbc-spi-impl-test` (unit suite) | 85/85 pass |
| `ConnectionAttributeTest/0.*` (mock suite) | 25/25 pass |
| clang-format | clean on all changed files |
The connection catalog test uses the mock fixture
(`FlightSQLODBCMockTestBase`,
an in-process SQLite Flight SQL server); its remote variant runs when
`ARROW_FLIGHT_SQL_ODBC_CONN` is set. The descriptor test is a standalone
unit
test. Verified locally against the mock fixture and the unit suite.
### 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._
* GitHub Issue: #50852
Lead-authored-by: Vikrant Puppala <[email protected]>
Co-authored-by: Isaac
Signed-off-by: David Li <[email protected]>
---
.../arrow/flight/sql/odbc/odbc_impl/CMakeLists.txt | 1 +
.../flight/sql/odbc/odbc_impl/odbc_connection.cc | 4 +-
.../flight/sql/odbc/odbc_impl/odbc_descriptor.cc | 2 +-
.../sql/odbc/odbc_impl/odbc_descriptor_test.cc | 53 ++++++++++++++++++++++
.../flight/sql/odbc/tests/connection_attr_test.cc | 25 ++++++++++
5 files changed, 82 insertions(+), 3 deletions(-)
diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/CMakeLists.txt
b/cpp/src/arrow/flight/sql/odbc/odbc_impl/CMakeLists.txt
index 4d990b3e89a..17d2ee1ffee 100644
--- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/CMakeLists.txt
+++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/CMakeLists.txt
@@ -206,6 +206,7 @@ add_arrow_test(odbc_spi_impl_test
json_converter_test.cc
record_batch_transformer_test.cc
util_test.cc
+ odbc_descriptor_test.cc
EXTRA_LINK_LIBS
${ODBC_SPI_IMPL_TEST_EXTRA_LIBS}
STATIC_LINK_LIBS
diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_connection.cc
b/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_connection.cc
index 142dac53ab6..e452008b27b 100644
--- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_connection.cc
+++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_connection.cc
@@ -465,9 +465,9 @@ void ODBCConnection::SetConnectAttr(SQLINTEGER attribute,
SQLPOINTER value,
case SQL_ATTR_CURRENT_CATALOG: {
std::string catalog;
if (is_unicode) {
- SetAttributeUTF8(value, string_length, catalog);
- } else {
SetAttributeSQLWCHAR(value, string_length, catalog);
+ } else {
+ SetAttributeUTF8(value, string_length, catalog);
}
if (!spi_connection_->SetAttribute(Connection::CURRENT_CATALOG,
catalog)) {
throw DriverException("Option value changed.", "01S02");
diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_descriptor.cc
b/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_descriptor.cc
index 11e4512eb8d..d58d8b93ba0 100644
--- a/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_descriptor.cc
+++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_descriptor.cc
@@ -215,7 +215,7 @@ void ODBCDescriptor::SetField(SQLSMALLINT record_number,
SQLSMALLINT field_ident
has_bindings_changed_ = true;
break;
case SQL_DESC_NAME:
- SetAttributeUTF8(value, buffer_length, record.name);
+ SetAttributeSQLWCHAR(value, buffer_length, record.name);
has_bindings_changed_ = true;
break;
case SQL_DESC_OCTET_LENGTH:
diff --git a/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_descriptor_test.cc
b/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_descriptor_test.cc
new file mode 100644
index 00000000000..f11711497cf
--- /dev/null
+++ b/cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_descriptor_test.cc
@@ -0,0 +1,53 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "arrow/flight/sql/odbc/odbc_impl/odbc_descriptor.h"
+
+#include "arrow/flight/sql/odbc/odbc_impl/diagnostics.h"
+#include "arrow/flight/sql/odbc/odbc_impl/encoding.h"
+#include "arrow/flight/sql/odbc/odbc_impl/encoding_utils.h"
+
+#include <sql.h>
+#include <sqlext.h>
+
+#include "gtest/gtest.h"
+
+namespace arrow::flight::sql::odbc {
+
+using ODBC::ODBCDescriptor;
+
+TEST(ODBCDescriptorTest, SetGetNameWideRoundTrips) {
+ arrow::flight::sql::odbc::Diagnostics diagnostics("vendor", "component",
+ OdbcVersion::V_3);
+ ODBCDescriptor desc(diagnostics, nullptr, nullptr,
/*is_app_descriptor=*/true,
+ /*is_writable=*/true, /*is_2x_connection=*/false);
+
+ desc.SetHeaderField(SQL_DESC_COUNT, reinterpret_cast<SQLPOINTER>(1), 0);
+
+ std::vector<uint8_t> wide;
+ Utf8ToWcs("my_column", &wide);
+ desc.SetField(1, SQL_DESC_NAME, wide.data(),
static_cast<SQLINTEGER>(wide.size()));
+
+ SQLWCHAR out[256];
+ SQLINTEGER out_len = 0;
+ desc.GetField(1, SQL_DESC_NAME, out, sizeof(out), &out_len);
+ std::string name =
+ ODBC::SqlWcharToString(out, static_cast<SQLSMALLINT>(out_len /
GetSqlWCharSize()));
+ EXPECT_EQ("my_column", name);
+}
+
+} // namespace arrow::flight::sql::odbc
diff --git a/cpp/src/arrow/flight/sql/odbc/tests/connection_attr_test.cc
b/cpp/src/arrow/flight/sql/odbc/tests/connection_attr_test.cc
index 65aadf1d8f9..07a5517ecef 100644
--- a/cpp/src/arrow/flight/sql/odbc/tests/connection_attr_test.cc
+++ b/cpp/src/arrow/flight/sql/odbc/tests/connection_attr_test.cc
@@ -410,4 +410,29 @@ TYPED_TEST(ConnectionAttributeTest,
TestSQLSetConnectAttrPacketSizeValid) {
#endif
}
+// A multi-character catalog set through the wide entry point must round-trip
+// intact.
+TYPED_TEST(ConnectionAttributeTest,
TestSQLSetGetConnectAttrCurrentCatalogWide) {
+ ASSIGN_SQLWCHAR_ARR_AND_LEN(catalog, L"my_catalog");
+
+ ASSERT_EQ(SQL_SUCCESS, SQLSetConnectAttr(this->conn,
SQL_ATTR_CURRENT_CATALOG, catalog,
+ catalog_len * GetSqlWCharSize()));
+
+ SQLWCHAR out_str[kOdbcBufferSize];
+ SQLINTEGER out_str_len;
+ ASSERT_EQ(SQL_SUCCESS, SQLGetConnectAttr(this->conn,
SQL_ATTR_CURRENT_CATALOG, out_str,
+ kOdbcBufferSize, &out_str_len));
+ // SQLGetConnectAttr returns the length in bytes; convert to characters.
+ out_str_len /= GetSqlWCharSize();
+ std::string out_catalog =
+ ODBC::SqlWcharToString(out_str, static_cast<SQLSMALLINT>(out_str_len));
+ EXPECT_EQ("my_catalog", out_catalog);
+
+ // Connections are shared across test suites, so restore the default catalog.
+ ASSIGN_SQLWCHAR_ARR_AND_LEN(empty_catalog, L"");
+ ASSERT_EQ(SQL_SUCCESS,
+ SQLSetConnectAttr(this->conn, SQL_ATTR_CURRENT_CATALOG,
empty_catalog,
+ empty_catalog_len * GetSqlWCharSize()));
+}
+
} // namespace arrow::flight::sql::odbc
