ClickHouse / ClickHouse/clickhouse-cpp

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

Abierto
#543 0 comentarios 0 reacciones 0 asignados Ver en GitHub
Lenguaje dominante
C
Estrellas
382
Forks
209
Merge medio
2 d 19 h
PR fusionados (30 d)
14

Descripción

## 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

Guía de contribución

No hay ninguna guía de contribución indexada para este repositorio

Línea de trabajo

Comienza con clickhouse/columns/array.cpp:49 y las implementaciones de Append en numeric.cpp:72-76 y string.cpp:73-79,248-260. Ejecuta las pruebas específicas de ut/column_array_ut.cpp e inspecciona el comportamiento existente de validación de arrays. La tarea estará terminada cuando se rechace un AppendAsColumn con un tipo incorrecto sin cambiar los offsets ni los datos, mientras que las columnas con el tipo correcto y las columnas vacías conserven su comportamiento actual.

Escrito por el modelo de indexación a partir del texto del issue.

Evaluación

Stack tecnológico
cpp
Área
backend-api-design, testing
Tipo de issue
Error
Dificultad
3/5
Tiempo estimado
1-2 días
Estado de actividad
Tranquilo
Claridad
Bien especificado
Aptitud para principiantes
75/100

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.