Put the string lengths in their proper place

[mcg1969]

Wow, I wonder how this one slipped through the cracks for so long!

The cbdata_ vector is of length rowset_size_. For strings, it contains either the string length, or SQL_NULL_DATA. Obviously, the length of string N should be in cbdata_[N]. But for some reason, the code was expecting to see every string length in the zeroth position. This was causing me problems when reading data that had variable string lengths.

[mcg1969]

Answering my own question: the lengths haven't been there for too long. This looks like the commit.

[mloskot]

Likely, it slipped because most/all users have relied on the default rowset_size_{1}, so the rowset_position_ was always Zero.

Unfortunately, even our tests do not cover such use case that would hit the bug.
It is a good idea to cover a bug fix with a test (e.g. null_test in base_test_fixture.h).

Good catch!

[mloskot]

@mcg1969 See my PR #166 which seems related to your problem too.

To summary, there is two-fold problem:

• Your fix in this PR corrects aspect of accessing column descriptor if rowset_size_ > 1
• There is another issue with null indicator not being set for variable-length data.

Would you agree?

[mcg1969]

Actually, 3 of the four fixes in this PR deal with variable-length data. I might not have caught every case though.

[lexicalunit]

@mcg1969 would you like to take a crack at writing a tests to catch this based on the code you were writing that exposed this issue to you?

[mcg1969]

I'll see what I can do!

[mloskot]

@mcg1969 I'd like to help and write some tests for this PR. Simply, did you reveal the problem by using execute argument batch_operations larger than 1?

If we agree, let's merge it and I will work on the tests straight away, so this is covered.

[mcg1969]

Thanks for the offer, I'm afraid I've been swamped.

The way to exercise the bug is to have at least two rows. The string in Row 2 should have a longer length than the string in Row 1. When that happens, Row 2 will get truncated, because it will use the length from the first row.

[mcg1969]

So yes, you need a batch size of at least 2...

[mloskot]

@mcg1969 In #185 I have managed to create tests which reproduce the bug as you described:

• Test case block_cursor_with_nvarchar_test, for test records:

insert into variable_string_test (i, s) values (1, 'this is a shorter text');
insert into variable_string_test (i, s) values (2, 'this is a longer text of the two texts in the table');

testing value of second row fails:

D:\dev\nanodbc\test\mssql_test.cpp(137): FAILED:
  REQUIRE( results.get<nanodbc::string_type>(1) == L"this is a longer text of the two texts in the table" )
with expansion:
  "this is a longer text "
  ==
  "this is a longer text of the two texts in the table"

• Test case block_cursor_with_nvarchar_and_first_row_null_test, for test rows:

insert into variable_string_test (i, s) values (1, NULL);
insert into variable_string_test (i, s) values (2, 'this is a longer text of the two texts in the table');

memory corruption / buffer overrun happens in result::result_impl::get_ref_impl<string_type>
while accessing the second row:

// Visual C++ checked iterators throw invalid iterator range
// due to str_size=2147483647
wide_string_type temp(s, s + str_size);

I think the test covers the issue pretty well and I'm going to merge your PR.
@lexicalunit do you agree?

[mcg1969]

Love it. Thank you so much for doing this.

[lexicalunit]

Looks great y'all!