[backfill: ClickHouse/clickhouse-cpp] Named-parameter string values mis-escape backslash/tab/newline (silent doubling)

[alex-clickhouse]

Target client repo: ClickHouse/clickhouse-cpp

Severity: sev:3 — silent data corruption on a default code path. Any string value containing \, \t, or \n passed via Query::SetParam is encoded with an extra backslash; the server accepts the literal and stores a corrupted value with no error surfaced.

Description

clickhouse/base/wire_format.cpp:112 (WireFormat::WriteQuotedString) is used to encode named-parameter values for the native protocol. In the escape path it unconditionally writes a single \ (line 139) before the per-character branch, and the per-character branches for \\, \t, \n each already begin with \\ themselves. The result is an extra backslash for those three characters:

• input \\ (one backslash) is emitted as '\\\\\\\\' (four backslashes between quotes); the server SQL-literal parser turns \\\\ into \\\\ → two backslashes
• input \\t (tab) is emitted as '\\\\\\\\t' (backslash-backslash-backslash-t between quotes); the server parses to \\<tab>
• input \\n (newline) — same pattern, parses to \\<newline>

The \\0, \\b, \\' branches use xNN hex escapes (x00, x08, x27) and combine with the stray \\ into a valid \\xNN, so those three values round-trip correctly — which is why the existing TEST_P(ClientCase, QueryParameters) (ut/client_ut.cpp:1856) doesn't catch it (and it never SELECTs the inserted string back to compare).

The bug is the C++ analogue of clickhouse-go #1576. In Go the same root cause produced a visible server parse error (Cannot parse quoted string: expected closing quote); in C++ the encoding happens to remain parseable, so the value is silently corrupted instead.

ClickHouse server version

Verified server-side parsing against 26.4.2.10 (local instance: SELECT length('\\\\\\\\'), hex('\\\\\\\\') → 2 / 5C5C, confirming a string of two backslashes is the parsed value of the 4-backslash literal the client emits for input \\). Client wire-output verified by code analysis of the actual WriteQuotedString source and a standalone replica of its logic; the C++ library itself was not built and the round-trip was not executed end-to-end in this run.

Reproduction

Minimal test case using the project's own Query::SetParam API and the QueryParameters test fixture style:

TEST_P(ClientCase, QueryParametersBackslash) {
    const auto& server_info = client_->GetServerInfo();
    if (versionNumber(server_info) < versionNumber(24, 7)) GTEST_SKIP();
    const std::string table = "test_cpp_param_backslash";
    client_->Execute("CREATE TEMPORARY TABLE IF NOT EXISTS " + table +
                     " (id UInt64, name String)");

    Query ins("INSERT INTO " + table + " VALUES ({id:UInt64}, {name:String})");
    ins.SetParam("id", "1").SetParam("name", "\\");        // a single backslash
    client_->Execute(ins);

    std::string seen;
    Query sel("SELECT name FROM " + table + " WHERE id = 1");
    sel.OnData([&](const Block& b) {
        if (b.GetRowCount()) seen = std::string(b[0]->As<ColumnString>()->At(0));
    });
    client_->Select(sel);

    // Expected: "\\" (length 1, a single backslash)
    // Observed: "\\\\" (length 2, two backslashes) — silent corruption
    EXPECT_EQ(seen, std::string("\\"));
}

Replace the param value with "\\t" or "\\n" for the tab/newline variants — both will produce \\<tab> and \\<newline> respectively instead of the original byte. "a'b" and "a\\0b" round-trip correctly (those branches use \\xNN hex escapes).

Suggested fix

In WireFormat::WriteQuotedString (clickhouse/base/wire_format.cpp:112), the leading WriteAll(output, \"\\\\\", 1) on line 139 should be removed; each per-character case should be responsible for emitting its full escape sequence (e.g. \\\\, \\t, \\n, \\x27). The length accounting (size + 2 + 3 * quoted_count) should be revisited accordingly — the current 3 * quoted_count factor encodes the assumption that each special char expands to 4 wire bytes, which matches the current (buggy) output but not the correct 2-byte expansion for \\\\/\\t/\\n or the 4-byte expansion for \\xNN cases.

The existing QueryParameters test should also be extended to SELECT the inserted strings back and EXPECT_EQ them against the originals — without that, similar regressions will continue to slip through.

Dedup search

• gh issue list --repo ClickHouse/clickhouse-cpp --search 'backslash parameter' --state all --limit 20 → 0 results
• gh issue list --repo ClickHouse/clickhouse-cpp --search 'WriteQuotedString' --state all --limit 20 → 0 results
• gh issue list --repo ClickHouse/clickhouse-cpp --search 'escape parameter' --state all --limit 20 → 1 unrelated (issue Execute throws exception, but command line client accepts same query. #13, 2019, command-line vs Execute)
• gh pr list --repo ClickHouse/clickhouse-cpp --search 'backslash parameter' --state all --limit 20 → 0 results
• gh issue list --repo ClickHouse/integrations-ai-playground --label backfill --label target:clickhouse-cpp --search 'backslash' --state all --limit 20 → 0 results
• gh pr list --repo ClickHouse/integrations-ai-playground --search 'backslash cpp' --state all --limit 20 → 0 results

Source bug

Filed in response to ClickHouse/clickhouse-go#1576 (Go client analogue — same root cause, different manifestation).

[polyglotAI-bot]

TL;DR: I was unable to reproduce data corruption on current master (commit 737145d) against ClickHouse 26.5.1. String parameter values containing \, \t, \n (and ', \0, \b) round-trip correctly through Query::SetParam. The extra backslash in WriteQuotedString is not a stray bug — it compensates for the fact that clickhouse-cpp transmits query parameters as custom settings, which the server decodes twice. Applying the suggested fix (single-escaping) actually breaks parameter binding with a server BAD_QUERY_PARAMETER error, so it would be a regression. Details below.

Reproduction attempt

I extended the existing QueryParameters test in ut/client_ut.cpp to insert a value via SetParam and read it back, asserting byte-for-byte equality, for: a single \, a \t, a \n, a mixed a\b<TAB>c<LF>d, two backslashes, a ', an embedded \0, a \b, and all of them together.

On unmodified master every case passes — the stored value equals the input exactly (verified with hex(name) and direct std::string comparison, so embedded NULs are covered).

What the client actually puts on the wire

Dumping the raw output of WireFormat::WriteQuotedString (first byte is the varint length prefix):

input "\t"  (tab)        -> 27 5C 5C 5C 74 27   =  ' \ \ \ t '
input "\\"  (backslash)  -> 27 5C 5C 5C 5C 27   =  ' \ \ \ \ '
input "\n"  (newline)    -> 27 5C 5C 5C 6E 27   =  ' \ \ \ n '
input "'"   (quote)      -> 27 5C 78 32 37 27   =  ' \ x 2 7 '
input "\0"  (nul)        -> 27 5C 78 30 30 27   =  ' \ x 0 0 '

So \t/\n/\\ are indeed emitted with a "doubled" backslash — but that is required given how the value is decoded on the server.

Why it round-trips: two decode passes

clickhouse-cpp sends query parameters as custom settings (client.cpp: WriteString(name), a Custom flag, then WriteQuotedString(value)), not through a dedicated parameters channel. The server then unescapes the value twice:

1. as a custom-setting value it is parsed as a quoted string literal (a Field) → first unescape;
2. during substitution, ReplaceQueryParameterVisitor::resolveParameterValueAsField runs serialization->deserializeTextEscaped(...) on the result → second unescape.

For a tab that chain is:

'\\\t'  --(quoted-literal parse)-->  \<TAB>  --(deserializeTextEscaped)-->  <TAB>

Two passes consume two backslashes. A single backslash per character (as the report proposes) only survives one pass.

The suggested fix regresses

I applied the suggested change (drop the leading WriteAll(output, "\\", 1), single-escape each case, with matching length accounting). QueryParameters then fails:

C++ exception with description "DB::Exception: Value A" thrown in the test body.   (BAD_QUERY_PARAMETER)

After only one unescape pass the value still contains a raw control byte, so the second pass (deserializeTextEscaped) rejects it — the same failure you get from handing the server a raw, unescaped tab:

Code: 457. DB::Exception: Value a<TAB>b cannot be parsed as String for query parameter 'v'
because it isn't parsed completely: only 1 of 3 bytes was parsed: a. (BAD_QUERY_PARAMETER)

Conclusion

On current master + ClickHouse 26.5.1 this appears to be working as intended. The report's analysis assumes a single server-side unescape, but this client's parameters are decoded twice (custom-setting literal parse, then deserializeTextEscaped), which is exactly what the double-escaping accounts for.

If there is a concrete reproduction I'm missing — a specific server version, or a value that genuinely round-trips wrong — please share the exact steps plus SELECT version() and I'll investigate again. Otherwise I believe this can be closed as not-a-bug.

Investigated by @polyglotAI-bot.