From f31d2206a74a7eb0afc784c0fd99367ac583ad1b Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Wed, 3 Feb 2021 15:50:25 +0300 Subject: [PATCH] more fuzzing and less bugs --- programs/client/QueryFuzzer.cpp | 19 ++++++ src/Interpreters/WindowDescription.cpp | 68 +++++++++++++++++++ src/Interpreters/WindowDescription.h | 7 ++ src/Parsers/ExpressionElementParsers.cpp | 1 + src/Processors/QueryPlan/WindowStep.cpp | 1 + src/Processors/Transforms/WindowTransform.cpp | 21 ++++-- .../01591_window_functions.reference | 5 ++ .../0_stateless/01591_window_functions.sql | 2 + 8 files changed, 118 insertions(+), 6 deletions(-) diff --git a/programs/client/QueryFuzzer.cpp b/programs/client/QueryFuzzer.cpp index d569a185dba..05c20434820 100644 --- a/programs/client/QueryFuzzer.cpp +++ b/programs/client/QueryFuzzer.cpp @@ -366,6 +366,8 @@ void QueryFuzzer::fuzzWindowFrame(WindowFrame & frame) default: break; } + + frame.is_default = (frame == WindowFrame{}); } void QueryFuzzer::fuzz(ASTs & asts) @@ -465,6 +467,23 @@ void QueryFuzzer::fuzz(ASTPtr & ast) fuzz(select->children); } + /* + * The time to fuzz the settings has not yet come. + * Apparently we don't have any infractructure to validate the values of + * the settings, and the first query with max_block_size = -1 breaks + * because of overflows here and there. + *//* + * else if (auto * set = typeid_cast(ast.get())) + * { + * for (auto & c : set->changes) + * { + * if (fuzz_rand() % 50 == 0) + * { + * c.value = fuzzField(c.value); + * } + * } + * } + */ else if (auto * literal = typeid_cast(ast.get())) { // There is a caveat with fuzzing the children: many ASTs also keep the diff --git a/src/Interpreters/WindowDescription.cpp b/src/Interpreters/WindowDescription.cpp index bfb53ebb79f..3569df6fd17 100644 --- a/src/Interpreters/WindowDescription.cpp +++ b/src/Interpreters/WindowDescription.cpp @@ -33,4 +33,72 @@ std::string WindowDescription::dump() const return ss.str(); } +std::string WindowFrame::toString() const +{ + WriteBufferFromOwnString buf; + toString(buf); + return buf.str(); +} + +void WindowFrame::toString(WriteBuffer & buf) const +{ + buf << toString(type) << " BETWEEN "; + if (begin_type == BoundaryType::Current) + { + buf << "CURRENT ROW"; + } + else if (begin_type == BoundaryType::Unbounded) + { + buf << "UNBOUNDED PRECEDING"; + } + else + { + buf << abs(begin_offset); + buf << " " + << (begin_offset > 0 ? "FOLLOWING" : "PRECEDING"); + } + buf << " AND "; + if (end_type == BoundaryType::Current) + { + buf << "CURRENT ROW"; + } + else if (end_type == BoundaryType::Unbounded) + { + buf << "UNBOUNDED PRECEDING"; + } + else + { + buf << abs(end_offset); + buf << " " + << (end_offset > 0 ? "FOLLOWING" : "PRECEDING"); + } +} + +void WindowFrame::checkValid() const +{ + if (begin_type == BoundaryType::Unbounded + || end_type == BoundaryType::Unbounded) + { + return; + } + + if (begin_type == BoundaryType::Current + && end_type == BoundaryType::Offset + && end_offset > 0) + { + return; + } + + if (end_type == BoundaryType::Current + && begin_type == BoundaryType::Offset + && begin_offset < 0) + { + return; + } + + throw Exception(ErrorCodes::BAD_ARGUMENTS, + "Window frame '{}' is invalid", + toString()); +} + } diff --git a/src/Interpreters/WindowDescription.h b/src/Interpreters/WindowDescription.h index d34b7721a5e..447352f7a83 100644 --- a/src/Interpreters/WindowDescription.h +++ b/src/Interpreters/WindowDescription.h @@ -53,6 +53,13 @@ struct WindowFrame int64_t end_offset = 0; + // Throws BAD_ARGUMENTS exception if the frame definition is incorrect, e.g. + // the frame start comes later than the frame end. + void checkValid() const; + + std::string toString() const; + void toString(WriteBuffer & buf) const; + bool operator == (const WindowFrame & other) const { // We don't compare is_default because it's not a real property of the diff --git a/src/Parsers/ExpressionElementParsers.cpp b/src/Parsers/ExpressionElementParsers.cpp index e4a9c285223..c129c312d11 100644 --- a/src/Parsers/ExpressionElementParsers.cpp +++ b/src/Parsers/ExpressionElementParsers.cpp @@ -640,6 +640,7 @@ static bool tryParseFrameDefinition(ASTWindowDefinition * node, IParser::Pos & p } else if (keyword_following.ignore(pos, expected)) { + // Positive offset or UNBOUNDED FOLLOWING. } else { diff --git a/src/Processors/QueryPlan/WindowStep.cpp b/src/Processors/QueryPlan/WindowStep.cpp index 82c589b8b20..1a71ca0adc7 100644 --- a/src/Processors/QueryPlan/WindowStep.cpp +++ b/src/Processors/QueryPlan/WindowStep.cpp @@ -57,6 +57,7 @@ WindowStep::WindowStep(const DataStream & input_stream_, { // We don't remove any columns, only add, so probably we don't have to update // the output DataStream::distinct_columns. + window_description.frame.checkValid(); } void WindowStep::transformPipeline(QueryPipeline & pipeline) diff --git a/src/Processors/Transforms/WindowTransform.cpp b/src/Processors/Transforms/WindowTransform.cpp index 3dcd0a91bca..b7b0c72eb94 100644 --- a/src/Processors/Transforms/WindowTransform.cpp +++ b/src/Processors/Transforms/WindowTransform.cpp @@ -22,6 +22,8 @@ WindowTransform::WindowTransform(const Block & input_header_, , input_header(input_header_) , window_description(window_description_) { + window_description.frame.checkValid(); + workspaces.reserve(functions.size()); for (const auto & f : functions) { @@ -210,14 +212,21 @@ auto WindowTransform::moveRowNumberNoCheck(const RowNumber & _x, int offset) con break; } + // Move to the first row in current block. Note that the offset is + // negative. + offset += x.row; + x.row = 0; + + // Move to the last row of the previous block, if we are not at the + // first one. Offset also is incremented by one, because we pass over + // the first row of this block. if (x.block == first_block_number) { break; } - // offset is negative - offset += (x.row + 1); --x.block; + offset += 1; x.row = blockRowsNumber(x) - 1; } } @@ -253,10 +262,10 @@ void WindowTransform::advanceFrameStartRowsOffset() assertValid(frame_start); -// fmt::print(stderr, "frame start {} partition start {}\n", frame_start, -// partition_start); +// fmt::print(stderr, "frame start {} left {} partition start {}\n", +// frame_start, offset_left, partition_start); - if (moved_row <= partition_start) + if (frame_start <= partition_start) { // Got to the beginning of partition and can't go further back. frame_start = partition_start; @@ -269,10 +278,10 @@ void WindowTransform::advanceFrameStartRowsOffset() { // A FOLLOWING frame start ran into the end of partition. frame_started = true; + return; } assert(partition_start < frame_start); - frame_start = moved_row; frame_started = offset_left == 0; } diff --git a/tests/queries/0_stateless/01591_window_functions.reference b/tests/queries/0_stateless/01591_window_functions.reference index bd1a954ddc4..c128aae7796 100644 --- a/tests/queries/0_stateless/01591_window_functions.reference +++ b/tests/queries/0_stateless/01591_window_functions.reference @@ -558,5 +558,10 @@ settings max_block_size = 2; 28 5 3 2 1 29 5 2 1 0 30 6 1 1 0 +SELECT count(*) OVER (ROWS BETWEEN 2 PRECEDING AND CURRENT ROW) FROM numbers(4); +1 +2 +3 +3 -- seen a use-after-free under MSan in this query once SELECT number, max(number) OVER (PARTITION BY intDiv(number, 7) ORDER BY number ASC NULLS LAST ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW) FROM numbers(1024) SETTINGS max_block_size = 2 FORMAT Null; diff --git a/tests/queries/0_stateless/01591_window_functions.sql b/tests/queries/0_stateless/01591_window_functions.sql index 3a6d2f3d18a..6c2883eae26 100644 --- a/tests/queries/0_stateless/01591_window_functions.sql +++ b/tests/queries/0_stateless/01591_window_functions.sql @@ -175,6 +175,8 @@ from (select number, intDiv(number, 5) p from numbers(31)) order by p, number settings max_block_size = 2; +SELECT count(*) OVER (ROWS BETWEEN 2 PRECEDING AND CURRENT ROW) FROM numbers(4); + -- seen a use-after-free under MSan in this query once SELECT number, max(number) OVER (PARTITION BY intDiv(number, 7) ORDER BY number ASC NULLS LAST ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW) FROM numbers(1024) SETTINGS max_block_size = 2 FORMAT Null;