You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "tcmartin (via GitHub)" <gi...@apache.org> on 2026/07/31 18:53:34 UTC
[PR] GH-50762: [C++][Feather] Avoid reading outside sliced bitmap buffers [arrow]
tcmartin opened a new pull request, #50763:
URL: https://github.com/apache/arrow/pull/50763
### 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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50762: [C++][Feather] Avoid reading outside sliced bitmap buffers [arrow]
Posted by "taepper (via GitHub)" <gi...@apache.org>.
taepper commented on code in PR #50763:
URL: https://github.com/apache/arrow/pull/50763#discussion_r3693279909
##########
cpp/src/arrow/ipc/feather.cc:
##########
Review Comment:
Ah, it might still need masking of the upper bits of `r`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50762: [C++][Feather] Avoid reading outside sliced bitmap buffers [arrow]
Posted by "taepper (via GitHub)" <gi...@apache.org>.
taepper commented on code in PR #50763:
URL: https://github.com/apache/arrow/pull/50763#discussion_r3693274694
##########
cpp/src/arrow/ipc/feather.cc:
##########
Review Comment:
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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50762: [C++][Feather] Avoid reading outside sliced bitmap buffers [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #50763:
URL: https://github.com/apache/arrow/pull/50763#issuecomment-5146425319
:warning: GitHub issue #50762 **has no components**, please add labels for components.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-50762: [C++][Feather] Avoid reading outside sliced bitmap buffers [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #50763:
URL: https://github.com/apache/arrow/pull/50763#discussion_r4080711242
##########
cpp/src/arrow/ipc/feather.cc:
##########
@@ -561,12 +586,14 @@ struct ArrayWriterV1 {
const auto& fw_type = checked_cast<const FixedWidthType&>(*values.type());
if (prim_values.values()) {
+ if constexpr (is_boolean_type<T>::value) {
+ return WriteBitmap(prim_values.values()->data(), values.length(),
+ prim_values.offset());
+ }
const uint8_t* buffer =
Review Comment:
Since the above is `if constexpr`, we might want to use an `else` branch here (making it `constexpr` too).
##########
cpp/src/arrow/ipc/feather_test.cc:
##########
@@ -323,6 +324,30 @@ TEST_P(TestFeather, SliceBooleanRoundTrip) {
CheckSlices(batch);
}
+TEST_P(TestFeather, ExactSizeSlicedBooleanBuffer) {
+ if (GetParam().version != kFeatherV1Version) {
+ GTEST_SKIP() << "This test targets the Feather V1 bitmap writer";
+ }
+
+ // The second byte is outside the declared buffer. It must not be read into
+ // the unused bits of the serialized one-bit slice.
+ std::array<uint8_t, 2> storage = {0x02, 0x01};
+ auto values = std::make_shared<Buffer>(storage.data(), 1);
+ auto data =
+ ArrayData::Make(boolean(), 1, {nullptr, values}, /*null_count=*/0, /*offset=*/1);
+ auto array = MakeArray(data);
+ auto table = Table::Make(schema({field("flag", boolean())}),
+ {std::make_shared<ChunkedArray>(array)});
+
+ DoWrite(*table);
+ ASSERT_GT(output_->size(), 8);
+ ASSERT_EQ(output_->data()[8], 0x01);
Review Comment:
And by the way, if `output_`'s size is 8, then you cannot read beyond `output_->data()[7]`.
##########
cpp/src/arrow/ipc/feather_test.cc:
##########
@@ -323,6 +324,30 @@ TEST_P(TestFeather, SliceBooleanRoundTrip) {
CheckSlices(batch);
}
+TEST_P(TestFeather, ExactSizeSlicedBooleanBuffer) {
+ if (GetParam().version != kFeatherV1Version) {
+ GTEST_SKIP() << "This test targets the Feather V1 bitmap writer";
+ }
+
+ // The second byte is outside the declared buffer. It must not be read into
+ // the unused bits of the serialized one-bit slice.
+ std::array<uint8_t, 2> storage = {0x02, 0x01};
+ auto values = std::make_shared<Buffer>(storage.data(), 1);
+ auto data =
+ ArrayData::Make(boolean(), 1, {nullptr, values}, /*null_count=*/0, /*offset=*/1);
+ auto array = MakeArray(data);
+ auto table = Table::Make(schema({field("flag", boolean())}),
+ {std::make_shared<ChunkedArray>(array)});
+
+ DoWrite(*table);
+ ASSERT_GT(output_->size(), 8);
+ ASSERT_EQ(output_->data()[8], 0x01);
Review Comment:
Is this testing the raw Feather bytes? Can you add a comment explaining these checks?
##########
cpp/src/arrow/ipc/feather_test.cc:
##########
@@ -323,6 +324,30 @@ TEST_P(TestFeather, SliceBooleanRoundTrip) {
CheckSlices(batch);
}
+TEST_P(TestFeather, ExactSizeSlicedBooleanBuffer) {
+ if (GetParam().version != kFeatherV1Version) {
+ GTEST_SKIP() << "This test targets the Feather V1 bitmap writer";
+ }
+
+ // The second byte is outside the declared buffer. It must not be read into
+ // the unused bits of the serialized one-bit slice.
+ std::array<uint8_t, 2> storage = {0x02, 0x01};
+ auto values = std::make_shared<Buffer>(storage.data(), 1);
+ auto data =
+ ArrayData::Make(boolean(), 1, {nullptr, values}, /*null_count=*/0, /*offset=*/1);
Review Comment:
Can we have a loop running `offset` from 0 to 7?
##########
cpp/src/arrow/ipc/feather.cc:
##########
@@ -561,12 +586,14 @@ struct ArrayWriterV1 {
const auto& fw_type = checked_cast<const FixedWidthType&>(*values.type());
if (prim_values.values()) {
+ if constexpr (is_boolean_type<T>::value) {
+ return WriteBitmap(prim_values.values()->data(), values.length(),
+ prim_values.offset());
+ }
const uint8_t* buffer =
prim_values.values()->data() + (prim_values.offset() * fw_type.bit_width() / 8);
- int64_t bit_offset = (prim_values.offset() * fw_type.bit_width()) % 8;
- return WriteBuffer(buffer,
- bit_util::BytesForBits(values.length() * fw_type.bit_width()),
- bit_offset);
+ return WriteBuffer(
Review Comment:
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?
##########
cpp/src/arrow/ipc/feather_test.cc:
##########
@@ -323,6 +324,30 @@ TEST_P(TestFeather, SliceBooleanRoundTrip) {
CheckSlices(batch);
}
+TEST_P(TestFeather, ExactSizeSlicedBooleanBuffer) {
+ if (GetParam().version != kFeatherV1Version) {
Review Comment:
Add a comment at top pointing to the GH issue?
##########
cpp/src/arrow/ipc/feather.cc:
##########
@@ -75,24 +75,31 @@ inline int64_t PaddedLength(int64_t nbytes) {
return ((nbytes + alignment - 1) / alignment) * alignment;
}
-Status WritePaddedWithOffset(io::OutputStream* stream, const uint8_t* data,
- int64_t bit_offset, const int64_t length,
- int64_t* bytes_written) {
+Status WritePaddedBitmap(io::OutputStream* stream, const uint8_t* data,
+ int64_t bit_offset, const int64_t bit_length,
+ int64_t* bytes_written) {
data = data + bit_offset / 8;
- uint8_t bit_shift = static_cast<uint8_t>(bit_offset % 8);
- if (bit_offset == 0) {
+ const uint8_t bit_shift = static_cast<uint8_t>(bit_offset % 8);
+ const int64_t length = bit_util::BytesForBits(bit_length);
+ if (bit_shift == 0) {
RETURN_NOT_OK(stream->Write(data, length));
} else {
constexpr int64_t buffersize = 256;
uint8_t buffer[buffersize];
const uint8_t lshift = static_cast<uint8_t>(8 - bit_shift);
+ const uint8_t* data_end =
+ data + bit_util::BytesForBits(bit_shift + bit_length);
const uint8_t* buffer_end = buffer + buffersize;
uint8_t* buffer_it = buffer;
- for (const uint8_t* end = data + length; data != end;) {
+ 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);
+ }
Review Comment:
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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org