Skip to content

Conversation

@slabko
Copy link
Contributor

@slabko slabko commented Nov 24, 2025

Fixes Text Corruption issue in Long UTF-8 Strings

@slabko slabko force-pushed the sql-get-data-in-parts branch 4 times, most recently from 4a559ec to 198a6d8 Compare November 26, 2025 16:13
@slabko slabko force-pushed the sql-get-data-in-parts branch from 198a6d8 to fc86d0f Compare November 26, 2025 16:58
@slabko slabko marked this pull request as ready for review November 27, 2025 17:06
@slabko slabko merged commit d413a15 into master Nov 27, 2025
21 checks passed
@slabko slabko deleted the sql-get-data-in-parts branch November 27, 2025 17:07
Copy link

@windsurf-bot windsurf-bot bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other comments (8)
  • driver/statement.h (82-85) The `getColumnOffset` method doesn't check if the column exists in the map before accessing it. This could lead to undefined behavior if the column key doesn't exist.
    size_t getColumnOffset(SQLUSMALLINT column)
    {
        auto it = column_offsets.find(column);
        return (it != column_offsets.end()) ? it->second : 0;
    }
    
  • driver/test/column_bindings_it.cpp (81-81) The `byte_offset` variable is declared but never used in this test.
  • driver/test/type_info_it.cpp (452-452) The line checking `COLUMN_NAME` against the `column` variable has been removed. This check is important to ensure we're processing the correct column data. Without it, the test could pass even if column names don't match what we expect.
  • driver/utils/type_info.h (385-385) The offset calculation in `offset_in_symbols = offset / sizeof(CharType)` assumes that all characters have the same byte size. For UTF-8 strings, this might lead to incorrect positioning when characters have variable byte lengths. Consider using a proper UTF-8 aware function to calculate the character position from byte offset.
  • driver/utils/type_info.h (388-389) The pointer arithmetic `converted.data() + offset / sizeof(*converted.data())` could potentially access memory outside the bounds of the `converted` buffer if offset is not properly validated. Consider adding additional bounds checking to ensure the calculated pointer is within the allocated memory.
  • driver/api/impl/impl.cpp (1503-1503) The `null_size` calculation assumes the target type is either SQL_C_CHAR or SQL_C_WCHAR, but this variable is used unconditionally. Consider moving this calculation inside the character type check or ensuring it's only used for character types.
  • driver/test/column_bindings_it.cpp (68-68) The `byte_offset` variable is being updated with `real_char_len` which is a character count, but the variable name and its usage in the substring suggest it should be a byte offset. This could cause issues with multi-byte character sets.
            byte_offset += real_char_len;
    
  • driver/test/column_bindings_it.cpp (27-27) There's a duplicate phrase in this comment line.
     * 5. Each part is null-terminated; the application must remove the null-termination character if concatenating the
    

💡 To request another review, post a new comment with "/windsurf-review".

Comment on lines +382 to +384
if (offset > converted_length_in_bytes) {
offset = converted_length_in_bytes;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a check to ensure offset is not negative before using it in calculations. A negative offset could lead to buffer overruns or other undefined behavior.

Comment on lines +1507 to +1513
} catch (const SqlException & ex) {
bool is_char_type = TargetType == SQL_C_CHAR || TargetType == SQL_C_WCHAR;
if (ex.getReturnCode() != SQL_SUCCESS_WITH_INFO || *binding_info.indicator < 0 || !is_char_type) {
throw;
}
statement.setColumnOffset(column_idx, offset + BufferLength - null_size);
throw;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exception is being rethrown unconditionally after checking conditions. This will cause the error handling logic to fail. Consider returning from the function instead of throwing after setting the column offset.

if (ret == SQL_SUCCESS) {
SQLLEN out_len = *binding_info.indicator;
// Make sure to add one character to indicate partial load even when string is empty.
statement.setColumnOffset(column_idx, offset + static_cast<size_t>(out_len) + null_size);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When calculating the new column offset, the code doesn't check if out_len is negative (which could happen with SQL_NULL_DATA). This could lead to incorrect offset calculations. Consider adding a check for negative values before the cast.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants