Skip to content

Reject LowCardinality indexes outside the dictionary #593

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
iliaal wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Reject LowCardinality indexes outside the dictionary #593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
40 changes: 36 additions & 4 deletions clickhouse/columns/lowcardinality.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ enum IndexFlag {
NeedUpdateDictionary = 1u << 10u
};

ColumnRef createIndexColumn(IndexType type) {
ColumnRef createIndexColumn(uint64_t type) {
// Switch on the raw wire value: converting an out-of-range value to IndexType is undefined.
switch (type) {
case IndexType::UInt8:
return std::make_shared<ColumnUInt8>();
Expand All @@ -58,7 +59,33 @@ ColumnRef createIndexColumn(IndexType type) {
return std::make_shared<ColumnUInt64>();
}

throw ValidationError("Invalid LowCardinality index type value: " + std::to_string(static_cast<uint64_t>(type)));
throw ValidationError("Invalid LowCardinality index type value: " + std::to_string(type));
}

template <typename IndexColumnType>
void validateIndexes(Column& index_column, size_t dictionary_size) {
const auto& indexes = static_cast<IndexColumnType&>(index_column).GetWritableData();
for (size_t i = 0, rows = indexes.size(); i < rows; ++i) {
if (indexes[i] >= dictionary_size) {
throw ProtocolError("LowCardinality index " + std::to_string(indexes[i]) + " at row " + std::to_string(i) +
" is out of range for a dictionary of " + std::to_string(dictionary_size) + " items");
}
}
}

void validateIndexes(Column& index_column, size_t dictionary_size) {
switch (index_column.Type()->GetCode()) {
case Type::UInt8:
return validateIndexes<ColumnUInt8>(index_column, dictionary_size);
case Type::UInt16:
return validateIndexes<ColumnUInt16>(index_column, dictionary_size);
case Type::UInt32:
return validateIndexes<ColumnUInt32>(index_column, dictionary_size);
case Type::UInt64:
return validateIndexes<ColumnUInt64>(index_column, dictionary_size);
default:
throw ValidationError("Invalid index column type for LowCardinality column:" + index_column.Type()->GetName());
}
}

IndexType indexTypeFromIndexColumn(const Column & index_column) {
Expand Down Expand Up @@ -426,7 +453,7 @@ auto Load(ColumnRef new_dictionary_column, InputStream& input, size_t rows) {
if (!WireFormat::ReadFixed(input, &index_serialization_type))
throw ProtocolError("Failed to read index serializaton type.");

auto new_index_column = createIndexColumn(static_cast<IndexType>(index_serialization_type & IndexTypeMask));
auto new_index_column = createIndexColumn(index_serialization_type & IndexTypeMask);
if (index_serialization_type & IndexFlag::NeedGlobalDictionaryBit)
throw UnimplementedError("Global dictionary is not supported.");

Expand All @@ -452,7 +479,12 @@ auto Load(ColumnRef new_dictionary_column, InputStream& input, size_t rows) {
if (number_of_rows != rows)
throw AssertionError("LowCardinality column must be read in full.");

new_index_column->LoadBody(&input, number_of_rows);
if (!new_index_column->LoadBody(&input, number_of_rows)) {
throw ProtocolError("Failed to read values of index column.");
}

// GetItem() dereferences the dictionary with these indexes unchecked.
validateIndexes(*new_index_column, dataColumn->Size());

if (auto nullable = new_dictionary_column->As<ColumnNullable>()) {
nullable->Append(true);
Expand Down
76 changes: 76 additions & 0 deletions ut/columns_ut.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1360,6 +1360,82 @@ TEST(ColumnsCase, ColumnLowCardinalityString_Load) {
}
}

namespace {
// Native LowCardinality body: key version, index type (0..3 for UInt8..UInt64) with
// HasAdditionalKeys, dictionary keys, row count and the indexes at that width.
std::string MakeLowCardinalityBody(uint64_t index_type, uint64_t keys, const std::string& dictionary,
const std::vector<uint64_t>& indexes) {
std::string out;
auto put = [&out](uint64_t v, size_t width) { out.append(reinterpret_cast<const char*>(&v), width); };
put(1, 8);
put(index_type | (1u << 9), 8);
put(keys, 8);
out += dictionary;
put(indexes.size(), 8);
for (auto index : indexes) {
put(index, size_t{1} << index_type);
}
return out;
}
} // namespace

TEST(ColumnsCase, ColumnLowCardinalityFixedString_LoadRejectsIndexOutOfRange) {
// 1001 * index wraps modulo 2^64 to byte 1500 of the 2002-byte dictionary.
const uint64_t wrapping_index = 12107403452974201212ull;
const std::string dictionary = std::string(1001, 'A') + std::string(1001, 'B');

for (const auto& [index_type, index] : std::vector<std::pair<uint64_t, uint64_t>>{{3, 2}, {3, wrapping_index}, {0, 2}}) {
ColumnLowCardinalityT<ColumnFixedString> col(1001);
const auto body = MakeLowCardinalityBody(index_type, 2, dictionary, {1, index});
ArrayInput input(body.data(), body.size());
EXPECT_FALSE(col.Load(&input, 2)) << "index type " << index_type << " index " << index;
}

for (uint64_t index_type : {0, 3}) {
ColumnLowCardinalityT<ColumnFixedString> col(1001);
const auto body = MakeLowCardinalityBody(index_type, 2, dictionary, {1, 0});
ArrayInput input(body.data(), body.size());
ASSERT_TRUE(col.Load(&input, 2)) << "index type " << index_type;
EXPECT_EQ(col.At(0), std::string(1001, 'B'));
EXPECT_EQ(col.At(1), std::string(1001, 'A'));
}
}

TEST(ColumnsCase, ColumnLowCardinalityString_LoadRejectsIndexOutOfRange) {
const std::string dictionary = std::string{0, 3} + "foo";
auto make_nullable = [] {
return ColumnLowCardinality(std::make_shared<ColumnNullable>(std::make_shared<ColumnString>(), std::make_shared<ColumnUInt8>()));
};
{
ColumnLowCardinalityT<ColumnString> col;
const auto body = MakeLowCardinalityBody(3, 2, dictionary, {1, 2});
ArrayInput input(body.data(), body.size());
EXPECT_FALSE(col.Load(&input, 2));
}
{
auto col = make_nullable();
const auto body = MakeLowCardinalityBody(3, 2, dictionary, {0, 2});
ArrayInput input(body.data(), body.size());
EXPECT_FALSE(col.Load(&input, 2));
}
{
ColumnLowCardinalityT<ColumnString> col;
const auto body = MakeLowCardinalityBody(3, 2, dictionary, {1, 0});
ArrayInput input(body.data(), body.size());
ASSERT_TRUE(col.Load(&input, 2));
EXPECT_EQ(col.At(0), "foo");
EXPECT_EQ(col.At(1), "");
}

// Index 0 of a Nullable dictionary is the null item.
auto col = make_nullable();
const auto body = MakeLowCardinalityBody(0, 2, dictionary, {0, 1});
ArrayInput input(body.data(), body.size());
ASSERT_TRUE(col.Load(&input, 2));
EXPECT_EQ(col.GetItem(0).type, Type::Void);
EXPECT_EQ(col.GetItem(1).AsBinaryData(), "foo");
}

// This is temporary disabled since we are not 100% compatitable with ClickHouse
// on how we serailize LC columns, but we check interoperability in other tests (see client_ut.cpp)
TEST(ColumnsCase, DISABLED_ColumnLowCardinalityString_Save) {
Expand Down
Loading