Copilot commented on code in PR #49668:
URL: https://github.com/apache/arrow/pull/49668#discussion_r3113569046
##########
cpp/src/arrow/flight/sql/odbc/tests/odbc_test_suite.h:
##########
@@ -38,6 +38,23 @@
// 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));
\
+ 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) \
+ SQLSMALLINT name##_len = static_cast<SQLSMALLINT>(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) \
+ SQLSMALLINT name##_len = static_cast<SQLSMALLINT>(std::wcslen(name));
Review Comment:
`ASSIGN_SQLWCHAR_ARR_AND_LEN` defines `name##_len` as `SQLSMALLINT`, but
many call sites pass this value to APIs whose length parameter is `SQLINTEGER`
(e.g., `SQLExecDirect`, `SQLPrepare`, `SQLNativeSql`). This can silently
truncate lengths and is type-inconsistent. Consider defining `name##_len` as
`SQLINTEGER` (or offering a separate macro for `SQLINTEGER` lengths) and
casting the computed length accordingly on all platforms.
```suggestion
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/odbc_test_suite.cc:
##########
@@ -510,13 +510,45 @@ std::wstring GetStringColumnW(SQLHSTMT stmt, int col_id) {
return std::wstring(buf, buf + char_count);
}
-std::wstring ConvertToWString(const std::vector<SQLWCHAR>& str_val,
SQLSMALLINT str_len) {
+size_t SqlWCharArrLen(const SQLWCHAR* str_val) {
+ if (!str_val) {
+ return 0;
+ }
+ const SQLWCHAR* p = str_val;
+ while (*p != 0) {
+ ++p;
+ }
+ return static_cast<size_t>(p - str_val);
+}
+
+std::wstring ConvertToWString(const SQLWCHAR* str_val, SQLSMALLINT str_len,
+ SQLSMALLINT buffer_size) {
+ if (str_len == -1) {
+#ifdef __linux__
+ str_len = SqlWCharArrLen(str_val);
+#else // Windows & Mac
+ str_len = std::wcslen(str_val);
+#endif
+ }
+ std::wstring attr_str;
+ if (str_len == 0) {
+ attr_str = L"";
+ } else {
+ assert(str_val != nullptr);
+ assert(str_len > 0 && str_len <= buffer_size);
+ attr_str.assign(str_val, str_val + str_len);
+ }
+ return attr_str;
+}
+
+std::wstring ConvertToWString(const std::vector<SQLWCHAR>& str_val,
SQLSMALLINT str_len,
+ SQLSMALLINT buffer_size) {
std::wstring attr_str;
if (str_len == 0) {
- attr_str = std::wstring(&str_val[0]);
+ attr_str = L"";
} else {
EXPECT_GT(str_len, 0);
- EXPECT_LE(str_len, static_cast<SQLSMALLINT>(kOdbcBufferSize));
+ EXPECT_LE(str_len, buffer_size);
Review Comment:
In `ConvertToWString(const std::vector<SQLWCHAR>&, SQLSMALLINT, ...)`,
`str_len` is documented/used as a byte length (`str_len / GetSqlWCharSize()`),
but the bounds check compares it to `buffer_size` (a character count). This
mixes units and can cause false test failures (or mask real overruns). Make the
check unit-consistent (e.g., compare `str_len` to `buffer_size *
GetSqlWCharSize()` if `str_len` is bytes, or treat `str_len` as characters and
remove the division).
```suggestion
EXPECT_LE(str_len, buffer_size * GetSqlWCharSize());
```
--
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]
