github-actions[bot] commented on code in PR #69512:
URL: https://github.com/apache/doris/pull/69512#discussion_r4357302531
##########
be/src/core/data_type/data_type_nullable.cpp:
##########
@@ -103,11 +103,60 @@ char* DataTypeNullable::serialize(const IColumn& column,
char* buf, int be_exec_
const char* DataTypeNullable::deserialize(const char* buf, MutableColumnPtr*
column,
int be_exec_version) const {
+ return deserialize(buf, nullptr, column, be_exec_version);
+}
+
+const char* DataTypeNullable::deserialize(const char* buf, const char* end,
+ MutableColumnPtr* column, int
be_exec_version) const {
auto* origin_column = column->get();
+ constexpr size_t header_size = sizeof(bool) + 2 * sizeof(size_t);
+ if (end != nullptr) {
+ if (end < buf || static_cast<size_t>(end - buf) < header_size) {
+ throw Exception(ErrorCode::INVALID_ARGUMENT,
+ "Malformed nullable column: missing serialized
header");
+ }
+ uint8_t is_const_flag = unaligned_load<uint8_t>(buf);
+ if (is_const_flag > 1) {
+ throw Exception(ErrorCode::INVALID_ARGUMENT,
+ "Malformed nullable column: invalid const flag
{}", is_const_flag);
+ }
+ bool is_const = is_const_flag != 0;
+ size_t row_num = unaligned_load<size_t>(buf + sizeof(bool));
+ size_t saved_num = unaligned_load<size_t>(buf + sizeof(bool) +
sizeof(size_t));
+ if (saved_num != (is_const ? 1 : row_num)) {
+ throw Exception(ErrorCode::INVALID_ARGUMENT,
+ "Malformed nullable column: inconsistent row count
{}, saved count {}",
+ row_num, saved_num);
+ }
+ }
+
size_t real_have_saved_num = 0;
buf = deserialize_const_flag_and_row_num(buf, column,
&real_have_saved_num);
- auto mem_size = real_have_saved_num * sizeof(bool);
+ if (end != nullptr) {
+ size_t remaining = static_cast<size_t>(end - buf);
+ size_t null_map_size = real_have_saved_num * sizeof(bool);
+ if (null_map_size <= SERIALIZED_MEM_SIZE_LIMIT) {
+ if (null_map_size > remaining) {
+ throw Exception(ErrorCode::INVALID_ARGUMENT,
+ "Malformed nullable column: truncated null
map");
+ }
+ } else {
+ if (remaining < sizeof(size_t)) {
+ throw Exception(ErrorCode::INVALID_ARGUMENT,
+ "Malformed nullable column: missing null map
length");
+ }
+ size_t encode_size = unaligned_load<size_t>(buf);
+ size_t payload_remaining = remaining - sizeof(size_t);
+ if (encode_size > payload_remaining ||
Review Comment:
[P1] Validate the compressed null-map stream before decoding
This checks the declared `encode_size`, but `streamvbyte_decode` derives its
reads from the row count and control bytes. On the 64-bit layout, a nonconstant
header with `row_num = saved_num = 257`, followed by `encode_size = 1`, one
zero byte, and a 17-byte zero-row HLL header passes this guard: 18 bytes remain
and `18 - 1 >= STREAMVBYTE_PADDING`. The decoder is then asked for
`upper_int32(257) = 65` integers, requiring at least 17 control bytes plus 65
data bytes, so it reads beyond `end` before nested HLL validation or Block's
returned-pointer check. The [pinned StreamVByte
decoder](https://raw.githubusercontent.com/lemire/streamvbyte/v1.0.0/src/streamvbyte_decode.c)
does not receive `encode_size`. Validate the control area and control-selected
data lengths against `encode_size` before decoding, and verify consumed bytes
match it; add a malformed compressed nullable-HLL case above the 256-row
threshold. This leaves the unsafe case in the newly added bounded path despite
the new
size guard.
##########
be/src/core/data_type/data_type_hll.cpp:
##########
@@ -85,8 +128,21 @@ const char* DataTypeHLL::deserialize(const char* buf,
MutableColumnPtr* column,
buf += sizeof(size_t) * real_have_saved_num;
for (int i = 0; i < real_have_saved_num; ++i) {
- data[i].deserialize(Slice(buf, hll_size_array[i]));
- buf += hll_size_array[i];
+ size_t hll_size = hll_size_array[i];
+ if (hll_size > HLL_COLUMN_DEFAULT_LEN) {
+ throw Exception(ErrorCode::INVALID_ARGUMENT,
+ "Malformed HLL column: value {} is too large: {}",
i, hll_size);
+ }
+ if (end != nullptr && hll_size > static_cast<size_t>(end - buf)) {
+ throw Exception(ErrorCode::INVALID_ARGUMENT,
+ "Malformed HLL column: value {} exceeds input: {}
> {}", i, hll_size,
+ end - buf);
+ }
+ if (!data[i].deserialize(Slice(buf, hll_size))) {
Review Comment:
[P2] Release exchange callbacks when the new HLL checks fail
The new rejection needs cleanup in its exchange caller. A one-row HLL block
with a declared value length of 1 and value byte `0x04` previously returned
normally because the false deserialize result was ignored; it now returns an
error through `Block::deserialize`. Under receiver backpressure,
`SenderQueue::get_batch` has already popped the `BlockItem` when
`RETURN_IF_ERROR(block_item.get_block(...))` exits, skipping both
`sub_blocks_memory_usage` and `block_item.call_done`. The item stores a raw
completion pointer with no destructor cleanup, and cancel/close only drain
items still queued. Consequently the RPC completion is lost and queue
accounting stays charged; on the HTTP path the `NewHttpClosure` and heap
request are also leaked because only `Run()` deletes them. Keep the validation,
but make the popped item's callback and accounting cleanup run on every
deserialize exit, and cover a rejected HLL block with a counted callback under
backpressure.
--
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]
