diff --git a/src/Columns/ColumnNullable.cpp b/src/Columns/ColumnNullable.cpp index 7c1047b29abb..85b68e4994b1 100644 --- a/src/Columns/ColumnNullable.cpp +++ b/src/Columns/ColumnNullable.cpp @@ -759,7 +759,7 @@ void ColumnNullable::prepareForSquashing(const Columns & source_columns, size_t void ColumnNullable::shrinkToFit() { getNestedColumn().shrinkToFit(); - getNullMapData().shrink_to_fit(); + getNullMapColumn().shrinkToFit(); } void ColumnNullable::ensureOwnership() diff --git a/src/Storages/MergeTree/MergeTreeReadTask.cpp b/src/Storages/MergeTree/MergeTreeReadTask.cpp index 0e8b05ba7957..bddd3cf7af88 100644 --- a/src/Storages/MergeTree/MergeTreeReadTask.cpp +++ b/src/Storages/MergeTree/MergeTreeReadTask.cpp @@ -254,12 +254,17 @@ MergeTreeReadTask::BlockAndProgress MergeTreeReadTask::read() Block block; if (read_result.num_rows != 0) { - for (const auto & column : read_result.columns) + for (auto & column : read_result.columns) { - /// We may have columns that has other references, usually it is a constant column that has been created during analysis - /// (that will not be const here anymore, i.e. after materialize()), and we do not need to shrink it anyway. + /// We may have columns that have other references, usually it is a constant column that has been created during analysis + /// (that will not be const here anymore, i.e. after materialize()). The contract is - not to shrink if column is shared. + /// But if some subcolumns are shared, we'll clone them via IColumn::mutate() and then safely shrink if (column->use_count() == 1) - column->assumeMutableRef().shrinkToFit(); + { + auto mutable_column = IColumn::mutate(std::move(column)); + mutable_column->shrinkToFit(); + column = std::move(mutable_column); + } } block = sample_block.cloneWithColumns(read_result.columns); } diff --git a/tests/queries/0_stateless/03680_mergetree_shrink_const_from_prewhere_repetitive.reference b/tests/queries/0_stateless/03680_mergetree_shrink_const_from_prewhere_repetitive.reference new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/queries/0_stateless/03680_mergetree_shrink_const_from_prewhere_repetitive.sh b/tests/queries/0_stateless/03680_mergetree_shrink_const_from_prewhere_repetitive.sh new file mode 100755 index 000000000000..d65d3ecaa74d --- /dev/null +++ b/tests/queries/0_stateless/03680_mergetree_shrink_const_from_prewhere_repetitive.sh @@ -0,0 +1,53 @@ +#!/usr/bin/env bash + +CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +# shellcheck source=../shell_config.sh +. "$CURDIR"/../shell_config.sh + +# This test covers #88605 and #90695 - different occurences of the same issue - calling shrinkToFit on a shared column. +# Due to flaky nature of the problem, the test repeats same queries multiple times in order the trigger a simultanuous +# attempt to shrink at several exec threads. +# This test complements 03680_mergetree_shrink_const_from_prewhere.sql +# +# #88605 (const_node_1): Here we have condition with a constant "materialize(255)", for which convertToFullColumnIfConst() will return underlying column w/o copying, +# and later shrinkToFit() will be called from multiple threads on this column, and leads to UB +# #90695 (const_node_2): The combination of "materialize()" with "and()" and "toNullable()" creates nested column and a chain, where double-free happened at shrinkToFit() + +setup() { + $CLICKHOUSE_CLIENT -q " + DROP TABLE IF EXISTS const_node_1; + DROP TABLE IF EXISTS const_node_2; + + CREATE TABLE const_node_1 (v Nullable(UInt8)) ENGINE = MergeTree ORDER BY tuple(); + SYSTEM STOP MERGES const_node_1; + INSERT INTO const_node_1 VALUES (1); + INSERT INTO const_node_1 VALUES (2); + INSERT INTO const_node_1 VALUES (3); + + DROP TABLE IF EXISTS const_node_2; + CREATE TABLE const_node_2 (x Int16) ENGINE = MergeTree PARTITION BY (x) ORDER BY x; + INSERT INTO const_node_2 VALUES (1), (2), (1), (3); + " +} + +run_queries() { + # During 30 seconds we gonna hammer server with these SELECT queries. Before the fix, it'd crash with high probability. Not crashing is the expected success. + local TIMELIMIT=$((SECONDS+30)) + while [ $SECONDS -lt "$TIMELIMIT" ]; do + $CLICKHOUSE_CLIENT -q " + SELECT v FROM const_node_1 PREWHERE and(materialize(255), *) ORDER BY v FORMAT NULL; + SELECT median(3) IGNORE NULLS FROM const_node_2 PREWHERE and(materialize(toNullable(materialize(1))), not(materialize(100) = *)) FORMAT NULL; + " + done +} + +cleanup() { + $CLICKHOUSE_CLIENT -q " + DROP TABLE IF EXISTS const_node_1; + DROP TABLE IF EXISTS const_node_2; + " +} + +setup +run_queries +cleanup diff --git a/tests/queries/0_stateless/03681_mergetree_shrink_const_nested_nullable.reference b/tests/queries/0_stateless/03681_mergetree_shrink_const_nested_nullable.reference new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/queries/0_stateless/03681_mergetree_shrink_const_nested_nullable.sh b/tests/queries/0_stateless/03681_mergetree_shrink_const_nested_nullable.sh new file mode 100755 index 000000000000..d9ea83c4f9c6 --- /dev/null +++ b/tests/queries/0_stateless/03681_mergetree_shrink_const_nested_nullable.sh @@ -0,0 +1,42 @@ +#!/usr/bin/env bash + +CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +# shellcheck source=../shell_config.sh +. "$CURDIR"/../shell_config.sh + +# This test covers #90695: a constant inside PREWHERE, wrapped in materialize(toNullable(materialize(...))), +# creates a ColumnNullable whose top-level use_count() == 1 but whose *nested* column is shared with the +# query-wide ActionsDAG. The old `if (column->use_count() == 1) column->assumeMutableRef().shrinkToFit();` +# guard in MergeTreeReadTask::read() only checks the top-level refcount, so it still shrinks (realloc's) +# the shared nested column in place from multiple threads, causing a double-free / heap corruption. +# +# Fixture: 8 partitions of 1 row each, so ColumnConst::convertToFullColumn()'s `if (s == 1) return data;` +# shortcut hands out the shared one-element column instead of copying it. + +setup() { + $CLICKHOUSE_CLIENT -q " + DROP TABLE IF EXISTS const_nested_nullable; + CREATE TABLE const_nested_nullable (x Int16) ENGINE = MergeTree PARTITION BY x ORDER BY x; + INSERT INTO const_nested_nullable VALUES (1), (2), (3), (4), (5), (6), (7), (8); + " +} + +run_queries() { + # During 30 seconds we gonna hammer server with these SELECT queries. Before the fix, it'd crash with high probability. Not crashing is the expected success. + local TIMELIMIT=$((SECONDS+30)) + while [ $SECONDS -lt "$TIMELIMIT" ]; do + $CLICKHOUSE_CLIENT -q " + SELECT median(3) IGNORE NULLS FROM const_nested_nullable PREWHERE and(materialize(toNullable(materialize(1))), not(materialize(100) = *)) FORMAT NULL; + " + done +} + +cleanup() { + $CLICKHOUSE_CLIENT -q " + DROP TABLE IF EXISTS const_nested_nullable; + " +} + +setup +run_queries +cleanup diff --git a/tests/queries/0_stateless/03682_mergetree_shrink_const_array_float64.reference b/tests/queries/0_stateless/03682_mergetree_shrink_const_array_float64.reference new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/queries/0_stateless/03682_mergetree_shrink_const_array_float64.sh b/tests/queries/0_stateless/03682_mergetree_shrink_const_array_float64.sh new file mode 100755 index 000000000000..ec405c9bdb53 --- /dev/null +++ b/tests/queries/0_stateless/03682_mergetree_shrink_const_array_float64.sh @@ -0,0 +1,46 @@ +#!/usr/bin/env bash + +CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +# shellcheck source=../shell_config.sh +. "$CURDIR"/../shell_config.sh + +# This test covers a variant of #90695/#92588 via ColumnArray instead of ColumnNullable: arrayMap's lambda +# body evaluates a bare Float64 literal, which the analyzer treats as a per-block constant. When the array +# has a single element, ColumnConst::convertToFullColumn()'s `if (s == 1) return data;` shortcut hands out +# a shared literal column as the arrayMap result's *nested* data column, wrapped in a freshly-built +# ColumnArray whose top-level use_count() == 1. The old +# `if (column->use_count() == 1) column->assumeMutableRef().shrinkToFit();` guard in +# MergeTreeReadTask::read() only checks that top-level refcount, so ColumnArray::shrinkToFit() still +# shrinks (realloc's) the shared nested ColumnVector in place from multiple threads, causing a +# double-free / heap corruption. +# +# Fixture: 8 partitions of 1 row each, so ColumnConst::convertToFullColumn()'s `if (s == 1) return data;` +# shortcut hands out the shared one-element column instead of copying it. + +setup() { + $CLICKHOUSE_CLIENT -q " + DROP TABLE IF EXISTS const_array_float64; + CREATE TABLE const_array_float64 (x Int16) ENGINE = MergeTree PARTITION BY x ORDER BY x; + INSERT INTO const_array_float64 VALUES (1), (2), (3), (4), (5), (6), (7), (8); + " +} + +run_queries() { + # During 30 seconds we gonna hammer server with these SELECT queries. Before the fix, it'd crash with high probability. Not crashing is the expected success. + local TIMELIMIT=$((SECONDS+30)) + while [ $SECONDS -lt "$TIMELIMIT" ]; do + $CLICKHOUSE_CLIENT -q " + SELECT arrayMap(v -> 1., [x]) FROM const_array_float64 PREWHERE and(materialize(toNullable(materialize(1))), not(materialize(100) = x)) FORMAT NULL; + " + done +} + +cleanup() { + $CLICKHOUSE_CLIENT -q " + DROP TABLE IF EXISTS const_array_float64; + " +} + +setup +run_queries +cleanup