Skip to content

AVRO-4228: [c++] Fix BinaryDecoder::arrayNext() to handle negative block counts#3646

Open
gfeyer wants to merge 2 commits intoapache:mainfrom
gfeyer:fix/cpp-arrayNext-negative-block-count
Open

AVRO-4228: [c++] Fix BinaryDecoder::arrayNext() to handle negative block counts#3646
gfeyer wants to merge 2 commits intoapache:mainfrom
gfeyer:fix/cpp-arrayNext-negative-block-count

Conversation

@gfeyer
Copy link

@gfeyer gfeyer commented Feb 8, 2026

What is the purpose of the change

BinaryDecoder::arrayNext() calls doDecodeLong() directly instead of doDecodeItemCount(), causing it to mishandle negative array block counts. Per the Avro spec, a negative block count means the absolute value is the item count followed by an additional long for the byte-size of the block. When arrayNext() reads a negative count, static_cast<size_t>(-100) produces a huge value and the byte-size long is left unconsumed, corrupting the stream position.

doDecodeItemCount() already handles this correctly and is used by arrayStart(), mapStart(), and mapNext(). Only arrayNext() bypassed it. The fix changes arrayNext() to call doDecodeItemCount() for consistency.

This affects any array large enough to be encoded in multiple blocks with negative counts. ClickHouse independently found the same bug (ClickHouse/ClickHouse#60438, ClickHouse#23).

Verifying this change

  • Added testArrayNegativeBlockCount in CodecTests.cc that decodes a hand-crafted array with negative block counts and verifies all items are read correctly
  • The fix was also verified against production Avro messages containing arrays with 266+ items encoded in multiple blocks with negative counts, which previously failed ~20% of the time in our systems and now decode correctly.

Documentation

  • Does this pull request introduce a new feature? no

@github-actions github-actions bot added the C++ Pull Requests for C++ binding label Feb 8, 2026
}
}

static void testArrayNegativeBlockCount() {
Copy link
Member

Choose a reason for hiding this comment

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

This test pass even without the fix above.
The reason is that the negative count is in the first block and this uses arrayStart() which already delegates to doDecodeItemCount().

Please update the test to use a negative count [also] in the second block.

auto result = doDecodeLong();
if (result < 0) {
doDecodeLong();
return static_cast<size_t>(-result);
Copy link
Member

Choose a reason for hiding this comment

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

This may lead to overflow if result is size_t's min value.
Possible improvement:

Suggested change
return static_cast<size_t>(-(result + 1)) + 1;

Comment on lines +2132 to +2136
BOOST_CHECK_EQUAL(result[0], 10);
BOOST_CHECK_EQUAL(result[1], 20);
BOOST_CHECK_EQUAL(result[2], 30);
BOOST_CHECK_EQUAL(result[3], 40);
BOOST_CHECK_EQUAL(result[4], 50);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
BOOST_CHECK_EQUAL(result[0], 10);
BOOST_CHECK_EQUAL(result[1], 20);
BOOST_CHECK_EQUAL(result[2], 30);
BOOST_CHECK_EQUAL(result[3], 40);
BOOST_CHECK_EQUAL(result[4], 50);
const std::vector<int32_t> expected = {10, 20, 30, 40, 50};
BOOST_CHECK_EQUAL_COLLECTIONS(result.begin(), result.end(), expected.begin(), expected.end());

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

Labels

C++ Pull Requests for C++ binding

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants