Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Columns/ColumnNullable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -760,6 +760,7 @@ void ColumnNullable::shrinkToFit()
{
getNestedColumn().shrinkToFit();
getNullMapData().shrink_to_fit();
getNullMapColumn().shrinkToFit();
}

void ColumnNullable::ensureOwnership()
Expand Down
14 changes: 9 additions & 5 deletions src/Storages/MergeTree/MergeTreeReadTask.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -254,12 +254,16 @@ 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.
if (column->use_count() == 1)
column->assumeMutableRef().shrinkToFit();
/// 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
{
auto mutable_column = IColumn::mutate(std::move(column));
mutable_column->shrinkToFit();
column = std::move(mutable_column);
}
}
block = sample_block.cloneWithColumns(read_result.columns);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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
Loading