GH-50762: [C++][Feather] Avoid reading outside sliced bitmap buffers by tcmartin · Pull Request #50763 · apache/arrow

tcmartin · 2026-07-31T18:53:31Z

Rationale for this change

The Feather V1 bitmap shift loop unconditionally read the byte after the final
source byte. For an exact-sized sliced bitmap, this caused a one-byte
out-of-bounds read and copied data outside the logical slice into unused bits of
the serialized bitmap.

Closes #50762.

What changes are included in this PR?

Keep bitmap length in bits while writing shifted Boolean and validity
bitmaps.
Stop before reading a source byte that is not required by the logical slice.
Clear unused trailing bits in the serialized bitmap.
Add an exact-sized sliced-Boolean regression test that checks the serialized
byte and round-trip result.

Are these changes tested?

Yes.

Full arrow-feather-test suite under ASan/UBSan: 84 passed, 5 expected
skips.
Standalone exact-sized-buffer reproducer under ASan/UBSan: offset-zero
control and offset-one trigger both complete cleanly after the fix.

Are there any user-facing changes?

No public API changes. Feather V1 output no longer incorporates bits outside a
logical bitmap slice.

This PR contains a Critical Fix. It prevents an out-of-bounds read and
incorrect data from being copied into serialized output when the documented
array-buffer contract is upheld.

AI assistance

I used an AI coding assistant to help trace the writer, draft the implementation
and regression test, and run the verification commands. I reviewed the complete
diff, reproduced the original fault under ASan, verified the bit-boundary
logic, and ran the full Feather test suite. No maintainer was tagged or pinged.

GitHub Issue: [C++][Feather] Avoid reading outside sliced bitmap buffers in V1 writer #50762

Track logical bit lengths separately from byte lengths when shifting sliced bitmaps, stop before the first byte outside the declared slice, and clear unused output bits. Add an exact-sized-buffer regression test that checks both serialized output and round-trip behavior. Constraint: Preserve the streaming writer and Feather V1 byte layout. Rejected: Copy the entire bitmap into a temporary allocation | adds allocation and abandons the existing bounded streaming design. Confidence: high Scope-risk: narrow Directive: Keep bitmap length in bits; byte length alone cannot determine whether the final shifted source byte is valid. Tested: ASan/UBSan standalone control and trigger; full arrow-feather-test suite (84 passed, 5 expected skips). Not-tested: Full Arrow repository test suite.

github-actions · 2026-07-31T18:54:03Z

⚠️ GitHub issue #50762 has no components, please add labels for components.

taepper · 2026-07-31T20:47:35Z

+    for (int64_t i = 0; i < length; ++i) {
      uint8_t r = static_cast<uint8_t>(*data++ >> bit_shift);
-      uint8_t l = static_cast<uint8_t>(*data << lshift);
+      uint8_t l =
+          data == data_end ? 0 : static_cast<uint8_t>(*data << lshift);
      uint8_t value = l | r;
+      if (i == length - 1 && bit_length % 8 != 0) {
+        value &= bit_util::LeastSignificantBitMask<uint8_t>(bit_length % 8);
+      }
      *buffer_it++ = value;
      if (buffer_it == buffer_end) {
        RETURN_NOT_OK(stream->Write(buffer, buffersize));
        buffer_it = buffer;
      }
    }
    if (buffer_it != buffer) {
      RETURN_NOT_OK(stream->Write(buffer, buffer_it - buffer));
    }


Would it be easier to let the loop run only until i + 1 < length.
And then change L109-111:

uint8_t r = static_cast<uint8_t>(*data++ >> bit_shift); *buffer_it++ = r; RETURN_NOT_OK(stream->Write(buffer, buffer_it - buffer));

I.e. always handle the last byte after the loop.

Ah, it might still need masking of the upper bits of r.

pitrou

I'm just seeing this PR, sorry @tcmartin

pitrou · 2026-09-23T08:59:34Z

-      return WriteBuffer(buffer,
-                         bit_util::BytesForBits(values.length() * fw_type.bit_width()),
-                         bit_offset);
+      return WriteBuffer(


Let's add a DCHECK_EQ(fw_type.bit_width() % 8, 0) to make sure we're not handling any non-byte-aligned type here?

pitrou · 2026-09-23T09:00:01Z

+        return WriteBitmap(prim_values.values()->data(), values.length(),
+                           prim_values.offset());
+      }
      const uint8_t* buffer =


Since the above is if constexpr, we might want to use an else branch here (making it constexpr too).

pitrou · 2026-09-23T09:03:23Z

+      if (i == length - 1 && bit_length % 8 != 0) {
+        value &= bit_util::LeastSignificantBitMask<uint8_t>(bit_length % 8);
+      }


Is this actually required? I don't think it matters that we are writing potentially non-zero bits past the logical end of the bitmap.

pitrou · 2026-09-23T09:07:19Z

+  ASSERT_GT(output_->size(), 8);
+  ASSERT_EQ(output_->data()[8], 0x01);


Is this testing the raw Feather bytes? Can you add a comment explaining these checks?

And by the way, if output_'s size is 8, then you cannot read beyond output_->data()[7].

pitrou · 2026-09-23T09:07:35Z

+  auto data =
+      ArrayData::Make(boolean(), 1, {nullptr, values}, /*null_count=*/0, /*offset=*/1);


Can we have a loop running offset from 0 to 7?

pitrou · 2026-09-23T09:07:54Z

 }

+TEST_P(TestFeather, ExactSizeSlicedBooleanBuffer) {
+  if (GetParam().version != kFeatherV1Version) {


Add a comment at top pointing to the GH issue?

tcmartin requested a review from pitrou as a code owner July 31, 2026 18:53

github-actions Bot added Component: C++ awaiting review Awaiting review labels Jul 31, 2026

taepper reviewed Jul 31, 2026

View reviewed changes

pitrou reviewed Sep 23, 2026

View reviewed changes

github-actions Bot added awaiting review Awaiting review awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Sep 23, 2026

Navigation Menu

Repository navigation

GH-50762: [C++][Feather] Avoid reading outside sliced bitmap buffers - #50763

GH-50762: [C++][Feather] Avoid reading outside sliced bitmap buffers#50763
tcmartin wants to merge 1 commit into
apache:mainfrom
tcmartin:gh-50762-feather-bitmap-bounds

tcmartin commented Jul 31, 2026 •

edited by github-actions Bot

Loading

Uh oh!

github-actions Bot commented Jul 31, 2026

Uh oh!

taepper Jul 31, 2026

Uh oh!

taepper Jul 31, 2026

Uh oh!

pitrou left a comment

Uh oh!

pitrou Sep 23, 2026

Uh oh!

pitrou Sep 23, 2026

Uh oh!

pitrou Sep 23, 2026

Uh oh!

pitrou Sep 23, 2026

Uh oh!

pitrou Sep 23, 2026

Uh oh!

pitrou Sep 23, 2026

Uh oh!

pitrou Sep 23, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		ASSERT_GT(output_->size(), 8);
		ASSERT_EQ(output_->data()[8], 0x01);

		auto data =
		ArrayData::Make(boolean(), 1, {nullptr, values}, /null_count=/0, /offset=/1);

Conversation

tcmartin commented Jul 31, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

AI assistance

Uh oh!

github-actions Bot commented Jul 31, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tcmartin commented Jul 31, 2026 •

edited by github-actions Bot

Loading