Copilot commented on code in PR #49564:
URL: https://github.com/apache/arrow/pull/49564#discussion_r2992111726
##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/attribute_utils.h:
##########
@@ -158,11 +158,21 @@ inline void SetAttributeSQLWCHAR(SQLPOINTER new_value,
SQLINTEGER input_length_i
thread_local std::vector<uint8_t> utf8_str;
if (input_length_in_bytes == SQL_NTS) {
arrow::flight::sql::odbc::WcsToUtf8(new_value, &utf8_str);
+ } else if (input_length_in_bytes <= 0) {
+ // empty string
+ attribute_to_write.clear();
+ return;
} else {
arrow::flight::sql::odbc::WcsToUtf8(
new_value, input_length_in_bytes /
arrow::flight::sql::odbc::GetSqlWCharSize(),
&utf8_str);
}
+
+ // add null-terminator
+ if (utf8_str.back() != '\0') {
+ utf8_str.push_back('\0');
+ }
Review Comment:
`SetAttributeSQLWCHAR` unconditionally checks `utf8_str.back()` to append a
NUL terminator. For an empty input string with `input_length_in_bytes ==
SQL_NTS`, `WcsToUtf8()` can produce an empty `utf8_str`, making `back()`
undefined behavior. Guard against `utf8_str.empty()` before accessing `back()`
(and consider always ensuring the conversion helpers include a terminator).
##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/CMakeLists.txt:
##########
@@ -130,20 +130,26 @@ if(WIN32)
win_system_dsn.cc)
endif()
-if(APPLE)
+if(WIN32)
+ find_package(ODBC REQUIRED)
+ target_include_directories(arrow_odbc_spi_impl PUBLIC ${ODBC_INCLUDE_DIR})
+ target_link_libraries(arrow_odbc_spi_impl
+ PUBLIC arrow_flight_sql_shared arrow_compute_shared
Boost::locale
+ ${ODBCINST})
+else()
+ # Unix
target_include_directories(arrow_odbc_spi_impl SYSTEM BEFORE PUBLIC
${ODBC_INCLUDE_DIR})
target_link_libraries(arrow_odbc_spi_impl
PUBLIC arrow_flight_sql_static
arrow_compute_static
Boost::locale
Boost::headers
RapidJSON)
-else()
- find_package(ODBC REQUIRED)
- target_include_directories(arrow_odbc_spi_impl PUBLIC ${ODBC_INCLUDE_DIR})
- target_link_libraries(arrow_odbc_spi_impl
- PUBLIC arrow_flight_sql_shared arrow_compute_shared
Boost::locale
- ${ODBCINST})
+
+ if(NOT APPLE)
+ # Explicitly link to unix-odbc on Linux
+ target_link_libraries(arrow_odbc_spi_impl PUBLIC ${ODBCINST})
+ endif()
Review Comment:
On non-Windows builds this target still uses ODBC variables/targets (e.g.
`${ODBC_INCLUDE_DIR}`) but `find_package(ODBC REQUIRED)` is now only called
under `WIN32`. This will leave `${ODBC_INCLUDE_DIR}`/`ODBC::ODBC` undefined on
Linux/macOS and should break configuration or compilation. Move
`find_package(ODBC REQUIRED)` outside the `WIN32` conditional (or add a Unix
branch) and link/include using the imported target consistently.
##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h:
##########
@@ -19,17 +19,29 @@
#include "arrow/flight/sql/odbc/odbc_impl/encoding.h"
#include "arrow/flight/sql/odbc/odbc_impl/platform.h"
-#include <sql.h>
-#include <sqlext.h>
+#include "arrow/flight/sql/odbc/odbc_impl/odbc_includes.h"
+
#include <algorithm>
#include <codecvt>
#include <cstring>
#include <locale>
#include <memory>
#include <string>
+// Include fwd.h headers after ODBC headers
+#include "arrow/flight/sql/odbc/odbc_impl/util.h"
+
#define _SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING
+#ifdef __linux__
+# define GET_SQWCHAR_PTR(wstring_var)
(ODBC::ToSqlWCharVector(wstring_var).data())
+#else
+// Windows and macOS
+# define GET_SQWCHAR_PTR(wstring_var) (wstring_var.c_str())
+#endif
+
+#define ODBC_INI
reinterpret_cast<LPCWSTR>(GET_SQWCHAR_PTR(std::wstring(L"ODBC.INI")))
Review Comment:
`GET_SQWCHAR_PTR` returns `.data()` from a temporary `std::vector<SQLWCHAR>`
on Linux. Even though the pointer survives through the immediate call
expression, it’s easy to accidentally use it beyond the full-expression, and it
also hides an allocation/conversion at each use. Consider returning a stable
buffer (e.g., thread_local vector) or refactoring to pass a
`std::vector<SQLWCHAR>`/`std::basic_string<SQLWCHAR>` object with well-defined
lifetime into the call site instead of a raw pointer.
##########
cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding_utils.h:
##########
@@ -117,4 +129,27 @@ inline std::string SqlStringToString(const unsigned char*
sql_str,
return res;
}
+
+inline std::vector<SQLWCHAR> ToSqlWCharVector(const std::wstring& ws) {
+ switch (arrow::flight::sql::odbc::GetSqlWCharSize()) {
+ case sizeof(wchar_t): {
+ return std::vector<SQLWCHAR>(ws.begin(), ws.end());
+ }
+#ifdef __linux__
+ case sizeof(char16_t): {
+ // Linux ODBC driver manager uses char16_t as SQLWCHAR
+ CONVERT_UTF8_STR(const std::string utf8s, ws);
+ CONVERT_UTF16_STR(const std::u16string utf16s, utf8s);
+ return std::vector<SQLWCHAR>(utf16s.begin(), utf16s.end());
Review Comment:
`ToSqlWCharVector()` builds a `std::vector<SQLWCHAR>` from
`ws.begin()/end()` (and similarly from `utf16s.begin()/end()`), which omits the
required NUL terminator. Call sites like
`SQLRemoveDSNFromIni(GET_SQWCHAR_PTR(...))` and
`SQLGetPrivateProfileString(..., ODBC_INI)` expect NUL-terminated strings, so
this can lead to reading past the buffer / undefined behavior. Ensure the
returned vector is NUL-terminated (e.g., append a `0` element).
```suggestion
std::vector<SQLWCHAR> result(ws.begin(), ws.end());
// Ensure NUL termination for APIs expecting a null-terminated
SQLWCHAR string.
result.push_back(0);
return result;
}
#ifdef __linux__
case sizeof(char16_t): {
// Linux ODBC driver manager uses char16_t as SQLWCHAR
CONVERT_UTF8_STR(const std::string utf8s, ws);
CONVERT_UTF16_STR(const std::u16string utf16s, utf8s);
std::vector<SQLWCHAR> result(utf16s.begin(), utf16s.end());
// Ensure NUL termination for APIs expecting a null-terminated
SQLWCHAR string.
result.push_back(0);
return result;
```
--
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]
