Copilot commented on code in PR #49668:
URL: https://github.com/apache/arrow/pull/49668#discussion_r3089501546
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc:
##########
@@ -808,26 +835,28 @@ TYPED_TEST(ConnectionInfoTest,
TestSQLGetInfoInsertStatement) {
}
TYPED_TEST(ConnectionInfoTest, TestSQLGetInfoIntegrity) {
- SQLWCHAR value[kOdbcBufferSize] = L"";
- GetInfo(conn, SQL_INTEGRITY, value);
+ SQLWCHAR value[kOdbcBufferSize] = {};
+ GetInfoSQLWCHAR(conn, SQL_INTEGRITY, value);
- EXPECT_STREQ(static_cast<const SQLWCHAR*>(L"N"), value);
+ std::wstring result = ConvertToWString(value);
+ EXPECT_EQ(std::wstring(L"N"), result);
}
TYPED_TEST(ConnectionInfoTest, TestSQLGetInfoKeywords) {
// Keyword strings can require 10000 buffer length
static constexpr int info_len = kOdbcBufferSize * 10;
- SQLWCHAR value[info_len] = L"";
- GetInfo(conn, SQL_KEYWORDS, value, info_len);
+ SQLWCHAR value[info_len] = {};
+ GetInfoSQLWCHAR(conn, SQL_KEYWORDS, value, info_len);
- EXPECT_GT(wcslen(value), 0);
+ EXPECT_GT(wcslen(reinterpret_cast<wchar_t*>(value)), 0);
Review Comment:
Using wcslen(reinterpret_cast<wchar_t*>(value)) is undefined on platforms
where SQLWCHAR is not wchar_t (e.g., Linux with UTF-16 SQLWCHAR). This can read
past the buffer or compute an incorrect length. Prefer SqlWCharArrLen(value)
(or ConvertToWString(value).length()) for the non-empty assertion, especially
since this buffer can be much larger than kOdbcBufferSize.
```suggestion
EXPECT_GT(ConvertToWString(value).length(), 0U);
```
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc:
##########
@@ -382,40 +390,43 @@ TYPED_TEST(ConnectionInfoTest,
TestSQLGetInfoStaticCursorAttributes2) {
// DBMS Product Information
TYPED_TEST(ConnectionInfoTest, TestSQLGetInfoDatabaseName) {
- SQLWCHAR value[kOdbcBufferSize] = L"";
- GetInfo(conn, SQL_DATABASE_NAME, value);
+ SQLWCHAR value[kOdbcBufferSize] = {};
+ GetInfoSQLWCHAR(conn, SQL_DATABASE_NAME, value);
- EXPECT_STREQ(static_cast<const SQLWCHAR*>(L""), value);
+ std::wstring result = ConvertToWString(value);
+ EXPECT_EQ(std::wstring(L""), result);
}
TYPED_TEST(ConnectionInfoTest, TestSQLGetInfoDbmsName) {
- SQLWCHAR value[kOdbcBufferSize] = L"";
- GetInfo(conn, SQL_DBMS_NAME, value);
+ SQLWCHAR value[kOdbcBufferSize] = {};
+ GetInfoSQLWCHAR(conn, SQL_DBMS_NAME, value);
- EXPECT_GT(wcslen(value), 0);
+ EXPECT_GT(wcslen(reinterpret_cast<wchar_t*>(value)), 0);
}
Review Comment:
Using wcslen(reinterpret_cast<wchar_t*>(value)) is undefined on platforms
where SQLWCHAR is not wchar_t (e.g., Linux with UTF-16 SQLWCHAR). This can read
past the buffer or compute an incorrect length. Prefer SqlWCharArrLen(value) or
ConvertToWString(value).length() for the non-empty assertion.
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc:
##########
@@ -382,40 +390,43 @@ TYPED_TEST(ConnectionInfoTest,
TestSQLGetInfoStaticCursorAttributes2) {
// DBMS Product Information
TYPED_TEST(ConnectionInfoTest, TestSQLGetInfoDatabaseName) {
- SQLWCHAR value[kOdbcBufferSize] = L"";
- GetInfo(conn, SQL_DATABASE_NAME, value);
+ SQLWCHAR value[kOdbcBufferSize] = {};
+ GetInfoSQLWCHAR(conn, SQL_DATABASE_NAME, value);
- EXPECT_STREQ(static_cast<const SQLWCHAR*>(L""), value);
+ std::wstring result = ConvertToWString(value);
+ EXPECT_EQ(std::wstring(L""), result);
}
TYPED_TEST(ConnectionInfoTest, TestSQLGetInfoDbmsName) {
- SQLWCHAR value[kOdbcBufferSize] = L"";
- GetInfo(conn, SQL_DBMS_NAME, value);
+ SQLWCHAR value[kOdbcBufferSize] = {};
+ GetInfoSQLWCHAR(conn, SQL_DBMS_NAME, value);
- EXPECT_GT(wcslen(value), 0);
+ EXPECT_GT(wcslen(reinterpret_cast<wchar_t*>(value)), 0);
}
TYPED_TEST(ConnectionInfoTest, TestSQLGetInfoDbmsVer) {
- SQLWCHAR value[kOdbcBufferSize] = L"";
- GetInfo(conn, SQL_DBMS_VER, value);
+ SQLWCHAR value[kOdbcBufferSize] = {};
+ GetInfoSQLWCHAR(conn, SQL_DBMS_VER, value);
- EXPECT_GT(wcslen(value), 0);
+ EXPECT_GT(wcslen(reinterpret_cast<wchar_t*>(value)), 0);
}
Review Comment:
Using wcslen(reinterpret_cast<wchar_t*>(value)) is undefined on platforms
where SQLWCHAR is not wchar_t (e.g., Linux with UTF-16 SQLWCHAR). This can read
past the buffer or compute an incorrect length. Prefer SqlWCharArrLen(value) or
ConvertToWString(value).length() for the non-empty assertion.
##########
cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.cc:
##########
@@ -491,7 +491,7 @@ bool WriteDSN(Connection::ConnPropertyMap properties) {
std::string driver = config.Get(FlightSqlConnection::DRIVER);
std::wstring w_driver = arrow::util::UTF8ToWideString(driver).ValueOr(L"");
- return RegisterDsn(config, w_driver.c_str());
+ return RegisterDsn(config, reinterpret_cast<LPCWSTR>(w_driver.c_str()));
}
Review Comment:
WriteDSN converts the driver name to std::wstring and then reinterpret_casts
w_driver.c_str() to LPCWSTR. On Linux, the driver manager expects UTF-16
SQLWCHAR (see system_dsn.cc ConvertToSQLWCHAR), so casting wchar_t* (UTF-32) to
LPCWSTR is likely incorrect and can mis-encode the driver name or cause
misaligned reads. Convert the UTF-8 driver string to the platform’s expected
SQLWCHAR representation before calling RegisterDsn (e.g., UTF8StringToUTF16 on
Linux, UTF8ToWideString elsewhere), and pass the resulting buffer pointer
without reinterpret_casting between incompatible character types.
##########
cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.h:
##########
@@ -38,6 +38,20 @@
// For DSN registration
#include "arrow/flight/sql/odbc/odbc_impl/system_dsn.h"
+#ifdef __linux__
+# define ASSIGN_SQLWCHAR_ARR(name, wstring_literal) \
+ auto name##_vec = ODBC::ToSqlWCharVector(std::wstring(wstring_literal)); \
+ SQLWCHAR* name = name##_vec.data();
+# define ASSIGN_SQLWCHAR_ARR_AND_LEN(name, wstring_literal) \
+ ASSIGN_SQLWCHAR_ARR(name, wstring_literal) \
+ size_t name##_len = std::wstring(wstring_literal).length();
+#else // Windows & Mac
+# define ASSIGN_SQLWCHAR_ARR(name, wstring_literal) SQLWCHAR name[] =
wstring_literal;
+# define ASSIGN_SQLWCHAR_ARR_AND_LEN(name, wstring_literal) \
+ ASSIGN_SQLWCHAR_ARR(name, wstring_literal) \
+ size_t name##_len = std::wcslen(name);
Review Comment:
On Linux, ASSIGN_SQLWCHAR_ARR builds a SQLWCHAR vector via
ODBC::ToSqlWCharVector but does not append a NUL terminator. Many call sites
pass SQL_NTS (and some helpers like ConvertToWString default to NTS), which can
lead to out-of-bounds reads in the driver manager/tests. Consider ensuring the
produced buffer is always NUL-terminated (e.g., append a 0 code unit) and base
any computed length on the converted buffer size (excluding the terminator)
rather than std::wstring(wstring_literal).length(). Also, defining the *_len
variable as an ODBC length type (SQLINTEGER/SQLSMALLINT as appropriate) will
avoid potential -Wshorten-64-to-32 warnings with Clang.
```suggestion
# define ASSIGN_SQLWCHAR_ARR(name, wstring_literal)
\
auto name##_vec = ODBC::ToSqlWCharVector(std::wstring(wstring_literal));
\
if (name##_vec.empty() || name##_vec.back() != static_cast<SQLWCHAR>(0))
{ \
name##_vec.push_back(static_cast<SQLWCHAR>(0));
\
}
\
SQLWCHAR* name = name##_vec.data();
# define ASSIGN_SQLWCHAR_ARR_AND_LEN(name, wstring_literal)
\
ASSIGN_SQLWCHAR_ARR(name, wstring_literal)
\
SQLINTEGER name##_len = static_cast<SQLINTEGER>(name##_vec.size() - 1);
#else // Windows & Mac
# define ASSIGN_SQLWCHAR_ARR(name, wstring_literal) SQLWCHAR name[] =
wstring_literal;
# define ASSIGN_SQLWCHAR_ARR_AND_LEN(name, wstring_literal) \
ASSIGN_SQLWCHAR_ARR(name, wstring_literal) \
SQLINTEGER name##_len = static_cast<SQLINTEGER>(std::wcslen(name));
```
##########
cpp/src/arrow/flight/sql/odbc/tests/statement_test.cc:
##########
@@ -1974,80 +1950,75 @@ TYPED_TEST(StatementTest,
TestSQLMoreResultsInvalidFunctionSequence) {
TYPED_TEST(StatementTest, TestSQLNativeSqlReturnsInputString) {
SQLWCHAR buf[1024];
SQLINTEGER buf_char_len = sizeof(buf) / GetSqlWCharSize();
- SQLWCHAR input_str[] = L"SELECT * FROM mytable WHERE id == 1";
- SQLINTEGER input_char_len = static_cast<SQLINTEGER>(wcslen(input_str));
+ ASSIGN_SQLWCHAR_ARR_AND_LEN(input_str, L"SELECT * FROM mytable WHERE id ==
1");
SQLINTEGER output_char_len = 0;
- std::wstring expected_string = std::wstring(input_str);
- ASSERT_EQ(SQL_SUCCESS, SQLNativeSql(conn, input_str, input_char_len, buf,
buf_char_len,
+ ASSERT_EQ(SQL_SUCCESS, SQLNativeSql(conn, input_str, input_str_len, buf,
buf_char_len,
&output_char_len));
- EXPECT_EQ(input_char_len, output_char_len);
+ EXPECT_EQ(input_str_len, output_char_len);
// returned length is in characters
std::wstring returned_string(buf, buf + output_char_len);
- EXPECT_EQ(expected_string, returned_string);
+ std::wstring input = ConvertToWString(input_str, input_str_len);
+ EXPECT_EQ(input, returned_string);
}
TYPED_TEST(StatementTest, TestSQLNativeSqlReturnsNTSInputString) {
SQLWCHAR buf[1024];
SQLINTEGER buf_char_len = sizeof(buf) / GetSqlWCharSize();
- SQLWCHAR input_str[] = L"SELECT * FROM mytable WHERE id == 1";
- SQLINTEGER input_char_len = static_cast<SQLINTEGER>(wcslen(input_str));
+ ASSIGN_SQLWCHAR_ARR_AND_LEN(input_str, L"SELECT * FROM mytable WHERE id ==
1");
SQLINTEGER output_char_len = 0;
- std::wstring expected_string = std::wstring(input_str);
ASSERT_EQ(SQL_SUCCESS,
SQLNativeSql(conn, input_str, SQL_NTS, buf, buf_char_len,
&output_char_len));
- EXPECT_EQ(input_char_len, output_char_len);
+ EXPECT_EQ(input_str_len, output_char_len);
// returned length is in characters
std::wstring returned_string(buf, buf + output_char_len);
+ std::wstring expected_string = ConvertToWString(input_str, input_str_len);
EXPECT_EQ(expected_string, returned_string);
}
TYPED_TEST(StatementTest, TestSQLNativeSqlReturnsInputStringLength) {
- SQLWCHAR input_str[] = L"SELECT * FROM mytable WHERE id == 1";
- SQLINTEGER input_char_len = static_cast<SQLINTEGER>(wcslen(input_str));
+ ASSIGN_SQLWCHAR_ARR_AND_LEN(input_str, L"SELECT * FROM mytable WHERE id ==
1");
SQLINTEGER output_char_len = 0;
- std::wstring expected_string = std::wstring(input_str);
ASSERT_EQ(SQL_SUCCESS,
- SQLNativeSql(conn, input_str, input_char_len, nullptr, 0,
&output_char_len));
+ SQLNativeSql(conn, input_str, input_str_len, nullptr, 0,
&output_char_len));
- EXPECT_EQ(input_char_len, output_char_len);
+ EXPECT_EQ(input_str_len, output_char_len);
ASSERT_EQ(SQL_SUCCESS,
SQLNativeSql(conn, input_str, SQL_NTS, nullptr, 0,
&output_char_len));
- EXPECT_EQ(input_char_len, output_char_len);
+ EXPECT_EQ(input_str_len, output_char_len);
}
TYPED_TEST(StatementTest, TestSQLNativeSqlReturnsTruncatedString) {
const SQLINTEGER small_buf_size_in_char = 11;
SQLWCHAR small_buf[small_buf_size_in_char];
SQLINTEGER small_buf_char_len = sizeof(small_buf) / GetSqlWCharSize();
- SQLWCHAR input_str[] = L"SELECT * FROM mytable WHERE id == 1";
- SQLINTEGER input_char_len = static_cast<SQLINTEGER>(wcslen(input_str));
+ ASSIGN_SQLWCHAR_ARR_AND_LEN(input_str, L"SELECT * FROM mytable WHERE id ==
1");
SQLINTEGER output_char_len = 0;
// Create expected return string based on buf size
SQLWCHAR expected_string_buf[small_buf_size_in_char];
- wcsncpy(expected_string_buf, input_str, 10);
+ wcsncpy(reinterpret_cast<wchar_t*>(expected_string_buf),
+ reinterpret_cast<const wchar_t*>(input_str), 10);
expected_string_buf[10] = L'\0';
Review Comment:
wcsncpy(reinterpret_cast<wchar_t*>(...)) is undefined when SQLWCHAR is not
wchar_t (notably on Linux where SQLWCHAR can be UTF-16). This can corrupt the
expected buffer and/or read misaligned memory. Instead, copy the first 10
SQLWCHAR code units using a type-safe approach (e.g., std::copy_n on SQLWCHAR)
and then explicitly write a SQLWCHAR NUL terminator.
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc:
##########
@@ -345,24 +351,26 @@ TYPED_TEST(ConnectionInfoTest,
TestSQLGetInfoParamArraySelects) {
}
TYPED_TEST(ConnectionInfoTest, TestSQLGetInfoRowUpdates) {
- SQLWCHAR value[kOdbcBufferSize] = L"";
- GetInfo(conn, SQL_ROW_UPDATES, value);
+ SQLWCHAR value[kOdbcBufferSize] = {};
+ GetInfoSQLWCHAR(conn, SQL_ROW_UPDATES, value);
- EXPECT_STREQ(static_cast<const SQLWCHAR*>(L"N"), value);
+ std::wstring result = ConvertToWString(value);
+ EXPECT_EQ(std::wstring(L"N"), result);
}
TYPED_TEST(ConnectionInfoTest, TestSQLGetInfoSearchPatternEscape) {
- SQLWCHAR value[kOdbcBufferSize] = L"";
- GetInfo(conn, SQL_SEARCH_PATTERN_ESCAPE, value);
+ SQLWCHAR value[kOdbcBufferSize] = {};
+ GetInfoSQLWCHAR(conn, SQL_SEARCH_PATTERN_ESCAPE, value);
- EXPECT_STREQ(static_cast<const SQLWCHAR*>(L"\\"), value);
+ std::wstring result = ConvertToWString(value);
+ EXPECT_EQ(std::wstring(L"\\"), result);
}
TYPED_TEST(ConnectionInfoTest, TestSQLGetInfoServerName) {
- SQLWCHAR value[kOdbcBufferSize] = L"";
- GetInfo(conn, SQL_SERVER_NAME, value);
+ SQLWCHAR value[kOdbcBufferSize] = {};
+ GetInfoSQLWCHAR(conn, SQL_SERVER_NAME, value);
- EXPECT_GT(wcslen(value), 0);
+ EXPECT_GT(wcslen(reinterpret_cast<wchar_t*>(value)), 0);
Review Comment:
Using wcslen(reinterpret_cast<wchar_t*>(value)) is undefined on platforms
where SQLWCHAR is not wchar_t (e.g., Linux with UTF-16 SQLWCHAR). This can read
past the buffer or compute an incorrect length. Prefer SqlWCharArrLen(value) or
ConvertToWString(value).length() for the non-empty assertion.
```suggestion
std::wstring result = ConvertToWString(value);
EXPECT_GT(result.length(), 0);
```
--
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]
