Skip to content

Fix ASAN integration with boost fibers in case of address+undefined sanitizers#100702

Merged
azat merged 2 commits intoClickHouse:masterfrom
azat:fix-fibers-asan-integration
Mar 25, 2026
Merged

Fix ASAN integration with boost fibers in case of address+undefined sanitizers#100702
azat merged 2 commits intoClickHouse:masterfrom
azat:fix-fibers-asan-integration

Conversation

@azat
Copy link
Copy Markdown
Member

@azat azat commented Mar 25, 2026 •
edited
Loading

@azat azat requested a review from alexey-milovidov March 25, 2026 13:10
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Mar 25, 2026 •
edited
Loading

Workflow [PR], commit [486160e]

Summary: ❌

job_name test_name status info comment
Unit tests (asan_ubsan) error

AI Review

Summary

This PR restores sanitizer integration for fiber context builds when SANITIZE contains combined values (for example address,undefined) by switching Boost context checks from exact match to regex match, and reverts the previous workaround in FiberStack/AsyncTaskExecutor. I did not find correctness, safety, performance, or compatibility issues in the changed code.

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

@azat azat mentioned this pull request Mar 25, 2026
Closed
@clickhouse-gh clickhouse-gh bot added pr-not-for-changelog This PR should not be mentioned in the changelog submodule changed At least one submodule changed in this PR. labels Mar 25, 2026
azat
azat commented Mar 25, 2026
View reviewed changes
contrib/boost-cmake/CMakeLists.txt
endif()

if (SANITIZE STREQUAL "address")
if (SANITIZE MATCHES "address")
Copy link
Copy Markdown
Member Author

@azat azat Mar 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix, since once CI started to use -DSANITIZE=address,undefined we lost the ASAN integrations for fibers

@alexey-milovidov alexey-milovidov self-assigned this Mar 25, 2026
@azat
Copy link
Copy Markdown
Member Author

azat commented Mar 25, 2026

Test

$ clickhouse server -- --listen_host=::
select count() from remote('127.0.{0..100}.{1..230}') settings table_function_remote_max_addresses=100000

Compiled from sources (to make sure my setup is the same):

So we are good now.

@azat azat enabled auto-merge March 25, 2026 13:35
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Mar 25, 2026

LLVM Coverage Report

Metric Baseline Current Δ
Lines 84.10% 84.10% +0.00%
Functions 24.50% 24.50% +0.00%
Branches 76.70% 76.70% +0.00%

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

@azat
Copy link
Copy Markdown
Member Author

azat commented Mar 25, 2026 •
edited
Loading

Unit tests (asan_ubsan)

This looks like a separate problem I will take a look separately - https://play.clickhouse.com/play?user=play&run=1#U0VMRUNUIGNoZWNrX3N0YXJ0X3RpbWUsIGNoZWNrX25hbWUsIHRlc3RfbmFtZSwgcmVwb3J0X3VybApGUk9NIGNoZWNrcwpXSEVSRSBjaGVja19zdGFydF90aW1lID49IG5vdygpIC0gSU5URVJWQUwgMjQgSE9VUgovLyAgICBBTkQgKGhlYWRfcmVmID0gJ21hc3RlcicgQU5EIHN0YXJ0c1dpdGgoaGVhZF9yZXBvLCAnQ2xpY2tIb3VzZS8nKSkKICAgIEFORCBjaGVja19zdGF0dXMgIT0gJ3N1Y2Nlc3MnCiAgICBBTkQgY2hlY2tfbmFtZSA9ICdVbml0IHRlc3RzIChhc2FuX3Vic2FuKScKICAgIGFuZCB0ZXN0X2NvbnRleHRfcmF3IGxpa2UgJyVzdGFjay1idWZmZXItb3ZlcmZsb3cgb24gYWRkcmVzcyUnCk9SREVSIEJZIGNoZWNrX3N0YXJ0X3RpbWU=

Fixed in b65f2d2

Report should be fixed in #100744

@azat azat added this pull request to the merge queue Mar 25, 2026
Merged via the queue into ClickHouse:master with commit 8251fa3 Mar 25, 2026
151 of 153 checks passed
@azat azat deleted the fix-fibers-asan-integration branch March 25, 2026 17:51
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 added the pr-synced-to-cloud The PR is synced to the cloud repo label Mar 25, 2026
groeneai pushed a commit to groeneai/ClickHouse that referenced this pull request Mar 26, 2026
Merge master to pick up ASAN fix (PR ClickHouse#100702)
7195707
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexey-milovidov alexey-milovidov alexey-milovidov approved these changes

Assignees

@alexey-milovidov alexey-milovidov

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog pr-synced-to-cloud The PR is synced to the cloud repo submodule changed At least one submodule changed in this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AddressSanitizer: stack-use-after-scope (STID: 2136-3203)

3 participants

@azat @alexey-milovidov @robot-clickhouse-ci-1