Fix query parameters colliding with builtin setting names

Query parameters were transported by packing them into a Settings object,
so a parameter whose name collided with a builtin setting or alias
(limit, offset, max_threads, log_comment, ...) got value-normalized,
alias-resolved, or misquoted, poisoning every later SET param_* in the
session. Bypass Settings::set/read and the builtin-setting accessor
entirely for parameters, keeping them on the existing custom-setting wire
encoding. No protocol version bump required.

Closes: https://github.com/ClickHouse/ClickHouse/issues/85768
Closes: https://github.com/ClickHouse/ClickHouse/issues/91168
Related: https://github.com/ClickHouse/ClickHouse/pull/109526

src/Client/Connection.cpp

@@ -1061,12 +1061,7 @@ void Connection::sendQuery(
     writeStringBinary(query, *out);
 
     if (server_revision >= DBMS_MIN_PROTOCOL_VERSION_WITH_PARAMETERS)
-    {
-        Settings params;
-        for (const auto & [name, value] : query_parameters)
-            params.set(name, value);
-        params.write(*out, SettingsWriteFormat::STRINGS_WITH_FLAGS);
-    }
+        writeQueryParameters(query_parameters, *out);
 
     maybe_compressed_in.reset();
     if (maybe_compressed_out && maybe_compressed_out != out)

src/Core/Settings.cpp

@@ -9135,17 +9135,32 @@ void Settings::dumpToMapColumn(IColumn * column, bool changed_only) const
     impl->dumpToMapColumn(column, changed_only);
 }
 
-NameToNameMap Settings::toNameToNameMap() const
+void writeQueryParameters(const NameToNameMap & parameters, WriteBuffer & out)
 {
-    NameToNameMap query_parameters;
-    for (const auto & param : *impl)
+    for (const auto & [name, value] : parameters)
     {
-        std::string value;
-        ReadBufferFromOwnString buf(param.getValueString());
-        readQuoted(value, buf);
-        query_parameters.emplace(param.getName(), value);
+        BaseSettingsHelpers::writeString(name, out);
+        BaseSettingsHelpers::writeFlags(BaseSettingsHelpers::Flags::CUSTOM, out);
+        BaseSettingsHelpers::writeString(SettingFieldCustom(Field(value)).toString(), out);
     }
-    return query_parameters;
+    BaseSettingsHelpers::writeString(std::string_view{}, out);
+}
+
+NameToNameMap readQueryParameters(ReadBuffer & in)
+{
+    NameToNameMap parameters;
+    while (true)
+    {
+        String name = BaseSettingsHelpers::readString(in);
+        if (name.empty())
+            break;
+        std::ignore = BaseSettingsHelpers::readFlags(in);
+        String value;
+        ReadBufferFromOwnString buf(BaseSettingsHelpers::readString(in));
+        readQuoted(value, buf);
+        parameters.emplace(std::move(name), std::move(value));
+    }
+    return parameters;
 }
 
 void Settings::write(WriteBuffer & out, SettingsWriteFormat format) const

src/Core/Settings.h

@@ -166,7 +166,6 @@ struct Settings
 
     void dumpToSystemSettingsColumns(MutableColumnsAndConstraints & params) const;
     void dumpToMapColumn(IColumn * column, bool changed_only = true) const;
-    NameToNameMap toNameToNameMap() const;
 
     void write(WriteBuffer & out, SettingsWriteFormat format = SettingsWriteFormat::DEFAULT) const;
     void read(ReadBuffer & in, SettingsWriteFormat format = SettingsWriteFormat::DEFAULT);
@@ -189,4 +188,11 @@ struct Settings
 private:
     std::unique_ptr<SettingsImpl> impl;
 };
+
+/// Query parameters are transported as raw (name, value) string pairs using the Custom-setting
+/// wire encoding (SettingsWriteFormat::STRINGS_WITH_FLAGS), but bypassing Settings::set/read and
+/// the builtin-setting accessor entirely, so a parameter name colliding with a builtin setting or
+/// alias can never be value-normalized, alias-resolved, or misquoted (issue #85768).
+void writeQueryParameters(const NameToNameMap & parameters, WriteBuffer & out);
+NameToNameMap readQueryParameters(ReadBuffer & in);
 }

src/Server/TCPHandler.cpp

@@ -2485,9 +2485,9 @@ void TCPHandler::processQuery(std::shared_ptr<QueryState> & state)
 
     readStringBinary(state->query, *in);
 
-    Settings passed_params;
+    NameToNameMap passed_params;
     if (client_tcp_protocol_version >= DBMS_MIN_PROTOCOL_VERSION_WITH_PARAMETERS)
-        passed_params.read(*in, settings_format);
+        passed_params = readQueryParameters(*in);
 
     if (is_interserver_mode)
     {
@@ -2677,7 +2677,7 @@ void TCPHandler::processQuery(std::shared_ptr<QueryState> & state)
     /// so we have to apply the changes first.
     state->query_context->setCurrentQueryId(state->query_id);
 
-    state->query_context->addQueryParameters(passed_params.toNameToNameMap());
+    state->query_context->addQueryParameters(passed_params);
 
     state->allow_partial_result_on_first_cancel = state->query_context->getSettingsRef()[Setting::partial_result_on_first_cancel];
 

tests/queries/0_stateless/04545_set_param_colliding_with_setting.reference

@@ -0,0 +1,11 @@
+5
+0
+1
+7
+3	hi
+John Doe
+0
+5
+limit	1
+max_threads	1
+a\'b\\c

tests/queries/0_stateless/04545_set_param_colliding_with_setting.sql

@@ -0,0 +1,52 @@
+-- Setting a query parameter whose name collides with a builtin setting
+-- (limit / offset / max_threads / log_comment) used to poison the client
+-- parameter map so every later SET param_* failed with CANNOT_PARSE_QUOTED_STRING.
+
+-- Snapshot the real settings before touching any colliding parameter, so the "no leak"
+-- check below doesn't have to assume a particular default (client/profile/randomized
+-- test settings can all make max_threads != its built-in default).
+CREATE TEMPORARY TABLE settings_before AS
+SELECT name, value, changed FROM system.settings WHERE name IN ('max_threads', 'limit');
+
+SET param_limit = 2;
+SET param_other = 5;
+SELECT {other:UInt32};
+
+SELECT number FROM numbers(10) LIMIT {limit:UInt8};
+
+SET param_offset = 7;
+SELECT {offset:UInt32};
+
+SET param_log_comment = 'hi';
+SET param_z = 3;
+SELECT {z:UInt8}, {log_comment:String};
+
+-- Non-colliding parameters keep working and string values are preserved.
+SET param_name = 'John Doe';
+SELECT {name:String};
+
+-- A parameter whose name collides with a setting that normalizes its value must
+-- round-trip the original text, not the setting's normalized form. max_threads = 0
+-- is stored as auto(<cores>) by SettingFieldMaxThreads, which used to leak into the
+-- parameter value (issue #85768).
+SET param_max_threads = 0;
+SELECT {max_threads:UInt64};
+
+-- A parameter whose name collides with a setting ALIAS must not be alias-resolved
+-- into the canonical setting name and value. insert_distributed_sync is an alias of
+-- distributed_foreground_insert (a Bool), which used to reject / rewrite the value.
+SET param_insert_distributed_sync = 5;
+SELECT {insert_distributed_sync:UInt8};
+
+-- A colliding parameter must never leak into the actual setting of the same name,
+-- regardless of what that setting's value/changed state was before this test ran.
+SELECT b.name, (b.value, b.changed) = (a.value, a.changed) AS unchanged
+FROM settings_before AS b
+INNER JOIN (SELECT name, value, changed FROM system.settings WHERE name IN ('max_threads', 'limit')) AS a
+ON a.name = b.name
+ORDER BY b.name;
+
+-- Quoting must round-trip for colliding names carrying string content with embedded
+-- quotes/backslashes, the same as it already does for a non-colliding parameter name.
+SET param_log_comment = 'a\'b\\c';
+SELECT {log_comment:String};

tests/queries/0_stateless/04546_set_param_colliding_with_setting_cli.reference

@@ -0,0 +1,4 @@
+foobar
+0
+1
+3

tests/queries/0_stateless/04546_set_param_colliding_with_setting_cli.sh

@@ -0,0 +1,12 @@
+#!/usr/bin/env bash
+
+CUR_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
+# shellcheck source=../shell_config.sh
+. "$CUR_DIR"/../shell_config.sh
+
+# Query parameters passed via --param_<name> on the command line must not be corrupted
+# when <name> collides with a builtin setting name (issue #91168).
+$CLICKHOUSE_CLIENT --param_max_threads="foobar" --query "SELECT {max_threads:String}"
+$CLICKHOUSE_CLIENT --param_limit=2 --query "SELECT number FROM numbers(10) LIMIT {limit:UInt8}"
+$CLICKHOUSE_CLIENT --param_offset=3 --query "SELECT {offset:UInt32}"
+