[GH-48691][C++] Write serializer could be crash if the value buffer is empty #48692
Conversation
|
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or See also: |
github-actions
bot
added
Component: Parquet
Component: C++
awaiting review
|
@wgtmac Hi, could you please help review this, thanks! |
| // Set all bits to 0 (null) | ||
| ::arrow::bit_util::SetBitsTo(null_bitmap->mutable_data(), 0, 100, false); | ||
|
|
||
| std::shared_ptr<::arrow::Buffer> data_buffer = nullptr; |
There was a problem hiding this comment.
Please correct me if I was wrong. I think the Arrow spec is vague on whether the value buffer can be null if all values are null. It also escapes the Array::Validate check as in
arrow/cpp/src/arrow/array/validate.cc
Lines 505 to 507 in abbcd53
If this violates the spec, is it better to fix Array::Validate() and calls it before calling functor.Serialize()?
There was a problem hiding this comment.
Arrow spec is vague on whether the value buffer can be null
Yes, that also confuses me, I think we don't support null value buffer but accept empty value buffer if it's all nulls in the batch? Seems we're avoiding null value buffers: https://github.com/apache/arrow/pull/2243/changes
There was a problem hiding this comment.
IIRC it's deliberate that a null buffer pointer is accepted there. I would rather not have this but it could break compatibility with existing usage.
In any case, feel free to open a separate issue about it.
github-actions
bot
added
awaiting committer review
| SerializeFunctor<ParquetType, ArrowType> functor; | ||
| RETURN_NOT_OK(functor.Serialize(checked_cast<const ArrayType&>(array), ctx, buffer)); | ||
| // The value buffer could be empty if all values are nulls. | ||
| if (array.null_count() != array.length()) { |
There was a problem hiding this comment.
In case of an invalid arrow array, value buffer can still be nullptr when array.null_count() == array.length() which crashes the following call.
There was a problem hiding this comment.
But we don't care about invalid Arrow arrays here, do we?
| ASSERT_OK_AND_ASSIGN(null_bitmap, ::arrow::AllocateBitmap(100)); | ||
| // Set all bits to 0 (null) | ||
| ::arrow::bit_util::SetBitsTo(null_bitmap->mutable_data(), 0, 100, false); |
There was a problem hiding this comment.
You can just use AllocateEmptyBitmap which will zero-initialize the bitmap.
| // Set all bits to 0 (null) | ||
| ::arrow::bit_util::SetBitsTo(null_bitmap->mutable_data(), 0, 100, false); | ||
|
|
||
| std::shared_ptr<::arrow::Buffer> data_buffer = nullptr; |
There was a problem hiding this comment.
IIRC it's deliberate that a null buffer pointer is accepted there. I would rather not have this but it could break compatibility with existing usage.
In any case, feel free to open a separate issue about it.
3 participants
Rationale for this change
WriteArrowSerialize could unconditionally read values from the Arrow array even for null rows. Since it's possible the caller could provided a zero-sized dummy buffer for all-null arrays, this caused an ASAN heap-buffer-overflow.
What changes are included in this PR?
Early check the array is not all null values before serialize it
Are these changes tested?
Added tests.
Are there any user-facing changes?
No