ClickHouse / ClickHouse/ClickHouse

Clamp applied to a copy: nested `use_query_cache` still escapes `readonly`/`CONST`

Open
#117,226 0 comments 0 reactions 2 assignees Claimed by @tempel-it View on GitHub
bug comp-query-analyzer comp-query-execution comp-rbac
Dominant language
C++
Stars
49.9k
Forks
9k
Avg merge
21h 32m
Merged PRs (30d)
515

Description

### Describe what's wrong

**A session that may not change settings (`readonly = 1`, or a `use_query_cache ... CONST` profile constraint) can still turn on the query result cache from inside a subquery/CTE, and then reads stale rows from it. `SELECT sum(x) FROM (SELECT x FROM t SETTINGS use_query_cache = 1) SETTINGS readonly = 1` returns the cached value after the table changed, while the top-level form `SELECT sum(x) FROM t SETTINGS use_query_cache = 1, readonly = 1` is refused with `READONLY`. Second symptom: a non-castable value in the same clause now surfaces `BAD_GET` (code 170) instead of the parse error it raised before this PR.**

- **Root cause:** [`src/Analyzer/QueryTreeBuilder.cpp:319`](https://github.com/ClickHouse/ClickHouse/blob/dd7889a115c63e/src/Analyzer/QueryTreeBuilder.cpp#L319) - `settings_changes = set_query.changes;` assigns the raw clause. The added comment claims the copy is only cosmetic ("the tree's AST and hash are unchanged"), but `Planner.cpp:2472` consumes the same list semantically, so the clamp never reaches the one setting the Planner reads off the node. For a value that fails `castValueUtil`, `SettingsConstraints::checkOrClamp` erases it silently ([`src/Access/SettingsConstraints.cpp:290`](https://github.com/ClickHouse/ClickHouse/blob/dd7889a115c63e/src/Access/SettingsConstraints.cpp#L290)) instead of throwing, and the Planner then calls `safeGet` on the raw `String` Field.

Analysis details (evidence, affected locations, impact)

**Why we believe this is a bug:** `QueryTreeBuilder::buildSelectExpression` ([`src/Analyzer/QueryTreeBuilder.cpp:316-319`](https://github.com/ClickHouse/ClickHouse/blob/dd7889a115c63e/src/Analyzer/QueryTreeBuilder.cpp#L316-L319)) clamps a COPY and stores the unclamped original on the `QueryNode`. `Planner::buildPlanForQueryNode` -> `shouldUseQueryCacheForSubquery` ([`src/Planner/Planner.cpp:2472-2475`](https://github.com/ClickHouse/ClickHouse/blob/dd7889a115c63e/src/Planner/Planner.cpp#L2472-L2475)) reads that unclamped list, not the node's context, and returns `change.value.safeGet()` for `use_query_cache`.

**Affected locations:**
- [`src/Analyzer/QueryTreeBuilder.cpp:319`](https://github.com/ClickHouse/ClickHouse/blob/dd7889a115c63e/src/Analyzer/QueryTreeBuilder.cpp#L319) — unclamped clause stored on QueryNode
- [`src/Planner/Planner.cpp:2472`](https://github.com/ClickHouse/ClickHouse/blob/dd7889a115c63e/src/Planner/Planner.cpp#L2472) — shouldUseQueryCacheForSubquery reads the unclamped list
- [`src/Access/SettingsConstraints.cpp:290`](https://github.com/ClickHouse/ClickHouse/blob/dd7889a115c63e/src/Access/SettingsConstraints.cpp#L290) — checkOrClamp erases uncastable/unknown changes instead of throwing

**Impact:** The constraint bypass this PR exists to close remains open for `use_query_cache`, and the resulting cached entry is served back to the same constrained user, so the query returns stale rows after the underlying table changes. Additionally, the query-cache machinery now runs in a state the context says is off, producing errors such as `QUERY_CACHE_USED_WITH_SYSTEM_TABLE` for a `readonly` session that never enabled the cache, and `BAD_GET` for a mistyped value.

### Does it reproduce on most recent release?

Yes — confirmed on current `master` (commit `dd7889a115c63e`).

### How to reproduce

[▶ Run on ClickHouse Fiddle](https://fiddle.clickhouse.com/510dc931-0537-4495-8b39-a00e326a8ba6)

Reproducer

```sql
-- Tag no-parallel: reads the server-wide query result cache, which neighbouring
-- 03381_query_result_cache_* tests drop.

-- A SETTINGS clause nested in a subquery must not enable the query result cache
-- when the session may not change settings.

DROP TABLE IF EXISTS t_qc_readonly;
CREATE TABLE t_qc_readonly (x UInt64) ENGINE = MergeTree ORDER BY x;
INSERT INTO t_qc_readonly VALUES (1);

-- Control: a nested clause is dropped for a session that may not change settings.
SELECT s = getSetting('max_block_size') FROM (SELECT getSetting('max_block_size') AS s SETTINGS max_block_size = 999) SETTINGS readonly = 1;

SELECT sum(x) FROM (SELECT x FROM t_qc_readonly SETTINGS use_query_cache = 1) SETTINGS readonly = 1;
SELECT count() FROM system.query_cache WHERE is_subquery = 1 AND query LIKE '%t_qc_readonly%';

INSERT INTO t_qc_readonly VALUES (10);
SELECT sum(x) FROM (SELECT x FROM t_qc_readonly SETTINGS use_query_cache = 1) SETTINGS readonly = 1;

DROP TABLE IF EXISTS t_qc_readonly;
```

### Expected behavior

```
1
1
0
11
```

### Error message and/or stacktrace

```
1
1
1
1
```

Suggested fix

Store the clamped list on the node: `settings_changes = checked_changes;` (or clamp `set_query.changes` in place). If the raw clause must be kept for AST/hash stability, `shouldUseQueryCacheForSubquery` must read `use_query_cache` from the node's context instead of from `getSettingsChanges()`.

Additional context

**Open risks:**
- `TableFunctionNode::getSettingsChanges` (src/Analyzer/QueryTreeBuilder.cpp:861) carries a `SETTINGS` clause from a table function argument and is not clamped at all by this PR - a separate path that was not in scope here.

Found during automated review of [PR #115397](https://github.com/ClickHouse/ClickHouse/pull/115397). Severity P2 · Finding `h_pr115397_001`

cc @tempel-it @pufit (from #115397)

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.