Reject a {database}/{table} substitution that is not a safe ZooKeeper path component - #114006
Conversation
Sorry, something went wrong.
… path component
TableZnodeInfo::resolve substitutes {database} and {table} into a ReplicatedMergeTree
ZooKeeper path without escaping or validating them. A table or database whose name
contains '/' therefore places its own znodes inside another table's keeper subtree:
with a {database}/{table}-bearing path template (shipped commented out in
programs/server/config.xml and recommended by the docs) or an explicit path,
CREATE TABLE inj.`victim/replicas/ghost` (c0 Int) ENGINE = ReplicatedMergeTree ORDER BY c0
registers a permanently inactive replica named `ghost` under inj.victim, which then
makes ALTER ... SETTINGS alter_sync = 2 fail with KEEPER_EXCEPTION while still applying
the metadata change, and makes OPTIMIZE ... FINAL hang. The damage survives a plain DROP
of the offending table and a restart; repairing it needs SYSTEM DROP REPLICA.
Two more classes reach a znode name the same way: '.' / '..' as a whole path component,
and control bytes. Both are rejected by the ZooKeeper data model, so the same DDL is
accepted by ClickHouse Keeper and refused by Apache ZooKeeper.
Reject rather than escape, since escaping would change the resulting znode name and so
silently repoint an existing table. Three checks are needed because the classes differ
in kind:
- '/' is checked on the substituted value. It cannot be seen on the assembled path,
where a substituted '/' is indistinguishable from a template separator.
- '.', '..' and control bytes are checked on the fully expanded string, gated on
whether {database}/{table} reached it in either pass. Checking the raw value would
wrongly reject an embedded substitution such as `table_{table}` for a table named
'.', which yields the legal component `table_.`.
- '{' and '}' are checked on the substituted value, because a name carrying macro
syntax survives the other two checks and is then expanded again by the second pass.
A lone '}' is rejected too: it can close a brace opened by a configured macro.
Both macro passes are validated. The second pass expands a configured macro whose value
contains {database}/{table}, which is a real configuration - the test suite's own
macros.xml ships default_path_test = /clickhouse/tables/{database}/{shard}/.
Validation is requested explicitly by the storage factory rather than derived from
`mode`, because extractZooKeeperPathFromReplicatedTableDef re-derives the path of an
existing table with a hardcoded LoadingStrictnessLevel::CREATE and swallows BAD_ARGUMENTS
into nullopt; validating there would silently drop a pre-existing table's replicated data
path from a backup. Fresh definitions (CREATE, DDL replay in a Replicated database, and a
full-definition ATTACH) are validated; short ATTACH, server startup and RESTORE are not,
so tables that exist today keep loading and restoring.
Each expanded output now gets its own MacroExpansionInfo. Macros::expand returns early
for a string without '{' without clearing the flags, so a shared one made expanded_table
sticky and would have rejected an explicitly written replica name because the path
substituted.
An explicitly supplied overlapping or nested zookeeper_path is not addressed here; that
is the same family as issue ClickHouse#22970. RESTORE ... AS <name> renames to a user-chosen
destination before the storage factory runs and is likewise not addressed.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Apache ZooKeeper's PathUtils.validatePath rejects U+007F-U+009F, but the
component check only tested single bytes for < 0x20 or == 0x7F, so a name
carrying a C1 control such as U+0085 (UTF-8 C2 85) still reached a znode
name. ClickHouse Keeper validates nothing, so the divergence between the
two coordination backends survived for that range.
The C1 range is detected as its 2-byte UTF-8 encoding C2 80..C2 9F, which
is exact because UTF-8 is a prefix code. Full parity with ZooKeeper is not
implemented on purpose: its rejected set also covers the UTF-16 surrogate
range, and a byte-level widening to 0x80-0x9F would reject the ordinary
continuation bytes of every non-ASCII name. Both shapes reject a name like
таблица_🚀_表, which works today; a test row and a mutant pin that.
A failure on the replica name now advises specifying the replica name
rather than the ZooKeeper path, since those are separate engine arguments.
Four branches had no automated coverage and were each independently
deletable with the suite still green: the {database} branch, a closing
brace with no opening one, the control-character loop, and the two ATTACH
arms of the opt-in guard. Rows needing a per-run unique database name or
UUID go in a .sh companion, because a fixed literal UUID makes parallel
copies collide and an Atomic database rejects a full-definition ATTACH
without one.
Five branches of the substitution validation were independently deletable with
the test suite still green, so nothing pinned them:
* the short-ATTACH exemption. A table created with a direct {database}/{table}
stores fully literal text, so on re-ATTACH there is nothing left to
substitute and the checks are skipped regardless of the flag. Removing the
exemption therefore left the suite green, even though that exemption is what
keeps existing path-unsafe tables loading at startup. Covered by a table
whose path comes from a configured macro, so the {database} does survive
into metadata, over a database later renamed to a path-unsafe name.
* the second-pass provenance record. Every existing row reached the output
through a direct macro in the first pass, so the flag never depended on the
second. Covered by a row where both macros are configured ones.
* the replica-name component check. Its only row carried a '/', so the check
on the substituted value rejected it first. Covered by a table named '..'
substituted into the replica name with an otherwise safe path.
* both ends of the C1 range. Only U+0085 in the interior was covered.
Each row was verified by observing the rejection rather than by reasoning about
the escape, and each new branch has a mutant that reddens its row and leaves the
others green.
Both tests also gain no-shared-merge-tree: under --replace-replicated-with-shared
the runner strips the path and replica-name arguments, which deletes the
substitution these tests exist to assert.
…s own metadata
A Replicated database re-derives a table by parsing the CREATE statement it stored in Keeper and
executing it. That statement can still carry an unexpanded macro: full_path_for_metadata is taken
after the first expansion pass, which unfolds only a direct {database}/{table}, so a CONFIGURED
macro whose value contains {database} survives verbatim. On replay the macro expands, the
substitution is recorded, and the new path checks run on a table that already exists.
The replay arrives BELOW LoadingStrictnessLevel::ATTACH, because parseQueryFromMetadata sets
create.attach = false, so `mode` cannot tell it apart from a fresh CREATE, and a user CREATE inside
a Replicated database must stay validated. Carry the provenance on the query context instead and set
it at the two sites that replay stored definitions: DatabaseReplicated::recoverLostReplica and the
restore path in InterpreterSystemQuery.
Measured before the fix: a lost replica of a database renamed to a path-unsafe name never recovers.
The replaying replica loops, rejecting its own stored definition every five seconds (13 attempts
observed, 0 tables recovered), where the pre-fix binary recovers on the first attempt.
Also cover four branches that had no discriminating test row: the second-pass replica-name
provenance, the replica_info term of the rename restriction, the expanded_database half of the
provenance accumulation, and the high end of the C0 control range. Test renamed 04827 -> 04832,
since 04827 was taken upstream in the meantime.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…pplied Two routes re-derive an existing table's keeper path and were still being judged as if the user had just supplied it. SYSTEM RESTART REPLICA re-attaches from stored metadata: doRestartReplica reads the definition back with getCreateTableQuery, whose attach flag DatabaseOnDisk::getCreateQueryFromMetadata clears, then sets only create.attach and calls the factory at ATTACH. That satisfies the full-definition-ATTACH arm of the guard, and full_path_for_metadata is captured after the first macro pass, so a configured macro survives into the stored text and re-substitutes. The statement then failed and, after ten retries, left the table permanently detached. Measured on a data directory written by a pre-fix server: one of six path-unsafe tables disappeared from system.tables. The re-attach now gets its own context copy carrying the recovery provenance, so the shared system context, the other branches of the same statement and the parallel tasks of SYSTEM RESTART REPLICAS are unaffected. SECONDARY_CREATE is by definition a replay of a definition another node already committed, its only producers being Replicated-database catch-up and RESTORE, so it is exempted too. Validating it prevented nothing measurable: on a lagging upgraded replica the rejected entry was retried, the replica declared itself lost, recovered from Keeper and created the offending table anyway. Measured, 31 rejections and one lost-replica recovery became zero and zero, with the same final table. A fresh unsafe CREATE is still refused, on the initiator of a Replicated database, in a plain database and through ON CLUSTER. A regression row covers the restart route, asserting that the table is still attached rather than that the statement returned successfully; a rejection leaves it detached, so the status alone can be satisfied by the wrong outcome.
doRestartReplica detaches the table, then re-creates it inside a block that retries on every exception and, if all retries fail, adjusts the in-memory metadata digest so DatabaseReplicated does not later assert a mismatch. An exception thrown after the detach but before that block reaches neither the retries nor the digest adjustment, and leaves the table permanently detached. The context copy carrying the recovery provenance was taken in exactly that window, and copying a context allocates: a Settings, a QueryAccessInfo and the table function result map. Take it before anything is detached instead, where an allocation failure is harmless. Pure relocation of two statements. The context is only read afterwards, it is still the local context argument of the factory call, and nothing between the old and the new position mutates the context being copied, so the copy's contents cannot depend on where it is taken. Widening its lifetime across the detach costs nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Internal second-model review (final round): 1 finding, disagreed with evidence
An independent model reviewed this change against the repository review skill; a separate reviewer ❌ doRestartReplica: the context copy can still throw after the table is shut down - DISAGREED. The claim is that taking the copy after flushAndShutdown creates an unhandled window in which a The preceding round's finding, which named the post-detach window, was agreed and fixed: the copy ⚠️ Confirmations re-derived independently rather than accepted (no action):
Earlier rounds of this review are recorded in the preceding comments. |
Sorry, something went wrong.
Pre-PR validation gate
50 randomized runs of both new tests and of 01148_zookeeper_path_macros_unfolding are clean, as are 8 copies of the new tests run concurrently. |
Sorry, something went wrong.
|
cc @PedroTadim @antonio2368, could you review this? You were both asked for on this one. A {database}/{table} value is spliced into a ReplicatedMergeTree keeper path unescaped and never validated, so a table name containing / lands at another table subtree replicas/<name> znode and registers a phantom replica there, which permanently breaks ALTER and OPTIMIZE on the victim. The substitution is now rejected when it would not be a single safe path component; every route that merely re-derives an existing path stays exempt, so existing tables keep loading. |
Sorry, something went wrong.
|
Workflow [PR], commit [22bea90] Summary: ✅
AI ReviewSummaryThis PR adds the right validation logic for unsafe {database} / {table} substitutions in ReplicatedMergeTree keeper paths and correctly exempts several true metadata-replay flows. The remaining problem is that the new gate still treats two fresh-definition paths as replays: renamed RESTORE and ATTACH TABLE ... AS REPLICATED. Both can still synthesize a new replicated definition from user-chosen names without validating the resulting keeper path, so the original path-injection bug is not fully closed. Findings❌ Blockers
Final Verdict
LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 142/147 (96.60%) · Uncovered code |
Sorry, something went wrong.
CI finish ledger - 2ab29adEvery failure below has an owner: a fixing PR (ours or external), or a full-effort fix task
No other test-level failure on this head (0 running, Finish Workflow success). Session id: cron:our-pr-ci-monitor:20260809-230000 |
Sorry, something went wrong.
|
@antonio2368 I am not sure if this is the best fix. As an expert in this area, you or someone in your team should review this 🙏 |
Sorry, something went wrong.
| /// Set for the CREATE queries a Replicated database replays from a definition it already stored, | ||
| /// to re-derive tables that exist. Such a definition describes the state of the world rather than | ||
| /// a change to it, so validation that may reject a freshly supplied definition must not run. |
There was a problem hiding this comment.
Make the comment shorter
Sorry, something went wrong.
There was a problem hiding this comment.
Shortened to two lines in 22bea904362ea6b5b084943b853f46361435cec2.
Sorry, something went wrong.
| { | ||
|
|
||
| /// The path and the replica name are separate engine arguments, so the remedy names the one that failed. | ||
| std::string_view howToOverride(std::string_view what) |
There was a problem hiding this comment.
It seems safer to accept enum with a function constexpr std::string_view toString(enum...)
Sorry, something went wrong.
There was a problem hiding this comment.
Done in 22bea904362ea6b5b084943b853f46361435cec2: enum class ZnodeString { Path, ReplicaName } with constexpr std::string_view toString(ZnodeString), and howToOverride now takes the enum too, so neither compares against a string literal.
Sorry, something went wrong.
| std::string_view bad; | ||
| if (value.contains('/')) | ||
| bad = "'/'"; | ||
| else if (value.contains('{') || value.contains('}')) | ||
| bad = "'{' or '}'"; | ||
| if (bad.empty()) | ||
| return; |
There was a problem hiding this comment.
You can do find_first_of on the value and if it's npos return otherwise you print it in the error message.
Sorry, something went wrong.
There was a problem hiding this comment.
Done in 22bea904362ea6b5b084943b853f46361435cec2: one find_first_of("/{}"), and the offending character itself goes into the message instead of the hardcoded per-case description.
Sorry, something went wrong.
| { | ||
| const auto byte = static_cast<unsigned char>(component[i]); | ||
| const auto next_byte = i + 1 < component.size() ? static_cast<unsigned char>(component[i + 1]) : 0; | ||
| Int32 code_point = -1; |
There was a problem hiding this comment.
Use std::optional<UInt16>
Sorry, something went wrong.
There was a problem hiding this comment.
Done in 22bea904362ea6b5b084943b853f46361435cec2: std::optional<UInt16>, so the -1 sentinel and the cast at the throw both go away.
Sorry, something went wrong.
| for (size_t pos = 0, next = 0; next != String::npos; pos = next + 1) | ||
| { | ||
| next = str.find('/', pos); | ||
| std::string_view component(str.data() + pos, (next == String::npos ? str.size() : next) - pos); | ||
|
|
||
| if (component == "." || component == "..") | ||
| throw Exception( | ||
| ErrorCodes::BAD_ARGUMENTS, | ||
| "The {} of a replicated table expands to {}, which has '{}' as a ZooKeeper path component. " | ||
| "Rename the table or the database, or {}", | ||
| what, quoteString(str), component, howToOverride(what)); | ||
|
|
||
| for (size_t i = 0; i < component.size(); ++i) | ||
| { | ||
| const auto byte = static_cast<unsigned char>(component[i]); | ||
| const auto next_byte = i + 1 < component.size() ? static_cast<unsigned char>(component[i + 1]) : 0; | ||
| Int32 code_point = -1; | ||
| if (byte < 0x20 || byte == 0x7F) | ||
| code_point = byte; | ||
| /// UTF-8 encodes U+0080-U+009F only as C2 80..C2 9F, so the pair test cannot match a | ||
| /// continuation byte of some other character. | ||
| else if (byte == 0xC2 && next_byte >= 0x80 && next_byte <= 0x9F) | ||
| code_point = next_byte; | ||
| if (code_point < 0) | ||
| continue; | ||
|
|
||
| throw Exception( | ||
| ErrorCodes::BAD_ARGUMENTS, | ||
| "The {} of a replicated table expands to a string containing the control character U+{}, " | ||
| "which is not valid in a ZooKeeper path. Rename the table or the database, or {}", | ||
| what, getHexUIntUppercase(static_cast<UInt16>(code_point)), howToOverride(what)); | ||
| } | ||
| } |
There was a problem hiding this comment.
std::string_view remaining = str;
while (true)
{
const size_t slash_pos = remaining.find('/');
const std::string_view component = remaining.substr(0, slash_pos);
if (component == "." || component == "..")
throw Exception(
ErrorCodes::BAD_ARGUMENTS,
"The {} of a replicated table expands to {}, which has '{}' as a ZooKeeper path component. "
"Rename the table or the database, or {}",
what, quoteString(str), component, howToOverride(what));
for (size_t i = 0; i < component.size(); ++i)
{
const auto byte = static_cast<unsigned char>(component[i]);
const auto next_byte = i + 1 < component.size() ? static_cast<unsigned char>(component[i + 1]) : 0;
Int32 code_point = -1;
if (byte < 0x20 || byte == 0x7F)
code_point = byte;
/// UTF-8 encodes U+0080-U+009F only as C2 80..C2 9F, so the pair test cannot match a
/// continuation byte of some other character.
else if (byte == 0xC2 && next_byte >= 0x80 && next_byte <= 0x9F)
code_point = next_byte;
if (code_point < 0)
continue;
throw Exception(
ErrorCodes::BAD_ARGUMENTS,
"The {} of a replicated table expands to a string containing the control character U+{}, "
"which is not valid in a ZooKeeper path. Rename the table or the database, or {}",
what, getHexUIntUppercase(static_cast<UInt16>(code_point)), howToOverride(what));
}
if (slash_pos == std::string_view::npos)
break;
remaining.remove_prefix(slash_pos + 1);
}the loop can be potentially more readable with using std::string_view manipulation
Sorry, something went wrong.
There was a problem hiding this comment.
Taken as written in 22bea904362ea6b5b084943b853f46361435cec2.
Since this is the validation loop, I checked it is equivalent rather than assuming. Against the previous shape, on a per-row prober that runs every row of both tests individually: 33/33 identical, including each error message. On top of that I ran a boundary probe for the cases this rewrite could plausibly change: '.' and '..' as the first, a middle and the last component; a control byte as the first, an interior and the last byte of a component (the next_byte == 0 path); a bare 0xC2 at the end of a component, and a 0xC2 immediately before a '/' whose next component starts with a control byte; both ends of C0 and C1 plus their neighbours U+0020, U+007E, U+00A0; a leading, a trailing and a lone '/'; and an empty trailing component from a template ending in '/'. All 21 as before.
As a control that the probe can actually fail, a mutant that skips the last component reddens 12 prober rows, 3 boundary rows and 04832.
Sorry, something went wrong.
|
|
||
| } | ||
|
|
||
| TableZnodeInfo TableZnodeInfo::resolve(const String & requested_path, const String & requested_replica_name, const StorageID & table_id, const ASTCreateQuery & query, LoadingStrictnessLevel mode, const ContextPtr & context, bool validate_substitutions) |
There was a problem hiding this comment.
format in multiple lines
Sorry, something went wrong.
There was a problem hiding this comment.
Wrapped in 22bea904362ea6b5b084943b853f46361435cec2, matching how the declaration is already split in the header.
Sorry, something went wrong.
…ing_view walk
Six readability and API-shape changes requested in review, no behaviour change:
* the "ZooKeeper path" / "replica name" subject of the messages becomes an enum
with constexpr toString and howToOverride functions, instead of a string_view
compared against a literal at the point of use
* checkSubstitutedValues finds the offending character with a single
find_first_of and reports that character, instead of two contains() calls and
a hardcoded description per case
* the component walk in checkPathComponents uses std::string_view find and
remove_prefix instead of index arithmetic over the whole string
* the control-character code point becomes std::optional<UInt16> instead of an
Int32 with -1 as the sentinel
* the resolve definition is wrapped to match the declaration
* the is_recovery_from_stored_metadata comment is shortened
Verified equivalent on the 33-row per-row prober for both tests plus a boundary
probe covering the first, middle and last component of the walk, control bytes at
the first, interior and last byte of a component, a bare C2 at the end of a
component, both ends of the C0 and C1 ranges and their neighbours, and each of
'/', '{' and '}' in a substituted value. A mutant that skips the last component
reddens 12 prober rows, 3 boundary rows and the test suite, so the probe
discriminates.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| /// replay can arrive at CREATE, so `mode` cannot tell it apart and the context carries it. | ||
| const bool validate_substitutions = (args.mode <= LoadingStrictnessLevel::CREATE | ||
| || (args.mode == LoadingStrictnessLevel::ATTACH && !args.query.attach_short_syntax)) | ||
| && !args.is_restore_from_backup |
There was a problem hiding this comment.
RESTORE is not always replaying an already-materialized keeper path. BackupMetadataFinder::findTableInBackupImpl renames the parsed CREATE to the restore target before execution (src/Backups/BackupMetadataFinder.cpp:277-289), so RESTORE TABLE ... AS ... and restores into renamed databases are fresh user-chosen definitions from the point of view of {database} / {table} expansion. This exemption also applies when the backup stored no explicit engine arguments, because adjustCreateQueryForBackup strips default ReplicatedMergeTree args (src/Backups/DDLAdjustingForBackupVisitor.cpp:63-69) and this function re-injects the current default_replica_path / default_replica_name (src/Storages/MergeTree/registerStorageMergeTree.cpp:320-337).
With the unconditional !args.is_restore_from_backup here, a renamed restore can still create a new replicated table whose unsafe {database} / {table} expansion is never checked, so the original keeper-path injection remains reachable through restore. This needs a narrower exemption than “all restore-from-backup”.
Sorry, something went wrong.
There was a problem hiding this comment.
Confirmed, and thanks for the precise citations: both re-derive at trunk and the consequence
reproduces. Measured on a built server with a low-privilege user (GRANT SELECT, INSERT, CREATE TABLE, BACKUP ON *.*, no SYSTEM RESTORE REPLICA): the restored table resolves inside the victim's
subtree, the victim goes total_replicas 1 -> 2 with an inactive phantom, ALTER ... alter_sync = 2
then fails Code: 999 KEEPER_EXCEPTION ... log_pointer ... doesn't exist, and a plain DROP of the
restored table leaves the phantom behind.
One correction to the reachability, because it decides the fix's shape. The strip at
DDLAdjustingForBackupVisitor.cpp:63-69 fires only when the stored path equals the configured
default verbatim, and resolve unfolds {database}/{table} at CREATE and stores the literal
(full_path_for_metadata). So under an unchanged {database}/{table} template the stored path is
never equal to the template, nothing is re-injected, and the unsafe RESTORE ... AS fails
Code: 253 REPLICA_ALREADY_EXISTS against the source path with the victim untouched. The route needs
a config migration: a backup taken while the default had no {database}/{table}, restored after
switching to the name-based template. The shipped {uuid}/{shard} cannot reach it at all
(Code: 36, {uuid} needs an explicit UUID).
I am not narrowing the exemption here, because the factory cannot express the condition:
StorageFactory::Arguments carries table_id, mode and is_restore_from_backup only, and nothing
about the rename. The renaming decision is in BackupMetadataFinder::findTableInBackupImpl
(renaming_map.getNewTableName), and a check driven from RestorerFromBackup::createTable hits a
real problem: resolve takes a Replicated database's {shard}/{replica} only under
isDDLOrOnClusterInternal, which is established later, so it can evaluate the wrong macros or reject
a valid renamed restore. That is a separate design with its own controls, so it gets its own PR
rather than a widened flag here. It is listed as not addressed in the description, and I have the
numbers and the candidate design (was_renamed plumbed to a point with a resolved context) recorded
for it.
Sorry, something went wrong.
| /// Every other route re-derives a table that already exists and must keep loading. Such a | ||
| /// replay can arrive at CREATE, so `mode` cannot tell it apart and the context carries it. | ||
| const bool validate_substitutions = (args.mode <= LoadingStrictnessLevel::CREATE | ||
| || (args.mode == LoadingStrictnessLevel::ATTACH && !args.query.attach_short_syntax)) |
There was a problem hiding this comment.
attach_short_syntax is not equivalent to “replay stored replicated metadata”. ATTACH TABLE ... AS REPLICATED enters the short-attach branch, but InterpreterCreateQuery::createTable first rewrites the stored MergeTree definition via convertMergeTreeTableIfPossible (src/Interpreters/InterpreterCreateQuery.cpp:1793-1818), and DatabaseOrdinary::setMergeTreeEngine then synthesizes fresh Replicated*MergeTree(default_replica_path, default_replica_name, ...) arguments from the current table name (src/Databases/DatabaseOrdinary.cpp:144-158).
Because create.attach_short_syntax stays true, this path still skips the new {database} / {table} validation even though it is creating a brand-new replicated definition. A detached table named like victim/replicas/ghost (or one in an unsafe database name) can therefore still inject into another table's keeper subtree through ATTACH ... AS REPLICATED.
Sorry, something went wrong.
There was a problem hiding this comment.
Citations are accurate: convertMergeTreeTableIfPossible -> DatabaseOrdinary::setMergeTreeEngine
does synthesize fresh ReplicatedMergeTree(default_replica_path, default_replica_name) from the
current name, attach_short_syntax stays true, and the new validation is skipped. Measured:
ATTACH TABLE ... AS REPLICATED on a table named victim/replicas/ghost succeeds and resolves to
/clickhouse/tables/s1/<db>/victim/replicas/ghost, inside the victim's subtree.
The injection does not complete on this route, which is why it is exempt. Right after the attach the
table is is_readonly = 1 with total_replicas = 0 and the victim's replicas/ still holds only
its own replica, and that is still true after a full server restart, so nothing is planted lazily
either. The phantom appears only after SYSTEM RESTORE REPLICA, which is a separate privilege: a
user with database-level CREATE TABLE/DROP/INSERT/SELECT gets Code: 497 Not enough privileges for it while their ATTACH ... AS REPLICATED succeeds. So reaching the damage takes an
administrator acting on the attacker's table, unlike the CREATE route this PR closes, where an
ordinary user gets there alone.
This is the case listed in the description as ATTACH ... AS REPLICATED (judged as a re-attach;
planting znodes needs SYSTEM RESTORE REPLICA). Leaving it as disclosed rather than validating it
here, since validating the short-attach branch is what would refuse to load existing path-unsafe
tables.
Sorry, something went wrong.
There was a problem hiding this comment.
Correcting my own assessment of the ATTACH ... AS REPLICATED route above: it is a real bypass, and
my reason for exempting it was wrong.
I argued the phantom needs SYSTEM RESTORE REPLICA and so takes an administrator. The grant is
table-scoped, but it is checked against the attacker's own table rather than the victim, and it is
included in ALL ON <db>.*. My measurement had used a grant set that excludes it, so the
Code: 497 I quoted was an artifact of that set.
Measured against the merged fix: a user with GRANT ALL ON shared.* and every privilege on
shared.victim revoked can still take shared.victim to total_replicas = 2 with an inactive ghost
replica, after which ALTER ... SETTINGS alter_sync = 2 fails Code: 999 KEEPER_EXCEPTION. Repair
needs an administrator. Details and the full control set are in
#115967, which I am picking up.
The CREATE-route fix in this PR is unaffected and still holds.
Sorry, something went wrong.
Build profile diff (arm_release)Comparing 22bea9043 with master 165d6a3f5 (stripped binary size, per-symbol sizes and ThinLTO time; object sizes against the warmup build of bcb7844b4; compile times per translation unit against the most recent warmup build that recompiled it). ✅ No significant changes. Binary sizes
Only the stripped binary is compared: the official master build keeps debug symbols while PR builds strip them, so the other binaries differ by construction. Object file sizes102 object files changed (+40.39 KiB total), 0 added.
737 more object files are built by the master warmup baseline only (it builds every object-file target, a pull request build only clickhouse-bundle) and not compared. Compile time of recompiled translation units1259 translation units recompiled, 8014 s compile time in total, 1259 of them have a recent master baseline. Median compile-time ratio to the baselines is ×1.08 (machine-speed difference or a change affecting every TU); per-TU deltas below are relative to that ratio. |
Sorry, something went wrong.
CI finish ledger - 22bea90CI is fully finished on this head: 171 unique check-runs, none queued or in progress, Config Workflow and Finish Workflow both green, Style check 0/14 failures, Fast test 0 failed / 11020 passed. No failures, so there is nothing to own. 154 success / 17 skipped / 0 non-green; praktika's own report gives top=OK with 154 OK and 18 skipped children. CIDB holds 0 FAIL/ERROR rows for this commit against a 301243-row positive control on the same SHA, so the query is not blind. The bugfix validation actually ran rather than being skipped, and its result is the intended one: Bugfix validation (functional tests, amd64) and (aarch64) both conclude OK reporting Failed: 2, Passed: 0, which is the check working as designed. That job runs the new regression test against master and requires it to fail there, so 2 failures on the master binary on both architectures is the confirmation that the test reproduces the bug. The two integration tests legs report Skipped, no integration tests updates and (unit tests) reports Skipped: no changed unit-test files, both correct for this diff. Session id: cron:our-pr-ci-monitor:20260820-150000 |
Sorry, something went wrong.
|
📊 Cloud Performance Report ✅ AI verdict: no_change — no significant changes across 34 queries analysed This PR only adds validation of ReplicatedMergeTree ZooKeeper path and replica-name macro substitutions at table CREATE, ATTACH, and recovery time; it does not touch the SELECT execution path that TPC-H queries run. The flagged Q17 regression (+59.4%) is therefore downgraded to not-sure: it cannot plausibly be caused by this change and the source measurements were noisy across only a handful of runs. Q7 (+16.4%) stays not-sure as well, sitting inside master's normal variance band with CPU time essentially unchanged. No query in this PR exercises the modified code, so both deltas read as run-to-run variance rather than real PR effects. clickbench🟢 No significant changes tpch_adapted_1_official⚠️ 2 inconclusive Flagged queries (2 of 22)
Change = percent below ×2; the ratio of medians (×N faster/slower) beyond, where percent understates the scale. q-value = BH-FDR adjusted p; smaller is stronger evidence. MIRAI flags a query when q < fdr_q (default 0.10) — the value the verdict is based on. Debug info
|
Sorry, something went wrong.
Reproducer
-- clickhouse-server with Keeper configured
CREATE TABLE victim (c0 Int) ENGINE = ReplicatedMergeTree('/clickhouse/inj/{database}/{table}', 'r1') ORDER BY c0;
CREATE TABLE `victim/replicas/ghost` (c0 Int) ENGINE = ReplicatedMergeTree('/clickhouse/inj/{database}/{table}', 'r1') ORDER BY c0;
SELECT total_replicas, replica_is_active FROM system.replicas WHERE database = currentDatabase() AND table = 'victim';
-- observed: 2, {'ghost':0,'r1':0} (expected: 1, {'r1':1})
-- the second table's znodes land inside victim's /replicas subtree; the phantom
-- inactive replica then breaks ALTER ... alter_sync=2 and hangs OPTIMIZE FINAL,
-- and survives DROP + restart until SYSTEM DROP REPLICA.Results:
Backport the fix to 26.7, 26.6, 26.5, 26.3, 25.8. CC component owner: @alexey-milovidov @CheSema Analysis metadatacomp-replication · Severity P2 · Finding phase_d_pr114006
|
Sorry, something went wrong.
…CATED A plain MergeTree table has no ZooKeeper path, so ATTACH TABLE ... AS REPLICATED mints one out of the server's default_replica_path and the table's own name. It then marks the synthesized definition attach_short_syntax, which disables validate_substitutions in the storage factory and so skips the path checks added in ClickHouse#114006. That exemption exists because an ordinary short ATTACH re-derives the path of a table that already exists and must keep loading whatever the stored path expands to. A conversion is the opposite case: the path is brand new. On a server configured with a name-based template, which the engine's own documentation recommends and which is not the {uuid}-based default, a table named `victim/replicas/ghost` therefore resolves to a path inside victim's own subtree. Converting it and running SYSTEM RESTORE REPLICA registers a second replica of victim, whose ALTER ... SETTINGS alter_sync = 2 then fails with a KEEPER_EXCEPTION over the stray replica's missing log_pointer while SELECT and INSERT keep working. Both statements are covered by GRANT ALL ON <database>.*, so no privilege on the affected table is required, and only an administrator can clear the stray replica. Add a side-effect-free helper next to the existing checkReplicaPathExists that resolves the prospective path and replica name through TableZnodeInfo::resolve with validation enabled, and call it from both conversion routes, which are the only two callers of setMergeTreeEngine. No new validation logic is introduced. The call site in the interpreter is deliberately ahead of all three of that function's irreversible steps. clearTransactionMetadata removes txn_version.txt from every part on every disk with no rollback, and per 04492_attach_as_replicated_clears_tmp_txn_version a part missing that file is read back as a rolled-back transaction and discarded as Outdated, so a rejection sited after it could lose rows. Measured: with the check moved down, the conversion is rejected identically but every txn_version.txt is already gone. On the convert_to_replicated flag file route the rejection happens during startup, so the server refuses to start. That matches what the neighbouring checkReplicaPathExists already does on the same route, and the recovery is the same: delete the flag file, after which the table loads unchanged. Closes: ClickHouse#115967
Related: #22970
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
A table or database whose name is substituted into a ReplicatedMergeTree ZooKeeper path through the {database} or {table} macro is now rejected if the name would not be a single safe path component. Previously a name containing / was spliced in unescaped, so the table registered its znodes inside another table's keeper subtree and permanently broke ALTER and OPTIMIZE there. A . or .. component, a control character and macro syntax are rejected too. Existing tables keep loading and attaching.
Description
TableZnodeInfo::resolve substitutes {database}/{table} into a ReplicatedMergeTree keeper path with no escaping and no validation. With such a template (shipped commented out in programs/server/config.xml, recommended by the docs), this
registers a permanently inactive replica under inj.victim. ALTER ... alter_sync = 2 then fails with KEEPER_EXCEPTION while still applying the metadata change, and OPTIMIZE ... FINAL hangs. SELECT/INSERT keep working, so the table looks healthy; the damage survives a plain DROP and a restart, and needs SYSTEM DROP REPLICA. A ./.. component and a control character reach a znode name the same way, and both are illegal in the ZooKeeper data model, so such DDL is accepted by ClickHouse Keeper and refused by ZooKeeper.
The name is rejected rather than escaped, because escaping would change the znode name and so silently repoint an existing table. Only fresh definitions are validated, so short ATTACH, startup, RESTORE, SYSTEM RESTART REPLICA and Replicated-database recovery keep working: an existing path-unsafe table still loads, restarts and recovers.
Not addressed: an explicit overlapping zookeeper_path (#22970; reachable with no macro, so substitution correctness rather than a privilege boundary), RESTORE ... AS <name>, ATTACH ... AS REPLICATED (judged as a re-attach; planting znodes needs SYSTEM RESTORE REPLICA), and the Kafka, ObjectStorageQueue and Paimon keeper paths.
New stateless tests, confirmed to fail pre-fix.
Why three checks at two levels, and why validation is opt-inControl characters are rejected over U+0000-U+001F and U+007F-U+009F. ZooKeeper also rejects the surrogate, private-use and non-character ranges; those are left alone, since names such as таблица_🚀_表 work today.
/ and braces are checked on the substituted value: in the assembled path a substituted / is indistinguishable from a template separator, and a value carrying macro syntax would be expanded again by the next pass (a lone } can close a brace opened by a configured macro).
./.. and control bytes are checked on the expanded string instead, because a legal component can be assembled from the value plus the surrounding template, possibly only in a later pass. Checking the raw value there would wrongly reject an embedded substitution such as table_{table} for a table named ., which yields the legal table_..
Both macro passes are validated. The second is where a configured macro whose value contains {database}/{table} expands, which the test suite's own macros.xml ships as default_path_test.
Validation is requested explicitly by the storage factory rather than derived from mode, because extractZooKeeperPathFromReplicatedTableDef re-derives an existing table's path with a hardcoded LoadingStrictnessLevel::CREATE and swallows BAD_ARGUMENTS into nullopt; validating there would silently drop that table's replicated data path from a backup.
Each expanded output now gets its own MacroExpansionInfo: Macros::expand returns early for a string without { without clearing the flags, so a shared one made expanded_table sticky and would have rejected an explicitly written replica name merely because the path substituted.
Workflow [PR]
Sync PR [sync-upstream/pr/114006]