github-actions[bot] commented on code in PR #65754: URL: https://github.com/apache/doris/pull/65754#discussion_r3636186982
########## be/src/core/data_type_serde/arrow_validation.h: ########## @@ -0,0 +1,320 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#pragma once + +#include <arrow/array/array_base.h> +#include <arrow/array/array_binary.h> +#include <arrow/array/array_nested.h> +#include <arrow/array/array_primitive.h> + +#include <cstddef> +#include <cstdint> +#include <limits> +#include <memory> +#include <string> +#include <string_view> +#include <utility> + +#include "common/compiler_util.h" +#include "common/exception.h" +#include "util/unaligned.h" + +namespace doris { +namespace arrow_validation_detail { + +inline std::string arrow_type_name(const arrow::Array& array) { + return array.type() ? array.type()->name() : "unknown"; +} + +inline void throw_invalid_arrow(std::string_view arrow_type, std::string_view message) { + throw Exception(ErrorCode::INVALID_ARGUMENT, "Invalid Arrow {}: {}", arrow_type, message); +} + +inline void throw_invalid_arrow(const arrow::Array& array, std::string_view message) { + throw_invalid_arrow(arrow_type_name(array), message); +} + +template <typename... Args> +inline void throw_invalid_arrow(std::string_view arrow_type, std::string_view format, + Args&&... args) { + throw Exception(ErrorCode::INVALID_ARGUMENT, "Invalid Arrow {}: {}", arrow_type, + fmt::format(std::string(format), std::forward<Args>(args)...)); +} + +template <typename... Args> +inline void throw_invalid_arrow(const arrow::Array& array, std::string_view format, + Args&&... args) { + throw_invalid_arrow(arrow_type_name(array), format, std::forward<Args>(args)...); +} + +inline void check_arrow_length_and_offset(const arrow::Array& array) { + if (UNLIKELY(array.length() < 0 || array.offset() < 0)) { + throw_invalid_arrow(array, "negative length or offset: length={}, offset={}", + array.length(), array.offset()); + } +} + +inline void check_arrow_no_offset(const arrow::Array& array) { + check_arrow_length_and_offset(array); + if (UNLIKELY(array.offset() != 0)) { + throw_invalid_arrow(array, "non-zero array offset is not supported: offset={}", + array.offset()); + } +} + +inline void check_add_overflow(size_t left, size_t right, const arrow::Array& array, + std::string_view item) { + if (UNLIKELY(left > std::numeric_limits<size_t>::max() - right)) { + throw_invalid_arrow(array, "{} size overflow", item); + } +} + +inline std::shared_ptr<arrow::Int32Array> get_int32_offsets_array(const arrow::Array& array) { + auto offsets_array = static_cast<const arrow::ListArray&>(array).offsets(); + auto offsets = std::dynamic_pointer_cast<arrow::Int32Array>(offsets_array); + if (UNLIKELY(!offsets)) { + throw_invalid_arrow(array, "offsets array is not Int32Array"); + } + return offsets; +} + +} // namespace arrow_validation_detail + +inline void check_arrow_no_offset(const arrow::Array& array) { + arrow_validation_detail::check_arrow_no_offset(array); +} + +// Validate the caller's requested read range before any Arrow buffer access. +// This rejects negative or out-of-array start/end values up front. +inline void check_arrow_array_range(const arrow::Array& array, int64_t start, int64_t end) { + arrow_validation_detail::check_arrow_no_offset(array); + if (UNLIKELY(start < 0 || end < start || end > array.length())) { + arrow_validation_detail::throw_invalid_arrow( + array, "read range is invalid: start={}, end={}, length={}", start, end, + array.length()); + } +} + +// Validate buffers[0] when a validity bitmap exists. This must run before +// IsNull()/IsValid()/null_count(), because those APIs may scan the bitmap. +inline void check_arrow_validity_bitmap(const arrow::Array& array) { + arrow_validation_detail::check_arrow_length_and_offset(array); + const auto& buffers = array.data()->buffers; + if (buffers.empty() || !buffers[0]) { + if (UNLIKELY(array.data()->null_count > 0)) { + arrow_validation_detail::throw_invalid_arrow( + array, "validity bitmap is missing but null_count={}", + array.data()->null_count.load()); + } + return; + } + + const size_t offset = static_cast<size_t>(array.offset()); + const size_t length = static_cast<size_t>(array.length()); + arrow_validation_detail::check_add_overflow(offset, length, array, "validity bitmap"); + const size_t count = offset + length; + const size_t required = count / 8 + (count % 8 != 0 ? 1 : 0); + const size_t available = static_cast<size_t>(buffers[0]->size()); + if (UNLIKELY(available < required)) { + arrow_validation_detail::throw_invalid_arrow( + array, "validity bitmap too small: {} bytes available, {} required", available, + required); + } +} + +// Validate buffers[1] for fixed-width arrays before raw_values(), Value(), or +// direct buffer memcpy. elem_size is the physical byte width of one value. +inline void check_arrow_fixed_width_buffer(const arrow::Array& array, size_t elem_size) { + check_arrow_validity_bitmap(array); + const auto& buffers = array.data()->buffers; + if (UNLIKELY(buffers.size() <= 1 || !buffers[1])) { + if (array.length() == 0) { + return; + } + arrow_validation_detail::throw_invalid_arrow(array, "data buffer is missing"); + } + + const size_t offset = static_cast<size_t>(array.offset()); + const size_t length = static_cast<size_t>(array.length()); + arrow_validation_detail::check_add_overflow(offset, length, array, "data buffer"); + const size_t count = offset + length; + if (UNLIKELY(elem_size != 0 && count > std::numeric_limits<size_t>::max() / elem_size)) { + arrow_validation_detail::throw_invalid_arrow(array, "data buffer size overflow"); + } + const size_t required = elem_size * count; + const size_t available = static_cast<size_t>(buffers[1]->size()); Review Comment: [P1] Reject negative and non-addressable buffers before the size cast `Buffer::size()` is signed, but this cast turns a negative declared size into a huge `size_t`. Arrow's public `Buffer(ptr, size)` constructor does not reject it, so a length-2 `Int64Array` backed by one value and a buffer size of `-1` passes this 16-byte check and is then copied out of bounds by the Number reader. The same cast is used for validity, Boolean, binary-offset, and value buffers; a present Buffer with null CPU data also passes size-only checks. Please reject negative sizes and require addressable data whenever bytes are needed before converting to `size_t`, and add coverage for these buffer forms. ########## be/src/core/data_type_serde/data_type_string_serde.cpp: ########## @@ -418,16 +425,23 @@ Status DataTypeStringSerDeBase<ColumnType>::read_column_from_arrow( memcpy(&end_offset, offsets_data + (offset_i + 1) * offset_size, offset_size); int32_t length = end_offset - start_offset; - const auto* raw_data = buffer->data() + start_offset; - - assert_cast<ColumnType&>(column).insert_data( - reinterpret_cast<const char*>(raw_data), length); Review Comment: [P1] Validate offsets before subtracting them The attacker-controlled offsets are subtracted into `int32_t` before this check runs. With offsets `{INT32_MIN, INT32_MAX}`, `end_offset - start_offset` already causes signed-overflow UB, so the validator never gets a safe negative offset/length to reject. The number-from-string branch has the same ordering, and LargeBinary's `value_length()` performs the analogous `int64_t` subtraction. Please widen/load and validate non-negative monotonic offsets before subtracting, and add extreme-offset coverage. ########## be/src/core/data_type_serde/arrow_validation.h: ########## @@ -0,0 +1,320 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#pragma once + +#include <arrow/array/array_base.h> +#include <arrow/array/array_binary.h> +#include <arrow/array/array_nested.h> +#include <arrow/array/array_primitive.h> + +#include <cstddef> +#include <cstdint> +#include <limits> +#include <memory> +#include <string> +#include <string_view> +#include <utility> + +#include "common/compiler_util.h" +#include "common/exception.h" +#include "util/unaligned.h" + +namespace doris { +namespace arrow_validation_detail { + +inline std::string arrow_type_name(const arrow::Array& array) { + return array.type() ? array.type()->name() : "unknown"; +} + +inline void throw_invalid_arrow(std::string_view arrow_type, std::string_view message) { + throw Exception(ErrorCode::INVALID_ARGUMENT, "Invalid Arrow {}: {}", arrow_type, message); +} + +inline void throw_invalid_arrow(const arrow::Array& array, std::string_view message) { + throw_invalid_arrow(arrow_type_name(array), message); +} + +template <typename... Args> +inline void throw_invalid_arrow(std::string_view arrow_type, std::string_view format, + Args&&... args) { + throw Exception(ErrorCode::INVALID_ARGUMENT, "Invalid Arrow {}: {}", arrow_type, + fmt::format(std::string(format), std::forward<Args>(args)...)); +} + +template <typename... Args> +inline void throw_invalid_arrow(const arrow::Array& array, std::string_view format, + Args&&... args) { + throw_invalid_arrow(arrow_type_name(array), format, std::forward<Args>(args)...); +} + +inline void check_arrow_length_and_offset(const arrow::Array& array) { + if (UNLIKELY(array.length() < 0 || array.offset() < 0)) { + throw_invalid_arrow(array, "negative length or offset: length={}, offset={}", + array.length(), array.offset()); + } +} + +inline void check_arrow_no_offset(const arrow::Array& array) { + check_arrow_length_and_offset(array); + if (UNLIKELY(array.offset() != 0)) { + throw_invalid_arrow(array, "non-zero array offset is not supported: offset={}", + array.offset()); + } +} + +inline void check_add_overflow(size_t left, size_t right, const arrow::Array& array, + std::string_view item) { + if (UNLIKELY(left > std::numeric_limits<size_t>::max() - right)) { + throw_invalid_arrow(array, "{} size overflow", item); + } +} + +inline std::shared_ptr<arrow::Int32Array> get_int32_offsets_array(const arrow::Array& array) { + auto offsets_array = static_cast<const arrow::ListArray&>(array).offsets(); + auto offsets = std::dynamic_pointer_cast<arrow::Int32Array>(offsets_array); + if (UNLIKELY(!offsets)) { + throw_invalid_arrow(array, "offsets array is not Int32Array"); + } + return offsets; +} + +} // namespace arrow_validation_detail + +inline void check_arrow_no_offset(const arrow::Array& array) { + arrow_validation_detail::check_arrow_no_offset(array); +} + +// Validate the caller's requested read range before any Arrow buffer access. +// This rejects negative or out-of-array start/end values up front. +inline void check_arrow_array_range(const arrow::Array& array, int64_t start, int64_t end) { + arrow_validation_detail::check_arrow_no_offset(array); + if (UNLIKELY(start < 0 || end < start || end > array.length())) { + arrow_validation_detail::throw_invalid_arrow( + array, "read range is invalid: start={}, end={}, length={}", start, end, + array.length()); + } +} + +// Validate buffers[0] when a validity bitmap exists. This must run before +// IsNull()/IsValid()/null_count(), because those APIs may scan the bitmap. +inline void check_arrow_validity_bitmap(const arrow::Array& array) { + arrow_validation_detail::check_arrow_length_and_offset(array); + const auto& buffers = array.data()->buffers; + if (buffers.empty() || !buffers[0]) { + if (UNLIKELY(array.data()->null_count > 0)) { + arrow_validation_detail::throw_invalid_arrow( + array, "validity bitmap is missing but null_count={}", + array.data()->null_count.load()); + } + return; + } + + const size_t offset = static_cast<size_t>(array.offset()); + const size_t length = static_cast<size_t>(array.length()); + arrow_validation_detail::check_add_overflow(offset, length, array, "validity bitmap"); + const size_t count = offset + length; + const size_t required = count / 8 + (count % 8 != 0 ? 1 : 0); + const size_t available = static_cast<size_t>(buffers[0]->size()); + if (UNLIKELY(available < required)) { + arrow_validation_detail::throw_invalid_arrow( + array, "validity bitmap too small: {} bytes available, {} required", available, + required); + } +} + +// Validate buffers[1] for fixed-width arrays before raw_values(), Value(), or +// direct buffer memcpy. elem_size is the physical byte width of one value. +inline void check_arrow_fixed_width_buffer(const arrow::Array& array, size_t elem_size) { + check_arrow_validity_bitmap(array); + const auto& buffers = array.data()->buffers; + if (UNLIKELY(buffers.size() <= 1 || !buffers[1])) { + if (array.length() == 0) { + return; + } + arrow_validation_detail::throw_invalid_arrow(array, "data buffer is missing"); + } + + const size_t offset = static_cast<size_t>(array.offset()); + const size_t length = static_cast<size_t>(array.length()); + arrow_validation_detail::check_add_overflow(offset, length, array, "data buffer"); + const size_t count = offset + length; + if (UNLIKELY(elem_size != 0 && count > std::numeric_limits<size_t>::max() / elem_size)) { + arrow_validation_detail::throw_invalid_arrow(array, "data buffer size overflow"); + } + const size_t required = elem_size * count; + const size_t available = static_cast<size_t>(buffers[1]->size()); + if (UNLIKELY(available < required)) { + arrow_validation_detail::throw_invalid_arrow( + array, "data buffer too small: {} bytes available, {} required", available, + required); + } +} + +// Validate the bit-packed boolean data bitmap before BooleanArray::Value(). +inline void check_arrow_boolean_buffer(const arrow::Array& array) { + check_arrow_validity_bitmap(array); + const auto& buffers = array.data()->buffers; + if (UNLIKELY(buffers.size() <= 1 || !buffers[1])) { + if (array.length() == 0) { + return; + } + arrow_validation_detail::throw_invalid_arrow(array, "data bitmap is missing"); + } + + const size_t offset = static_cast<size_t>(array.offset()); + const size_t length = static_cast<size_t>(array.length()); + arrow_validation_detail::check_add_overflow(offset, length, array, "data bitmap"); + const size_t count = offset + length; + const size_t required = count / 8 + (count % 8 != 0 ? 1 : 0); + const size_t available = static_cast<size_t>(buffers[1]->size()); + if (UNLIKELY(available < required)) { + arrow_validation_detail::throw_invalid_arrow( + array, "data bitmap too small: {} bytes available, {} required", available, + required); + } +} + +// Validate one variable-width value range after reading offsets and before +// forming value_data() + offset. +inline void check_arrow_value_range(const arrow::Array& array, int64_t offset, int64_t length, + size_t buffer_size) { + if (UNLIKELY(offset < 0 || length < 0)) { + arrow_validation_detail::throw_invalid_arrow( + array, "value range has negative offset or length: offset={}, length={}", offset, + length); + } + const size_t safe_offset = static_cast<size_t>(offset); + const size_t safe_length = static_cast<size_t>(length); + if (UNLIKELY(safe_offset > buffer_size || safe_length > buffer_size - safe_offset)) { + arrow_validation_detail::throw_invalid_arrow( + array, "value range exceeds data buffer: offset={}, length={}, buffer_size={}", + offset, length, buffer_size); + } +} + +namespace arrow_validation_detail { + +// Offsets buffers may come from external Arrow producers through Buffer::Wrap or FFI and are not +// guaranteed to be aligned to int32_t. Do not use Int32Array::Value() here because it performs a +// typed raw_values()[i] load and can trigger UBSan on misaligned buffers. Keep this validation path +// consistent with the array/map readers below, which load offsets through unaligned_load(). +inline int32_t read_int32_offset(const arrow::Int32Array& offsets, int64_t index) { + const auto* data = reinterpret_cast<const uint8_t*>(offsets.raw_values()); + return unaligned_load<int32_t>(data + index * sizeof(int32_t)); +} + +inline int64_t check_arrow_offsets_range(const arrow::Int32Array& offsets, int64_t start, + int64_t end) { + check_arrow_array_range(offsets, 0, offsets.length()); + check_arrow_fixed_width_buffer(offsets, sizeof(int32_t)); + if (UNLIKELY(start < 0 || end < start || end >= offsets.length())) { + arrow_validation_detail::throw_invalid_arrow( + offsets, "offsets read range is invalid: start={}, end={}, offsets_length={}", + start, end, offsets.length()); + } + + int64_t previous_offset = read_int32_offset(offsets, start); + if (UNLIKELY(previous_offset < 0)) { + arrow_validation_detail::throw_invalid_arrow( + offsets, "offsets contain negative value: offset[{}]={}", start, previous_offset); + } + for (int64_t i = start + 1; i <= end; ++i) { + const int64_t current_offset = read_int32_offset(offsets, i); + if (UNLIKELY(current_offset < previous_offset)) { + arrow_validation_detail::throw_invalid_arrow( + offsets, + "offsets are not monotonically non-decreasing: offset[{}]={} < offset[{}]={}", + i, current_offset, i - 1, previous_offset); + } + previous_offset = current_offset; + } + return previous_offset; +} + +} // namespace arrow_validation_detail + +// Validate List offsets before reading offsets or recursing into values. +inline void check_arrow_list_offsets(const arrow::ListArray& array, int64_t start, int64_t end) { + check_arrow_array_range(array, start, end); + const auto offsets = arrow_validation_detail::get_int32_offsets_array(array); + const int64_t last_offset = + arrow_validation_detail::check_arrow_offsets_range(*offsets, start, end); + const int64_t values_length = array.values() ? array.values()->length() : 0; + if (UNLIKELY(last_offset > values_length)) { + arrow_validation_detail::throw_invalid_arrow( + array, "offsets exceed values length: last_offset={}, values_length={}", + last_offset, values_length); + } +} + +// Validate Map offsets before reading offsets or recursing into keys/items. +inline void check_arrow_map_offsets(const arrow::MapArray& array, int64_t start, int64_t end) { + check_arrow_array_range(array, start, end); + const auto offsets = arrow_validation_detail::get_int32_offsets_array(array); + const int64_t last_offset = + arrow_validation_detail::check_arrow_offsets_range(*offsets, start, end); + const int64_t keys_length = array.keys() ? array.keys()->length() : 0; + if (UNLIKELY(last_offset > keys_length)) { + arrow_validation_detail::throw_invalid_arrow( + array, "offsets exceed keys length: last_offset={}, keys_length={}", last_offset, + keys_length); + } + const int64_t items_length = array.items() ? array.items()->length() : 0; + if (UNLIKELY(last_offset > items_length)) { + arrow_validation_detail::throw_invalid_arrow( + array, "offsets exceed items length: last_offset={}, items_length={}", last_offset, + items_length); + } +} + +// Validate String/Binary offsets buffer before value_offset(), value_length(), +// raw_value_offsets(), or manual offset reads. Variable-width Arrow arrays need +// offset + length + 1 offset entries. +template <typename ArrowBinaryArray> +inline void check_arrow_binary_offsets_buffer(const ArrowBinaryArray& array) { + check_arrow_validity_bitmap(array); + const auto& buffers = array.data()->buffers; + if (UNLIKELY(buffers.size() <= 1 || !buffers[1])) { Review Comment: [P1] Preserve valid empty binary arrays Arrow 17 deliberately accepts a zero-length Binary/LargeBinary array with no offsets buffer (ARROW-544), and it also permits a present zero-byte offsets buffer. This helper rejects both forms with validation enabled by default: it requires `buffers[1]` here and later requires one offset entry. The LargeBinary reader previously returned successfully for `[0,0)` because it only touched offsets inside the empty loop, so valid empty RecordBatches now fail conversion. Please special-case `length() == 0` consistently with Arrow, make the 32-bit callers avoid dereferencing omitted offsets for an empty range, and add Binary/LargeBinary compatibility tests. ########## be/src/core/data_type_serde/data_type_string_serde.cpp: ########## @@ -441,13 +455,24 @@ Status DataTypeStringSerDeBase<ColumnType>::read_column_from_arrow( } else if (arrow_array->type_id() == arrow::Type::LARGE_STRING || arrow_array->type_id() == arrow::Type::LARGE_BINARY) { const auto* concrete_array = dynamic_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); + } std::shared_ptr<arrow::Buffer> buffer = concrete_array->value_data(); + const size_t buffer_size = buffer ? static_cast<size_t>(buffer->size()) : 0; for (auto offset_i = start; offset_i < end; ++offset_i) { if (!concrete_array->IsNull(offset_i)) { - const auto* raw_data = buffer->data() + concrete_array->value_offset(offset_i); Review Comment: [P1] Read LargeBinary offsets without aligned typed loads These accessors load through Arrow's `int64_t* raw_value_offsets_`. An external offset buffer wrapped at an unaligned address (for example `base + 1` with valid `{0,1}` offsets) passes the new size checks, then hits alignment UB here. The new list/map helper already notes that FFI/`Buffer::Wrap` offsets are not guaranteed aligned and uses `unaligned_load` for that reason. Please read and validate LargeBinary offsets through the same byte-oriented approach instead of the typed accessors. ########## be/src/core/data_type_serde/data_type_string_serde.cpp: ########## @@ -418,16 +425,23 @@ Status DataTypeStringSerDeBase<ColumnType>::read_column_from_arrow( memcpy(&end_offset, offsets_data + (offset_i + 1) * offset_size, offset_size); int32_t length = end_offset - start_offset; - const auto* raw_data = buffer->data() + start_offset; - - assert_cast<ColumnType&>(column).insert_data( - reinterpret_cast<const char*>(raw_data), length); + if (config::enable_arrow_input_validation) { + check_arrow_value_range(*concrete_array, start_offset, length, buffer_size); + } Review Comment: [P1] Reject a missing value buffer before dereferencing it A one-row Binary/String array with offsets `{0, 0}` and `value_data() == nullptr` still passes the new checks: `buffer_size` is 0 and `check_arrow_value_range(..., 0, 0, 0)` succeeds. This line then evaluates `buffer->data()` before `insert_data()` can ignore the pointer, so the malformed input crashes instead of producing the intended validation error. The LargeBinary branch has the same ordering. Please reject a null value buffer (or handle the zero-length value without touching it) and cover both branches with this case. ########## be/src/core/data_type_serde/data_type_ipv4_serde.cpp: ########## @@ -114,6 +116,9 @@ Status DataTypeIPv4SerDe::write_column_to_arrow(const IColumn& column, const Nul Status DataTypeIPv4SerDe::read_column_from_arrow(IColumn& column, const arrow::Array* arrow_array, int64_t start, int64_t end, const cctz::time_zone& ctz) const { + if (config::enable_arrow_input_validation) { Review Comment: [P1] Validate the fixed-width buffer before dereferencing it `check_arrow_no_offset()` checks only length/offset metadata and rejects non-zero array offsets; it does not validate `start/end` or `buffers[1]`. A valid empty `arrow::Int32Array(0, nullptr)` read over `[0,0)` therefore passes this check, but the code below still calls `buffer->data()` on the null shared pointer. Non-empty null/short buffers also pass and crash or over-read. The same no-offset-only pattern remains in the changed Date/DateTimeV2/DateV2/Decimal readers (with analogous unchecked binary/struct paths). Please run the range and physical-buffer checks before the first Arrow access and cover these newly opted-in readers. -- 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]
