Dockerfile.windows: enable assertions and disable /GF for the sanitizer build#298
Conversation
Matches the Linux/macOS Dockerfiles, which set ENABLE_ASSERTS=ON whenever ENABLE_SANITIZERS is non-empty. Without this, bun-webkit-windows-amd64-asan ships with ASSERT_ENABLED=0, so ASSERT_ENABLED-gated symbols that the JSC headers reference from inline code (ObjectInitializationScope, DoesGCCheck::verifyCanGC, StrongRefTracker, Structure::checkConsistency, MarkedBlock::assertMarksNotStale, ...) are absent from the .lib. Consumers that compile with ASSERT_ENABLED=1 then fail to link, and consumers that compile with ASSERT_ENABLED=0 get a different WTF struct layout than the Linux/macOS asan artifacts (ASSERT_ENABLED gates fields like RefCountedBase::m_deletionHasBegun).
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — brings the Windows sanitizer leg into parity with Dockerfile / Dockerfile.macos.
What was reviewed:
- Confirmed
DockerfileandDockerfile.macosuse the identicalENABLE_ASSERTS=AUTO→ON-when-sanitizers pattern. - Confirmed
ENABLE_ASSERTSacceptsAUTO/ON/OFFinSource/cmake/OptionsCommon.cmake, so the non-sanitizer path (AUTO) is a no-op vs. the prior implicit default. - Checked shell chaining: the new
; \continuation between the two assignment lines is fine (assignments can't fail; matches the existingSAN_FLAGS=""; if ...idiom on the same line).
Extended reasoning...
Overview
Two-line change to Dockerfile.windows in the WebKit cmake configure step: introduce a shell-local ENABLE_ASSERTS variable defaulting to AUTO, flip it to ON inside the existing if [ -n "${ENABLE_SANITIZERS}" ] branch, and pass it through as -DENABLE_ASSERTS=.... This mirrors the exact pattern already present in Dockerfile (lines 263-266, 290) and Dockerfile.macos (lines 187-188, 214).
Security risks
None. This is a CI/build-artifact configuration flag that toggles ASSERT_ENABLED in the produced sanitizer prebuilt. No auth, crypto, permissions, or runtime-input handling is touched.
Level of scrutiny
Low. It's a mechanical parity fix for a build matrix leg (bun-webkit-windows-amd64-asan) and only affects the sanitizer variant — the non-sanitizer path now passes -DENABLE_ASSERTS=AUTO explicitly, which OptionsCommon.cmake:293 shows is the existing cache default, so behavior there is unchanged. Any regression would surface immediately as a CI build failure rather than a runtime bug.
Other factors
- The shell edit keeps the surrounding
&&-chain intact: the split intoSAN_FLAGS=""; ENABLE_ASSERTS="AUTO"; \followed byif ... fi && \is equivalent to the previous single line since variable assignments always succeed and theifstill terminates with&&. OptionsCommon.cmakeconfirmsENABLE_ASSERTSis a tri-state (AUTO/ON/OFF) and thatONprepends-DASSERT_ENABLED=1, which is exactly what the PR description says is needed for the ASSERT-gated symbols to be present in the.lib.- No prior reviewer comments to address; coderabbit was rate-limited and left no substantive feedback.
clang-cl /O2 implies /GF, which emits each string literal as a SELECT_ANY COMDAT sized to the original string. The ASAN descriptor for that literal records size_with_redzone, so at runtime __asan_register_globals poisons past the end of the COMDAT section into whatever the linker placed next. In the bun link (larger .rdata than jsc.exe) that manifests as a global-buffer-overflow on first VM init when ICU reads its own empty string literal (merged with an instrumented WTF/JSC literal by the linker). With /GF-, string literals are private unnamed constants and the ASAN padding is part of the section.
robobun
changed the title
There was a problem hiding this comment.
LGTM — small, self-contained build-config change bringing the Windows sanitizer leg into parity with the Linux/macOS Dockerfiles.
What was reviewed:
- Confirmed
DockerfileandDockerfile.macosalready setENABLE_ASSERTS=ONunderENABLE_SANITIZERS; this mirrors that pattern. - Confirmed
ENABLE_ASSERTSis a valid cache var inOptionsCommon.cmakewithAUTOas the default, so non-sanitizer legs are unchanged. - Checked whether
/GF-inCMAKE_C(XX)_FLAGScould be overridden by a later/GFon the compile line — ruled out.
Extended reasoning...
Overview
This PR touches a single file, Dockerfile.windows, and changes three lines in the WebKit cmake configure step: it initializes ENABLE_ASSERTS="AUTO", and inside the existing if [ -n "${ENABLE_SANITIZERS}" ] branch it (a) prepends /GF- to SAN_FLAGS and (b) sets ENABLE_ASSERTS="ON". It then passes -DENABLE_ASSERTS="${ENABLE_ASSERTS}" to cmake. No source code, no runtime logic, no other build legs are touched.
Security risks
None. This is a Docker build recipe for a CI artifact. No new inputs are introduced, no network endpoints, no auth/crypto/permissions. The added flags are compiler/cmake options that only affect the -asan Windows tarball.
Level of scrutiny
Low. The change is:
- Mechanical parity:
Dockerfile(line 263-266, 290) andDockerfile.macos(line 187-188, 214) already do exactly thisENABLE_ASSERTS=AUTO→ON-under-sanitizers dance and pass it to cmake. This PR copies that pattern verbatim. - Scoped to a debug/CI artifact: only the
ENABLE_SANITIZERS-gated leg changes. The non-sanitizer Windows builds getSAN_FLAGS=""andENABLE_ASSERTS="AUTO", whichOptionsCommon.cmake:293confirms is the existing cmake default — a no-op for those legs. - Well-motivated: the PR description gives a concrete, reproducible failure (
global-buffer-overflowinicu_73::StringPiece::StringPieceon first VM construction in bun's link) that/GF-addresses, and a concrete link/ABI-layout mismatch thatENABLE_ASSERTS=ONaddresses. The author has clearly exercised the resulting artifact.
Other factors
- The bug hunting system found no issues. One finder raised the possibility that
/GF-(placed inCMAKE_C(XX)_FLAGS) is overridden by a later/GFon the compile line (from/O2inCMAKE_CXX_FLAGS_RELEASEoradd_compile_options(/Oy- /Gw /Gy /GF)inOptionsMSVC.cmake:98); the verifier examined this and refuted it. - Shell syntax is correct: the new
ENABLE_ASSERTS="AUTO";assignment is chained with the surrounding&&via the followingif ...; fi &&, matching the existingSAN_FLAGS/EXE_LD_FLAGSidiom in the same RUN block. - No prior reviews from me on this PR; the only timeline entry is a coderabbit rate-limit notice.
Given the tiny scope, the established pattern being copied, and the fact that only a sanitizer CI artifact is affected, this is safe to approve without human review.
Preview Builds
|
2 participants
Matches the Linux/macOS Dockerfiles, which set
ENABLE_ASSERTS=ONwheneverENABLE_SANITIZERSis non-empty, and additionally disables/GFstring pooling (which/O2turns on implicitly) for the sanitizer leg.ENABLE_ASSERTS
Without this,
bun-webkit-windows-amd64-asanships withASSERT_ENABLED=0, so ASSERT_ENABLED-gated symbols that the JSC headers reference from inline code (ObjectInitializationScope,DoesGCCheck::verifyCanGC,StrongRefTracker,Structure::checkConsistency,MarkedBlock::assertMarksNotStale, ...) are absent from the.lib. A consumer compiling withASSERT_ENABLED=1to match the other-asanprebuilts then fails to link, and a consumer compiling withASSERT_ENABLED=0gets a different WTF struct layout than the Linux/macOS asan artifacts (ASSERT_ENABLEDgates fields likeRefCountedBase::m_deletionHasBegun)./GF-
clang-cl
/O2implies/GF, which emits each string literal as aSELECT_ANYCOMDAT sized to the original string (e.g. 1 byte for""). The ASAN descriptor for that literal still recordssize_with_redzone(32 bytes for"", larger for longer strings), so at runtime__asan_register_globalspoisons past the end of the COMDAT section into whatever the linker placed next. It additionally lets an uninstrumented literal (from ICU, which the sanitizer leg does not instrument) win COMDAT selection over an instrumented one of the same content, with the instrumented descriptor still registered.jsc.exefrom the current tarball avoids this by luck of.rdatalayout. In bun's larger link it surfaces as a deterministicglobal-buffer-overflowinicu_73::StringPiece::StringPieceviau_getTimeZoneFilesDirectoryon firstJSC::VMconstruction, with the report pointing at an instrumented WTF/JSC literal whose redzone covers an adjacent global.With
/GF-, string literals are private unnamed constants; the ASAN padding is part of the section and cannot merge with ICU's uninstrumented literals.Non-sanitizer Windows legs are unchanged (
ENABLE_ASSERTS=AUTOis the existing cmake default;SAN_FLAGSstays empty).