ClickHouse / ClickHouse/clickhouse-cpp

ColumnArray::AppendAsColumn silently accepts a wrong-typed element column, writes zero data bytes and desynchronizes the native block stream

Offen
#543 0 Kommentare 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen
Vorherrschende Sprache
C
Sterne
382
Forks
209
Ø Merge
2 T. 19 Std.
Gemergte PRs (30 T.)
14

Beschreibung

## Description

`ColumnArray::AppendAsColumn()` (`clickhouse/columns/array.cpp:49`) relies on `data_->Append()` throwing when the supplied column type does not match the array's element type:

```cpp
void ColumnArray::AppendAsColumn(ColumnRef array) {
// appending data may throw (i.e. due to ype check failure), so do it first to avoid partly modified state.
data_->Append(array);
AddOffset(array->Size());
}
```

But most `Column::Append(ColumnRef)` implementations **silently no-op** on a type mismatch instead of throwing:

* `ColumnVector::Append` — `clickhouse/columns/numeric.cpp:72-76`: `if (auto col = column->As>()) { ... }`, no `else`.
* `ColumnString::Append` — `clickhouse/columns/string.cpp:248-260`: same shape.
* `ColumnFixedString::Append` — `clickhouse/columns/string.cpp:73-79`: same shape, and additionally silent when `string_size_` differs.

So passing a wrong-typed column to `AppendAsColumn` appends **nothing** to `data_` while `AddOffset(array->Size())` still advances the offsets by `array->Size()`. The `ColumnArray` is left internally inconsistent — exactly the state its own constructor rejects with `ValidationError("Mismatch between data and offsets: ...")` — and `SaveBody()` then writes offsets promising N elements followed by zero element bytes.

On the wire this **desynchronizes the native-protocol block**: the server reads the following column's bytes as this array's element data. The user gets no client-side error, only a confusing (and misleading) server-side exception, and the connection is left unusable for the next operation.

This is the C++ analogue of ClickHouse/clickhouse-java#3041, where `SerializerUtils.serializeArrayData` silently writes zero bytes for a non-`null`, non-array/non-`List` value.

Note the same failure class was reported for `ColumnNullable` in PR #376 (closed, unmerged): `nulls_` grows even when the nested `Append()` silently fails. `ColumnArray` has the identical problem with `offsets_`.

## ClickHouse server version

`26.7.2.59` (native protocol, port 9000), verified against a live server.
Repo at commit `737145d`.

## Reproduction

Added to `ut/column_array_ut.cpp` (needs `#include ` for the second test):

```cpp
TEST(ArrayDesync, WrongTypedAppendAsColumn) {
// Array(String), but we append a UInt64 column as one row's elements
auto arr = std::make_shared(std::make_shared());
auto wrong = std::make_shared();
wrong->Append(1); wrong->Append(2); wrong->Append(3);

EXPECT_NO_THROW(arr->AppendAsColumn(wrong)); // currently passes -- the defect

std::cerr << "arr->Size()=" << arr->Size()
<< " claimed elems row0=" << arr->GetSize(0)
<< " actual data size=" << arr->GetAsColumn(0)->Size() << std::endl;

Buffer buf;
BufferOutput out(&buf);
arr->SaveBody(&out);
out.Flush();
std::cerr << "SaveBody bytes = " << buf.size() << std::endl;
}

TEST(ArrayDesync, EndToEndInsert) {
clickhouse::Client client(clickhouse::ClientOptions().SetHost("localhost").SetPort(9000));
client.Execute("DROP TABLE IF EXISTS test_arr_desync");
client.Execute("CREATE TABLE test_arr_desync (id UInt32, val Array(String), tail String) ENGINE = Memory");

clickhouse::Block b;
auto id = std::make_shared(); id->Append(1);

auto val = std::make_shared(std::make_shared());
auto bad = std::make_shared(); bad->Append(7); bad->Append(8);
val->AppendAsColumn(bad); // wrong element type, silently dropped

auto tail = std::make_shared(); tail->Append("TAILVALUE");
b.AppendColumn("id", id);
b.AppendColumn("val", val);
b.AppendColumn("tail", tail);

try { client.Insert("test_arr_desync", b); std::cerr << "INSERT SUCCEEDED" << std::endl; }
catch (const std::exception& e) { std::cerr << "INSERT threw: " << e.what() << std::endl; }

try {
client.Select("SELECT id, val, tail FROM test_arr_desync", [](const clickhouse::Block&) {});
} catch (const std::exception& e) { std::cerr << "SELECT threw: " << e.what() << std::endl; }
}
```

### Actual output

```
[ RUN ] ArrayDesync.WrongTypedAppendAsColumn
arr->Size()=1 claimed elems row0=3 actual data size=0
SaveBody bytes = 8
[ OK ] ArrayDesync.WrongTypedAppendAsColumn

[ RUN ] ArrayDesync.EndToEndInsert
INSERT threw: DB::Exception: Unknown data type family: TAILVALUE
SELECT threw: cannot execute query while executing another operation
[ OK ] ArrayDesync.EndToEndInsert
```

Reading that: `SaveBody` emitted only the 8-byte offset (`3`) and **zero** element bytes. End-to-end, the server consumed the `tail` column's string payload as the *type name* of the next column — hence `Unknown data type family: TAILVALUE`. The connection was then left mid-operation, so the follow-up `SELECT` also failed.

### Expected

`AppendAsColumn` (or the underlying `Append`) should reject a column whose type does not match the array's element type with a clear client-side `ValidationError` / `std::runtime_error` naming both types, leaving the `ColumnArray` unmodified. Appending a correctly-typed column must keep working, and `AppendAsColumn` of an empty correctly-typed column must still add a zero-length row.

## Suggested fix

Two possible layers, not mutually exclusive:

1. **Narrow** — in `ColumnArray::AppendAsColumn` (`clickhouse/columns/array.cpp:49`), validate before mutating:
```cpp
void ColumnArray::AppendAsColumn(ColumnRef array) {
if (!data_->Type()->IsEqual(array->Type()))
throw ValidationError("Cannot append column of type " + array->Type()->GetName()
+ " to Array of " + data_->Type()->GetName());
data_->Append(array);
AddOffset(array->Size());
}
```
This covers the array path only, but it is where the offsets/data desync is introduced.

2. **General** — make the `Append(ColumnRef)` implementations throw on mismatch instead of silently returning (`numeric.cpp:72`, `string.cpp:73`, `string.cpp:248`, and any sibling with the same `if (auto col = column->As<...>())`-with-no-`else` shape). That would also fix the `ColumnNullable` variant from PR #376 and make the existing comment in `AppendAsColumn` true. It is a behavior change for callers currently relying on the silent no-op, so it may warrant its own discussion.

Regression tests: a wrong-typed `AppendAsColumn` throws and leaves `Size()`/`GetOffset()` unchanged, plus contrast cases that a correctly-typed column and an empty correctly-typed column still behave as today.

## Link

Relayed from https://github.com/ClickHouse/clickhouse-java/issues/3041

Beitragsleitfaden

Für dieses Repository ist kein Beitragsleitfaden indexiert

Rechercherichtung

Beginne mit clickhouse/columns/array.cpp:49 sowie den Append-Implementierungen in numeric.cpp:72-76 und string.cpp:73-79,248-260. Führe die fokussierten Tests in ut/column_array_ut.cpp aus und untersuche das bestehende Verhalten der Array-Validierung. Die Aufgabe ist abgeschlossen, wenn ein falsch typisiertes AppendAsColumn abgelehnt wird, ohne Offsets oder Daten zu ändern, während korrekt typisierte und leere Spalten ihr aktuelles Verhalten beibehalten.

Vom Indexierungsmodell aus dem Issue-Text verfasst.

Bewertung

Tech-Stack
cpp
Bereich
backend-api-design, testing
Issue-Typ
Bug
Schwierigkeit
3/5
Geschätzter Aufwand
1-2 Tage
Aktivitätsstatus
Ruhig
Klarheit
Klar beschrieben
Anfängerfreundlichkeit
75/100

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.