github-actions[bot] commented on code in PR #65715:
URL: https://github.com/apache/doris/pull/65715#discussion_r3594834924
##########
be/src/core/data_type_serde/data_type_array_serde.cpp:
##########
@@ -322,29 +322,59 @@ Status
DataTypeArraySerDe::read_column_from_arrow(IColumn& column, const arrow::
const cctz::time_zone& ctz)
const {
auto& column_array = static_cast<ColumnArray&>(column);
auto& offsets_data = column_array.get_offsets();
- const auto* concrete_array = dynamic_cast<const
arrow::ListArray*>(arrow_array);
- auto arrow_offsets_array = concrete_array->offsets();
- auto* arrow_offsets =
dynamic_cast<arrow::Int32Array*>(arrow_offsets_array.get());
- if (config::enable_arrow_input_validation) {
- check_arrow_list_offsets(*concrete_array, start, end);
- }
- auto prev_size = offsets_data.back();
- const auto* base_offsets_ptr = reinterpret_cast<const
uint8_t*>(arrow_offsets->raw_values());
- const size_t offset_element_size = sizeof(int32_t);
- const uint8_t* start_offset_ptr = base_offsets_ptr + start *
offset_element_size;
- const uint8_t* end_offset_ptr = base_offsets_ptr + end *
offset_element_size;
- auto arrow_nested_start_offset = unaligned_load<int32_t>(start_offset_ptr);
- auto arrow_nested_end_offset = unaligned_load<int32_t>(end_offset_ptr);
-
- for (auto i = start + 1; i < end + 1; ++i) {
- const uint8_t* current_offset_ptr = base_offsets_ptr + i *
offset_element_size;
- auto current_offset = unaligned_load<int32_t>(current_offset_ptr);
- // convert to doris offset, start from offsets.back()
- offsets_data.emplace_back(prev_size + current_offset -
arrow_nested_start_offset);
- }
- return nested_serde->read_column_from_arrow(
- column_array.get_data(), concrete_array->values().get(),
arrow_nested_start_offset,
- arrow_nested_end_offset, ctz);
+
+ const auto read_list = [&](const auto* concrete_array) -> Status {
+ const int64_t arrow_nested_start_offset =
concrete_array->value_offset(start);
+ const int64_t arrow_nested_end_offset =
concrete_array->value_offset(end);
+ const auto prev_size = offsets_data.back();
+ for (int64_t i = start + 1; i <= end; ++i) {
+ // Convert Arrow offsets to Doris offsets, starting at
offsets.back().
+ offsets_data.emplace_back(prev_size +
concrete_array->value_offset(i) -
+ arrow_nested_start_offset);
+ }
+ return nested_serde->read_column_from_arrow(
+ column_array.get_data(), concrete_array->values().get(),
arrow_nested_start_offset,
+ arrow_nested_end_offset, ctz);
+ };
+
+ switch (arrow_array->type_id()) {
+ case arrow::Type::LIST: {
+ const auto* concrete_array = dynamic_cast<const
arrow::ListArray*>(arrow_array);
+ if (concrete_array == nullptr) {
+ return Status::InvalidArgument("Expected Arrow ListArray, got {}",
+ arrow_array->type()->name());
+ }
+ if (config::enable_arrow_input_validation) {
+ check_arrow_list_offsets(*concrete_array, start, end);
+ }
+ return read_list(concrete_array);
+ }
+ case arrow::Type::LARGE_LIST: {
+ const auto* concrete_array = dynamic_cast<const
arrow::LargeListArray*>(arrow_array);
+ if (concrete_array == nullptr) {
+ return Status::InvalidArgument("Expected Arrow LargeListArray, got
{}",
+ arrow_array->type()->name());
+ }
+ if (config::enable_arrow_input_validation) {
+ check_arrow_array_range(*concrete_array, start, end);
Review Comment:
[P1] Validate LargeList offsets before reading them
check_arrow_array_range() only validates the logical start/end; it does not
verify LargeList's offsets-buffer capacity, monotonic/non-negative values, or
final child bound. For example, a length-1 LargeListArray backed by the single
int64 offset {0} passes this check, after which read_list calls
value_offset(end = 1) past the buffer instead of returning INVALID_ARGUMENT.
Please add a 64-bit analogue of check_arrow_list_offsets before entering the
lambda and cover short, non-monotonic, negative, and child-overrun offsets with
malformed-input tests.
##########
be/src/core/data_type_serde/data_type_array_serde.cpp:
##########
@@ -322,29 +322,59 @@ Status
DataTypeArraySerDe::read_column_from_arrow(IColumn& column, const arrow::
const cctz::time_zone& ctz)
const {
auto& column_array = static_cast<ColumnArray&>(column);
auto& offsets_data = column_array.get_offsets();
- const auto* concrete_array = dynamic_cast<const
arrow::ListArray*>(arrow_array);
- auto arrow_offsets_array = concrete_array->offsets();
- auto* arrow_offsets =
dynamic_cast<arrow::Int32Array*>(arrow_offsets_array.get());
- if (config::enable_arrow_input_validation) {
- check_arrow_list_offsets(*concrete_array, start, end);
- }
- auto prev_size = offsets_data.back();
- const auto* base_offsets_ptr = reinterpret_cast<const
uint8_t*>(arrow_offsets->raw_values());
- const size_t offset_element_size = sizeof(int32_t);
- const uint8_t* start_offset_ptr = base_offsets_ptr + start *
offset_element_size;
- const uint8_t* end_offset_ptr = base_offsets_ptr + end *
offset_element_size;
- auto arrow_nested_start_offset = unaligned_load<int32_t>(start_offset_ptr);
- auto arrow_nested_end_offset = unaligned_load<int32_t>(end_offset_ptr);
-
- for (auto i = start + 1; i < end + 1; ++i) {
- const uint8_t* current_offset_ptr = base_offsets_ptr + i *
offset_element_size;
- auto current_offset = unaligned_load<int32_t>(current_offset_ptr);
- // convert to doris offset, start from offsets.back()
- offsets_data.emplace_back(prev_size + current_offset -
arrow_nested_start_offset);
- }
- return nested_serde->read_column_from_arrow(
- column_array.get_data(), concrete_array->values().get(),
arrow_nested_start_offset,
- arrow_nested_end_offset, ctz);
+
+ const auto read_list = [&](const auto* concrete_array) -> Status {
+ const int64_t arrow_nested_start_offset =
concrete_array->value_offset(start);
Review Comment:
[P1] Keep list offset loads unaligned-safe
This lambda regresses the existing LIST path from explicit unaligned loads
to Arrow's typed value_offset() accessor. Doris's own arrow_validation.h notes
that Buffer::Wrap/FFI offsets need not be aligned, while Arrow 17 implements
value_offset() as a direct typed dereference. A valid one-row ListArray whose
{0, 1} offsets start at an odd address therefore passes validation and then
hits alignment UB here; LargeList has the same problem. Please load the
validated 32/64-bit offsets with unaligned_load and add a valid
misaligned-buffer test.
##########
be/src/core/data_type_serde/data_type_varbinary_serde.cpp:
##########
@@ -115,6 +118,76 @@ Status DataTypeVarbinarySerDe::write_column_to_arrow(const
IColumn& column, cons
return Status::OK();
}
+Status DataTypeVarbinarySerDe::read_column_from_arrow(IColumn& column,
+ const arrow::Array*
arrow_array,
+ int64_t start, int64_t
end,
+ const cctz::time_zone&
ctz) const {
+ auto& varbinary_column = assert_cast<ColumnVarbinary&>(column);
+ if (arrow_array->type_id() == arrow::Type::STRING ||
+ arrow_array->type_id() == arrow::Type::BINARY) {
+ const auto* concrete_array = assert_cast<const
arrow::BinaryArray*>(arrow_array);
+ if (config::enable_arrow_input_validation) {
+ check_arrow_array_range(*concrete_array, start, end);
+ check_arrow_binary_offsets_buffer(*concrete_array);
+ }
+ const auto& buffer = concrete_array->value_data();
Review Comment:
[P1] Validate VARBINARY value buffers and ranges
The enabled checks only prove that the offsets buffer has enough entries. A
one-row BinaryArray with offsets {0, 8} and a one-byte values buffer copies
eight bytes here, while {0, 0} with a missing values buffer dereferences
buffer->data(); LargeBinary/LargeString have the same gaps. Extreme signed
endpoints can also overflow while deriving length before a range check. Please
require value_data() to be present, load both endpoints safely, and validate
them as non-negative, non-decreasing, and within the values-buffer bound before
subtracting or forming pointers. Cover missing, oversized, reversed, and
extreme endpoints for both widths.
##########
be/src/core/data_type_serde/data_type_varbinary_serde.cpp:
##########
@@ -115,6 +118,76 @@ Status DataTypeVarbinarySerDe::write_column_to_arrow(const
IColumn& column, cons
return Status::OK();
}
+Status DataTypeVarbinarySerDe::read_column_from_arrow(IColumn& column,
+ const arrow::Array*
arrow_array,
+ int64_t start, int64_t
end,
+ const cctz::time_zone&
ctz) const {
+ auto& varbinary_column = assert_cast<ColumnVarbinary&>(column);
+ if (arrow_array->type_id() == arrow::Type::STRING ||
+ arrow_array->type_id() == arrow::Type::BINARY) {
+ const auto* concrete_array = assert_cast<const
arrow::BinaryArray*>(arrow_array);
+ if (config::enable_arrow_input_validation) {
+ check_arrow_array_range(*concrete_array, start, end);
+ check_arrow_binary_offsets_buffer(*concrete_array);
+ }
+ const auto& buffer = concrete_array->value_data();
+ const uint8_t* offsets_data = concrete_array->value_offsets()->data();
+ constexpr size_t offset_size = sizeof(int32_t);
+
+ for (int64_t offset_i = start; offset_i < end; ++offset_i) {
+ if (concrete_array->IsNull(offset_i)) {
+ varbinary_column.insert_default();
+ continue;
+ }
+ int32_t start_offset = 0;
+ int32_t end_offset = 0;
+ memcpy(&start_offset, offsets_data + offset_i * offset_size,
offset_size);
+ memcpy(&end_offset, offsets_data + (offset_i + 1) * offset_size,
offset_size);
+ const auto length = end_offset - start_offset;
+ varbinary_column.insert_data(
+ reinterpret_cast<const char*>(buffer->data() +
start_offset), length);
+ }
+ } else if (arrow_array->type_id() == arrow::Type::FIXED_SIZE_BINARY) {
+ const auto* concrete_array = assert_cast<const
arrow::FixedSizeBinaryArray*>(arrow_array);
+ if (config::enable_arrow_input_validation) {
+ check_arrow_array_range(*concrete_array, start, end);
+ check_arrow_fixed_width_buffer(*concrete_array,
+
static_cast<size_t>(concrete_array->byte_width()));
+ }
+ const auto width = concrete_array->byte_width();
+ for (int64_t offset_i = start; offset_i < end; ++offset_i) {
+ if (concrete_array->IsNull(offset_i)) {
+ varbinary_column.insert_default();
+ } else {
+ varbinary_column.insert_data(
+ reinterpret_cast<const
char*>(concrete_array->GetValue(offset_i)), width);
+ }
+ }
+ } else if (arrow_array->type_id() == arrow::Type::LARGE_STRING ||
+ arrow_array->type_id() == arrow::Type::LARGE_BINARY) {
+ const auto* concrete_array = assert_cast<const
arrow::LargeBinaryArray*>(arrow_array);
+ if (config::enable_arrow_input_validation) {
+ check_arrow_array_range(*concrete_array, start, end);
+ check_arrow_binary_offsets_buffer(*concrete_array);
+ }
+ const auto& buffer = concrete_array->value_data();
+ for (int64_t offset_i = start; offset_i < end; ++offset_i) {
+ if (concrete_array->IsNull(offset_i)) {
+ varbinary_column.insert_default();
+ continue;
+ }
+ const auto value_offset = concrete_array->value_offset(offset_i);
Review Comment:
[P1] Read LargeBinary offsets without alignment assumptions
Unlike the 32-bit branch above, this calls Arrow's typed
value_offset()/value_length() accessors. In Arrow 17 those directly dereference
an int64_t pointer, but Doris explicitly documents that external
Buffer::Wrap/FFI offsets may be unaligned. Thus even valid {0, 1} offsets at an
odd address can hit alignment UB despite the existing capacity check. Please
load both int64 offsets from value_offsets()->data() with unaligned_load (or an
equivalent shared helper) and add a valid misaligned LargeBinary test.
##########
be/src/core/data_type_serde/data_type_array_serde.cpp:
##########
@@ -322,29 +322,59 @@ Status
DataTypeArraySerDe::read_column_from_arrow(IColumn& column, const arrow::
const cctz::time_zone& ctz)
const {
auto& column_array = static_cast<ColumnArray&>(column);
auto& offsets_data = column_array.get_offsets();
- const auto* concrete_array = dynamic_cast<const
arrow::ListArray*>(arrow_array);
- auto arrow_offsets_array = concrete_array->offsets();
- auto* arrow_offsets =
dynamic_cast<arrow::Int32Array*>(arrow_offsets_array.get());
- if (config::enable_arrow_input_validation) {
- check_arrow_list_offsets(*concrete_array, start, end);
- }
- auto prev_size = offsets_data.back();
- const auto* base_offsets_ptr = reinterpret_cast<const
uint8_t*>(arrow_offsets->raw_values());
- const size_t offset_element_size = sizeof(int32_t);
- const uint8_t* start_offset_ptr = base_offsets_ptr + start *
offset_element_size;
- const uint8_t* end_offset_ptr = base_offsets_ptr + end *
offset_element_size;
- auto arrow_nested_start_offset = unaligned_load<int32_t>(start_offset_ptr);
- auto arrow_nested_end_offset = unaligned_load<int32_t>(end_offset_ptr);
-
- for (auto i = start + 1; i < end + 1; ++i) {
- const uint8_t* current_offset_ptr = base_offsets_ptr + i *
offset_element_size;
- auto current_offset = unaligned_load<int32_t>(current_offset_ptr);
- // convert to doris offset, start from offsets.back()
- offsets_data.emplace_back(prev_size + current_offset -
arrow_nested_start_offset);
- }
- return nested_serde->read_column_from_arrow(
- column_array.get_data(), concrete_array->values().get(),
arrow_nested_start_offset,
- arrow_nested_end_offset, ctz);
+
+ const auto read_list = [&](const auto* concrete_array) -> Status {
+ const int64_t arrow_nested_start_offset =
concrete_array->value_offset(start);
+ const int64_t arrow_nested_end_offset =
concrete_array->value_offset(end);
+ const auto prev_size = offsets_data.back();
+ for (int64_t i = start + 1; i <= end; ++i) {
+ // Convert Arrow offsets to Doris offsets, starting at
offsets.back().
+ offsets_data.emplace_back(prev_size +
concrete_array->value_offset(i) -
+ arrow_nested_start_offset);
+ }
+ return nested_serde->read_column_from_arrow(
+ column_array.get_data(), concrete_array->values().get(),
arrow_nested_start_offset,
+ arrow_nested_end_offset, ctz);
+ };
+
+ switch (arrow_array->type_id()) {
+ case arrow::Type::LIST: {
+ const auto* concrete_array = dynamic_cast<const
arrow::ListArray*>(arrow_array);
+ if (concrete_array == nullptr) {
+ return Status::InvalidArgument("Expected Arrow ListArray, got {}",
+ arrow_array->type()->name());
+ }
+ if (config::enable_arrow_input_validation) {
+ check_arrow_list_offsets(*concrete_array, start, end);
+ }
+ return read_list(concrete_array);
+ }
+ case arrow::Type::LARGE_LIST: {
+ const auto* concrete_array = dynamic_cast<const
arrow::LargeListArray*>(arrow_array);
+ if (concrete_array == nullptr) {
+ return Status::InvalidArgument("Expected Arrow LargeListArray, got
{}",
+ arrow_array->type()->name());
+ }
+ if (config::enable_arrow_input_validation) {
+ check_arrow_array_range(*concrete_array, start, end);
+ }
+ return read_list(concrete_array);
+ }
+ case arrow::Type::FIXED_SIZE_LIST: {
+ const auto* concrete_array = dynamic_cast<const
arrow::FixedSizeListArray*>(arrow_array);
+ if (concrete_array == nullptr) {
+ return Status::InvalidArgument("Expected Arrow FixedSizeListArray,
got {}",
+ arrow_array->type()->name());
+ }
+ if (config::enable_arrow_input_validation) {
+ check_arrow_array_range(*concrete_array, start, end);
Review Comment:
[P1] Validate FixedSizeList offset arithmetic
This only checks the parent row range, but Arrow computes value_offset(i) as
signed list_size * i with no overflow check. A constructible FixedSizeList with
list_size = INT32_MAX and logical length/end 4,294,967,299 passes this check,
then overflows in value_offset(end) before the nested serde can reject the
empty/truncated child. Please compute start/end offsets with overflow-safe
arithmetic and validate child coverage before read_list, with a
huge-logical-length malformed test that requires no large allocation.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
