Skip to content

Do not attempt to read from read buffer if client disconnects during fuzzing#100732

Closed
antaljanosbenjamin wants to merge 2 commits intomasterfrom
fix-read-from-cancelled-buffer-in-fuzzer
Closed

Do not attempt to read from read buffer if client disconnects during fuzzing#100732
antaljanosbenjamin wants to merge 2 commits intomasterfrom
fix-read-from-cancelled-buffer-in-fuzzer

Conversation

@antaljanosbenjamin
Copy link
Copy Markdown
Member

Some info about what went wrong:
Root cause: The AST fuzzer (executeASTFuzzerQueries) runs fuzzed internal queries after the original query completes (via onFinish() callback). These internal queries inherit the outer query's InteractiveCancelCallback through CurrentThread::tryGetQueryContext(). When JoinOrderOptimizer::checkLimits() invokes this callback, it calls receivePacketsExpectCancel() which reads from the TCP input buffer in. If the client has disconnected, in gets canceled. The fuzzer's catch(...) swallows the exception and continues. When the fuzzer finishes and the TCPHandler loop calls in->eof(), the chassert(!isCanceled()) in ReadBuffer::next() fires.

Changelog category (leave one):

  • CI Fix or Improvement (changelog entry is not required)

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

…fuzzing

Some info about what went wrong:
Root cause: The AST fuzzer (executeASTFuzzerQueries) runs fuzzed internal queries after the original query completes (via onFinish() callback). These internal queries inherit the outer query's InteractiveCancelCallback through CurrentThread::tryGetQueryContext(). When JoinOrderOptimizer::checkLimits() invokes this callback, it calls receivePacketsExpectCancel() which reads from the TCP input buffer in. If the client has disconnected, in gets canceled. The fuzzer's catch(...) swallows the exception and continues. When the fuzzer finishes and the TCPHandler loop calls in->eof(), the chassert(!isCanceled()) in ReadBuffer::next() fires.
@antaljanosbenjamin antaljanosbenjamin mentioned this pull request Mar 25, 2026
1 task
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Mar 25, 2026

Workflow [PR], commit [68d3e4d]

Summary:

job_name test_name status info comment
AST fuzzer (arm_asan_ubsan) failure
Logical error: ColumnUnique can't contain null values. (STID: 1503-2b15) FAIL cidb

AI Review

Summary

This PR fixes a real cancellation-path issue during AST fuzzing: fuzzed internal queries inherited an interactive cancel callback tied to the client connection, which could cancel the input buffer and later trigger an exception when TCPHandler checked EOF. The fix (resetInputCallbacks + clearing interactive_cancel_callback, plus defensive isCanceled handling before eof) is coherent, minimal, and backed by a regression test that reproduces client disconnect timing. I did not find blockers or major issues in the touched code.

Missing context
  • ⚠️ Most CI checks were still pending during review, so full post-merge confidence from CI artifacts/logs is not yet available.
ClickHouse Rules
Item Status Notes
Deletion logging
Serialization versioning
Core-area scrutiny
No test removal
Experimental gate
No magic constants
Backward compatibility
SettingsChangesHistory.cpp
PR metadata quality
Safe rollout
Compilation time
Final Verdict
  • Status: ✅ Approve

@clickhouse-gh clickhouse-gh bot added the pr-ci label Mar 25, 2026
@Avogar Avogar self-assigned this Mar 25, 2026
@Avogar
Copy link
Copy Markdown
Member

Avogar commented Mar 25, 2026

This is the third PR with this issue :)
#100585 and #100536. I closed mine in faivour of the second one. Let me know what you think is better

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Mar 25, 2026

LLVM Coverage Report

Metric Baseline Current Δ
Lines 84.10% 84.20% +0.10%
Functions 24.60% 24.40% -0.20%
Branches 76.70% 76.70% +0.00%

PR changed lines: PR changed-lines coverage: 100.00% (15/15, 0 noise lines excluded)
Diff coverage report
Uncovered code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants