Skip to content

Commit 71ebd0e

Browse files
EnmkEnmk
authored
Merge pull request #273 from ClickHouse/Enmk-issue-266-test
Fix crash on invalid AST
2 parents 5bc1985 + 262b152 commit 71ebd0e

File tree

6 files changed

+146
-24
lines changed

6 files changed

+146
-24
lines changed

‎clickhouse/columns/factory.cpp‎

Lines changed: 33 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,26 @@
2323
#include "../exceptions.h"
2424

2525
#include <stdexcept>
26+
#include <string>
2627

2728
namespace clickhouse {
2829
namespace {
2930

31+
// Like Python's list's []:
32+
// * 0 - first element
33+
// * 1 - second element
34+
// * -1 - last element
35+
// * -2 - one before last, etc.
36+
const auto& GetASTChildElement(const TypeAst & ast, int position) {
37+
if (static_cast<size_t>(abs(position)) >= ast.elements.size())
38+
throw ValidationError("AST child element index out of bounds: " + std::to_string(position));
39+
40+
if (position < 0)
41+
position = ast.elements.size() + position;
42+
43+
return ast.elements[static_cast<size_t>(position)];
44+
}
45+
3046
static ColumnRef CreateTerminalColumn(const TypeAst& ast) {
3147
switch (ast.code) {
3248
case Type::Void:
@@ -58,24 +74,24 @@ static ColumnRef CreateTerminalColumn(const TypeAst& ast) {
5874
return std::make_shared<ColumnFloat64>();
5975

6076
case Type::Decimal:
61-
return std::make_shared<ColumnDecimal>(ast.elements.front().value, ast.elements.back().value);
77+
return std::make_shared<ColumnDecimal>(GetASTChildElement(ast, 0).value, GetASTChildElement(ast, -1).value);
6278
case Type::Decimal32:
63-
return std::make_shared<ColumnDecimal>(9, ast.elements.front().value);
79+
return std::make_shared<ColumnDecimal>(9, GetASTChildElement(ast, 0).value);
6480
case Type::Decimal64:
65-
return std::make_shared<ColumnDecimal>(18, ast.elements.front().value);
81+
return std::make_shared<ColumnDecimal>(18, GetASTChildElement(ast, 0).value);
6682
case Type::Decimal128:
67-
return std::make_shared<ColumnDecimal>(38, ast.elements.front().value);
83+
return std::make_shared<ColumnDecimal>(38, GetASTChildElement(ast, 0).value);
6884

6985
case Type::String:
7086
return std::make_shared<ColumnString>();
7187
case Type::FixedString:
72-
return std::make_shared<ColumnFixedString>(ast.elements.front().value);
88+
return std::make_shared<ColumnFixedString>(GetASTChildElement(ast, 0).value);
7389

7490
case Type::DateTime:
7591
if (ast.elements.empty()) {
7692
return std::make_shared<ColumnDateTime>();
7793
} else {
78-
return std::make_shared<ColumnDateTime>(ast.elements[0].value_string);
94+
return std::make_shared<ColumnDateTime>(GetASTChildElement(ast, 0).value_string);
7995
}
8096
case Type::DateTime64:
8197
if (ast.elements.empty()) {
@@ -120,13 +136,13 @@ static ColumnRef CreateColumnFromAst(const TypeAst& ast, CreateColumnByTypeSetti
120136
switch (ast.meta) {
121137
case TypeAst::Array: {
122138
return std::make_shared<ColumnArray>(
123-
CreateColumnFromAst(ast.elements.front(), settings)
139+
CreateColumnFromAst(GetASTChildElement(ast, 0), settings)
124140
);
125141
}
126142

127143
case TypeAst::Nullable: {
128144
return std::make_shared<ColumnNullable>(
129-
CreateColumnFromAst(ast.elements.front(), settings),
145+
CreateColumnFromAst(GetASTChildElement(ast, 0), settings),
130146
std::make_shared<ColumnUInt8>()
131147
);
132148
}
@@ -159,9 +175,10 @@ static ColumnRef CreateColumnFromAst(const TypeAst& ast, CreateColumnByTypeSetti
159175

160176
enum_items.reserve(ast.elements.size() / 2);
161177
for (size_t i = 0; i < ast.elements.size(); i += 2) {
162-
enum_items.push_back(
163-
Type::EnumItem{ ast.elements[i].value_string,
164-
(int16_t)ast.elements[i + 1].value });
178+
enum_items.push_back(Type::EnumItem{
179+
ast.elements[i].value_string,
180+
static_cast<int16_t>(ast.elements[i + 1].value)
181+
});
165182
}
166183

167184
if (ast.code == Type::Enum8) {
@@ -176,14 +193,14 @@ static ColumnRef CreateColumnFromAst(const TypeAst& ast, CreateColumnByTypeSetti
176193
break;
177194
}
178195
case TypeAst::LowCardinality: {
179-
const auto nested = ast.elements.front();
196+
const auto nested = GetASTChildElement(ast, 0);
180197
if (settings.low_cardinality_as_wrapped_column) {
181198
switch (nested.code) {
182199
// TODO (nemkov): update this to maximize code reuse.
183200
case Type::String:
184201
return std::make_shared<LowCardinalitySerializationAdaptor<ColumnString>>();
185202
case Type::FixedString:
186-
return std::make_shared<LowCardinalitySerializationAdaptor<ColumnFixedString>>(nested.elements.front().value);
203+
return std::make_shared<LowCardinalitySerializationAdaptor<ColumnFixedString>>(GetASTChildElement(nested, 0).value);
187204
case Type::Nullable:
188205
throw UnimplementedError("LowCardinality(" + nested.name + ") is not supported with LowCardinalityAsWrappedColumn on");
189206
default:
@@ -196,11 +213,11 @@ static ColumnRef CreateColumnFromAst(const TypeAst& ast, CreateColumnByTypeSetti
196213
case Type::String:
197214
return std::make_shared<ColumnLowCardinalityT<ColumnString>>();
198215
case Type::FixedString:
199-
return std::make_shared<ColumnLowCardinalityT<ColumnFixedString>>(nested.elements.front().value);
216+
return std::make_shared<ColumnLowCardinalityT<ColumnFixedString>>(GetASTChildElement(nested, 0).value);
200217
case Type::Nullable:
201218
return std::make_shared<ColumnLowCardinality>(
202219
std::make_shared<ColumnNullable>(
203-
CreateColumnFromAst(nested.elements.front(), settings),
220+
CreateColumnFromAst(GetASTChildElement(nested, 0), settings),
204221
std::make_shared<ColumnUInt8>()
205222
)
206223
);
@@ -210,7 +227,7 @@ static ColumnRef CreateColumnFromAst(const TypeAst& ast, CreateColumnByTypeSetti
210227
}
211228
}
212229
case TypeAst::SimpleAggregateFunction: {
213-
return CreateTerminalColumn(ast.elements.back());
230+
return CreateTerminalColumn(GetASTChildElement(ast, -1));
214231
}
215232

216233
case TypeAst::Map: {

‎clickhouse/types/type_parser.cpp‎

Lines changed: 51 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,21 @@
11
#include "type_parser.h"
22

3+
#include "clickhouse/exceptions.h"
4+
#include "clickhouse/base/platform.h" // for _win_
5+
36
#include <algorithm>
7+
#include <cmath>
48
#include <map>
59
#include <mutex>
610
#include <unordered_map>
711

12+
#if defined _win_
13+
#include <string.h>
14+
#else
15+
#include <strings.h>
16+
#endif
17+
18+
819
namespace clickhouse {
920

1021
bool TypeAst::operator==(const TypeAst & other) const {
@@ -16,6 +27,7 @@ bool TypeAst::operator==(const TypeAst & other) const {
1627
}
1728

1829
static const std::unordered_map<std::string, Type::Code> kTypeCode = {
30+
{ "Void", Type::Void },
1931
{ "Int8", Type::Int8 },
2032
{ "Int16", Type::Int16 },
2133
{ "Int32", Type::Int32 },
@@ -41,23 +53,38 @@ static const std::unordered_map<std::string, Type::Code> kTypeCode = {
4153
{ "IPv4", Type::IPv4 },
4254
{ "IPv6", Type::IPv6 },
4355
{ "Int128", Type::Int128 },
56+
// { "UInt128", Type::UInt128 },
4457
{ "Decimal", Type::Decimal },
4558
{ "Decimal32", Type::Decimal32 },
4659
{ "Decimal64", Type::Decimal64 },
4760
{ "Decimal128", Type::Decimal128 },
4861
{ "LowCardinality", Type::LowCardinality },
49-
{ "Map", Type::Map},
50-
{ "Point", Type::Point},
51-
{ "Ring", Type::Ring},
52-
{ "Polygon", Type::Polygon},
53-
{ "MultiPolygon", Type::MultiPolygon},
62+
{ "Map", Type::Map },
63+
{ "Point", Type::Point },
64+
{ "Ring", Type::Ring },
65+
{ "Polygon", Type::Polygon },
66+
{ "MultiPolygon", Type::MultiPolygon },
5467
};
5568

69+
template <typename L, typename R>
70+
inline int CompateStringsCaseInsensitive(const L& left, const R& right) {
71+
int64_t size_diff = left.size() - right.size();
72+
if (size_diff != 0)
73+
return size_diff > 0 ? 1 : -1;
74+
75+
#if defined _win_
76+
return _strnicmp(left.data(), right.data(), left.size());
77+
#else
78+
return strncasecmp(left.data(), right.data(), left.size());
79+
#endif
80+
}
81+
5682
static Type::Code GetTypeCode(const std::string& name) {
5783
auto it = kTypeCode.find(name);
5884
if (it != kTypeCode.end()) {
5985
return it->second;
6086
}
87+
6188
return Type::Void;
6289
}
6390

@@ -97,6 +124,17 @@ static TypeAst::Meta GetTypeMeta(const StringView& name) {
97124
return TypeAst::Terminal;
98125
}
99126

127+
bool ValidateAST(const TypeAst& ast) {
128+
// Void terminal that is not actually "void" produced when unknown type is encountered.
129+
if (ast.meta == TypeAst::Terminal
130+
&& ast.code == Type::Void
131+
&& CompateStringsCaseInsensitive(ast.name, std::string_view("void")) != 0)
132+
//throw UnimplementedError("Unsupported type: " + ast.name);
133+
return false;
134+
135+
return true;
136+
}
137+
100138

101139
TypeParser::TypeParser(const StringView& name)
102140
: cur_(name.data())
@@ -111,6 +149,7 @@ bool TypeParser::Parse(TypeAst* type) {
111149
type_ = type;
112150
open_elements_.push(type_);
113151

152+
size_t processed_tokens = 0;
114153
do {
115154
const Token & token = NextToken();
116155
switch (token.type) {
@@ -159,11 +198,17 @@ bool TypeParser::Parse(TypeAst* type) {
159198
// Ubalanced braces, brackets, etc is an error.
160199
if (open_elements_.size() != 1)
161200
return false;
162-
return true;
201+
202+
// Empty input string, no tokens produced
203+
if (processed_tokens == 0)
204+
return false;
205+
206+
return ValidateAST(*type);
163207
}
164208
case Token::Invalid:
165209
return false;
166210
}
211+
++processed_tokens;
167212
} while (true);
168213
}
169214

‎ut/CreateColumnByType_ut.cpp‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,12 @@ TEST(CreateColumnByType, DateTime) {
5050
ASSERT_EQ(CreateColumnByType("DateTime64(3, 'UTC')")->As<ColumnDateTime64>()->Timezone(), "UTC");
5151
}
5252

53+
TEST(CreateColumnByType, AggregateFunction) {
54+
EXPECT_EQ(nullptr, CreateColumnByType("AggregateFunction(argMax, Int32, DateTime64(3))"));
55+
EXPECT_EQ(nullptr, CreateColumnByType("AggregateFunction(argMax, FIxedString(10), DateTime64(3, 'UTC'))"));
56+
}
57+
58+
5359
class CreateColumnByTypeWithName : public ::testing::TestWithParam<const char* /*Column Type String*/>
5460
{};
5561

‎ut/client_ut.cpp‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1313,6 +1313,25 @@ TEST_P(ClientCase, OnProfileEvents) {
13131313
}
13141314
}
13151315

1316+
TEST_P(ClientCase, SelectAggregateFunction) {
1317+
// Verifies that perofing SELECT value of type AggregateFunction(...) doesn't crash the client.
1318+
// For details: https://github.com/ClickHouse/clickhouse-cpp/issues/266
1319+
client_->Execute("CREATE TEMPORARY TABLE IF NOT EXISTS tableplus_crash_example (col AggregateFunction(argMax, Int32, DateTime(3))) engine = Memory");
1320+
client_->Execute("insert into tableplus_crash_example values (unhex('010000000001089170A883010000'))");
1321+
1322+
client_->Select("select version()",
1323+
[&](const Block& block) {
1324+
std::cerr << PrettyPrintBlock{block} << std::endl;
1325+
});
1326+
1327+
// Column type `AggregateFunction` is not supported.
1328+
EXPECT_THROW(client_->Select("select toTypeName(col), col from tableplus_crash_example",
1329+
[&](const Block& block) {
1330+
std::cerr << PrettyPrintBlock{block} << std::endl;
1331+
}), clickhouse::UnimplementedError);
1332+
}
1333+
1334+
13161335
const auto LocalHostEndpoint = ClientOptions()
13171336
.SetHost( getEnvOrDefault("CLICKHOUSE_HOST", "localhost"))
13181337
.SetPort( getEnvOrDefault<size_t>("CLICKHOUSE_PORT", "9000"))

‎ut/type_parser_ut.cpp‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -239,3 +239,34 @@ TEST(TypeParserCase, ParseMap) {
239239
ASSERT_EQ(ast.elements[1].meta, TypeAst::Terminal);
240240
ASSERT_EQ(ast.elements[1].name, "String");
241241
}
242+
243+
TEST(TypeParser, EmptyName) {
244+
{
245+
TypeAst ast;
246+
EXPECT_EQ(false, TypeParser("").Parse(&ast));
247+
}
248+
249+
{
250+
TypeAst ast;
251+
EXPECT_EQ(false, TypeParser(" ").Parse(&ast));
252+
}
253+
}
254+
255+
TEST(ParseTypeName, EmptyName) {
256+
// Empty and invalid names shouldn't produce any AST and shoudn't crash
257+
EXPECT_EQ(nullptr, ParseTypeName(""));
258+
EXPECT_EQ(nullptr, ParseTypeName(" "));
259+
EXPECT_EQ(nullptr, ParseTypeName(std::string(5, '\0')));
260+
}
261+
262+
TEST(TypeParser, AggregateFunction) {
263+
{
264+
TypeAst ast;
265+
EXPECT_FALSE(TypeParser("AggregateFunction(argMax, Int32, DateTime(3))").Parse(&ast));
266+
}
267+
268+
{
269+
TypeAst ast;
270+
EXPECT_FALSE(TypeParser("AggregateFunction(argMax, LowCardinality(Nullable(FixedString(4))), DateTime(3, 'UTC'))").Parse(&ast));
271+
}
272+
}

‎ut/types_ut.cpp‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ TEST(TypesCase, IsEqual) {
8080
const std::string type_names[] = {
8181
"UInt8",
8282
"Int8",
83-
"UInt128",
83+
// "UInt128",
8484
"String",
8585
"FixedString(0)",
8686
"FixedString(10000)",
@@ -128,7 +128,11 @@ TEST(TypesCase, IsEqual) {
128128
EXPECT_TRUE(type->IsEqual(*type));
129129

130130
for (const auto & other_type_name : type_names) {
131-
const auto other_type = clickhouse::CreateColumnByType(other_type_name)->Type();
131+
SCOPED_TRACE(other_type_name);
132+
const auto other_column = clickhouse::CreateColumnByType(other_type_name);
133+
ASSERT_NE(nullptr, other_column);
134+
135+
const auto other_type = other_column->Type();
132136

133137
const auto should_be_equal = type_name == other_type_name;
134138
EXPECT_EQ(should_be_equal, type->IsEqual(other_type))

0 commit comments

Comments
 (0)