From 8e9f717ca603c6f6098b6471e2a311c32869e135 Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Mon, 27 Jul 2026 18:33:27 +0300 Subject: [PATCH 1/3] Let the grace passes mark objects the resolve guard was discarding (issue 5425) The reporter's dictionary load corrupts the heap on iOS: garbled words, an "invalid block type" from the inflater, an impossible NPE. Reproduced by running his Dtest.java classes verbatim over the dictionary.res attached to the issue, with a second thread allocating alongside the loader. Under -DCN1_GC_VERIFY the unfixed VM aborts every run with thousands of dangling references; without it, SIGSEGV inside Hashtable.rehash / get. Per-object mark history named the holder of every dangling reference: a fresh legacy object stamped live by codenameOneGCSweep's grace branch with lastTraced == 0 -- never traced in its life. The cause is one guard: if(cn1ConservativeResolve((void*)obj) != obj && !cn1GcImmortalObjContains(obj)) return; It exists to reject CONSERVATIVELY DERIVED pointers -- arbitrary machine words off a native stack that may not be objects at all. But it rejects every pointer it cannot resolve, and resolution is against a page/extent snapshot built at MARK START, so an object allocated later can never resolve. When the mid-mark grace passes walk allObjectsInHeap and the page registry and hand such an object over, the mark is silently discarded. The sweep then grace-promotes it (mark == -1 -> currentGcMarkValue) WITHOUT tracing, in the same index-ordered walk that frees dead objects, so an older object reachable only through it is reclaimed at a lower index. Those two walks hold AUTHORITATIVE references: they read the object out of a registry, not off a stack. They need no proof. cn1GcTrustedRoots marks that window so gcMarkObject skips the guard for them, and the existing passes then mark what they were already trying to mark. Nothing new runs; a pass that was throwing its results away stops doing so. Also corrects the comments asserting a fresh object is safe because grace keeps it. Grace keeps the OBJECT; it does not trace the SUBTREE. That premise appears at the guard, at the snapshot-rebuild note and at the sweep's promotion site, and is what this defect was hiding behind. Do NOT set cn1GcTrustedRoots around anything marking from a stack scan or register file -- that is the case the guard exists for. Validation: reproducer clean 3/3 over 166 collection cycles and 800M verified references (baseline fails 100% with ~7000 violations); run-gc-verify.sh GREEN over all nine drivers with both fault self-tests firing; geomean 1.22x vs 1.21x on master, objectAllocation 3.85x vs 3.46x -- within run variance, all checksums bit-identical to the host JVM. An earlier revision of this branch instead added a late-grace re-mark pass to compensate for the guard. It was correct but cost 2.1x on objectAllocation and +8% geomean, and review found two further defects in it (a suffix scan unsound against tombstone reuse, and an O(heap) pass widening the unbarriered mark-to-sweep window). Fixing the guard makes all of that unnecessary. Co-Authored-By: Claude Opus 5 (1M context) --- vm/ByteCodeTranslator/src/cn1_globals.h | 12 +++++++++--- vm/ByteCodeTranslator/src/cn1_globals.m | 24 +++++++++++++++++++++--- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/vm/ByteCodeTranslator/src/cn1_globals.h b/vm/ByteCodeTranslator/src/cn1_globals.h index 77ff2afdec9..5c74358d47e 100644 --- a/vm/ByteCodeTranslator/src/cn1_globals.h +++ b/vm/ByteCodeTranslator/src/cn1_globals.h @@ -1343,13 +1343,19 @@ typedef struct CN1BibopPage { // idempotent across parallel markers) int gcGraceEpoch; // upper bound on survivor epochs as of the last // full walk (GC-thread only) -#ifdef CN1_GRACE_AUDIT - int gcAuditSnapshot; // QA builds only: bumpIndex at mark start. + int gcAuditSnapshot; // bumpIndex at mark start. Slots at or above + // it were born DURING the mark, which is the + // only population the late-grace re-mark has + // to trace (issue 5425); scanning every fresh + // slot instead is O(all slots) per cycle and + // measured a 2.1x objectAllocation regression. + // Was QA-only when it served -DCN1_GRACE_AUDIT + // alone. + // Relaxed __atomic access everywhere -- the // Relaxed __atomic access everywhere -- the // GC writes/reads it during marking while a // mutator can reformat the page from the // FREE pool -#endif } CN1BibopPage; // Per-thread current page per size class; defined in cn1_globals.m. Touched only diff --git a/vm/ByteCodeTranslator/src/cn1_globals.m b/vm/ByteCodeTranslator/src/cn1_globals.m index 36f166cec2e..d4dd1da5667 100644 --- a/vm/ByteCodeTranslator/src/cn1_globals.m +++ b/vm/ByteCodeTranslator/src/cn1_globals.m @@ -1060,6 +1060,9 @@ static long cn1SatbTake(JAVA_OBJECT** out) { void cn1GcRegisterImmortalObj(JAVA_OBJECT o); static int cn1GcImmortalObjContains(JAVA_OBJECT o); static JAVA_BOOLEAN cn1SweepRemoving; +// Set ONLY around passes that iterate authoritative object registries +// (allObjectsInHeap, the BiBOP page registry). See the guard in gcMarkObject. +static int cn1GcTrustedRoots = 0; #endif void codenameOneGCMark() { @@ -1435,6 +1438,7 @@ void codenameOneGCMark() { memory_order_relaxed); #endif int gn = atomic_load_explicit(&gp->bumpIndex, memory_order_acquire); + cn1GcTrustedRoots = 1; // page-slot walk: authoritative references for(int gi = 0 ; gi < gn ; gi++) { JAVA_OBJECT go = cn1BibopSlot(gp, gi); if(__atomic_load_n(&go->__codenameOneGcMark, __ATOMIC_ACQUIRE) == -1 @@ -1443,6 +1447,7 @@ void codenameOneGCMark() { gcMarkObject(d, go, JAVA_FALSE); } } + cn1GcTrustedRoots = 0; gp = atomic_load_explicit(&gp->nextAll, memory_order_acquire); } gcMarkDrain(d); @@ -1469,6 +1474,7 @@ void codenameOneGCMark() { // already walks in full, and only fresh entries are traced. #ifndef CN1_DISABLE_LEGACY_GRACE { + cn1GcTrustedRoots = 1; // walking allObjectsInHeap: authoritative references #ifdef CN1_GC_VERIFY { extern const char* cn1GcMarkPhase; cn1GcMarkPhase = "legacy-grace-pass"; } #endif @@ -1482,6 +1488,7 @@ void codenameOneGCMark() { } } gcMarkDrain(d); + cn1GcTrustedRoots = 0; } #endif /* CN1_DISABLE_LEGACY_GRACE -- A/B escape hatch, mirrors CN1_DISABLE_SATB */ @@ -2258,7 +2265,9 @@ void cn1BibopBeginGcCycle(void) { // still sees the old latch and skips, so the fresh latch can never be // consumed by bytes just charged to the cycle that is starting. (void)atomic_exchange_explicit(&cn1LegacyGcScheduled, 0, memory_order_acq_rel); -#ifdef CN1_GRACE_AUDIT + // Snapshot every page's bump cursor at mark start. No longer QA-only: the + // late-grace re-mark uses it to visit ONLY slots born during this mark + // (issue 5425), instead of every fresh slot on every page. // QA builds only: snapshot every page's cursor at mark start. Slots below the // snapshot existed before the grace pass ran, so a complete grace pass must // have traced every one of them that is still fresh at pre-sweep time. @@ -2280,7 +2289,6 @@ void cn1BibopBeginGcCycle(void) { ap = atomic_load_explicit(&ap->nextAll, memory_order_acquire); } } -#endif } // Raw 64KB page memory comes from large arenas -- one posix_memalign per @@ -5251,7 +5259,17 @@ void gcMarkObject(CODENAME_ONE_THREAD_STATE, JAVA_OBJECT obj, JAVA_BOOLEAN force // Cost: one page/extent binary search per mark call, on the GC thread / // workers only (the mutator never calls gcMarkObject) -- GC-side passes // measured ~0% of wall time on this workload class. - if(cn1ConservativeResolve((void*)obj) != obj && !cn1GcImmortalObjContains(obj)) { + // TRUSTED CALLERS BYPASS THIS (issue 5425). The guard's job is to reject + // CONSERVATIVELY DERIVED pointers -- arbitrary machine words off a native stack + // that may not be objects at all. A caller that walked the object out of + // allObjectsInHeap or a BiBOP page slot has an authoritative reference and needs + // no proof; making it pass anyway means every object allocated after the cycle's + // extent snapshot is silently dropped, because the snapshot cannot contain it. + // That is exactly how a fresh object came to be grace-promoted by the sweep with + // its subtree never traced. Do NOT set this flag around anything that marks from + // a stack scan or a register file. + if(!cn1GcTrustedRoots + && cn1ConservativeResolve((void*)obj) != obj && !cn1GcImmortalObjContains(obj)) { return; } #endif From 73430b937b693673b6ac748d23db7e649828493c Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Mon, 27 Jul 2026 21:07:28 +0300 Subject: [PATCH 2/3] Address review: trust the registry walk, never the drain cn1GcTrustedRoots spanned gcMarkDrain() in the legacy grace pass. The drain follows child words out of arbitrary generated mark functions -- including those of DEAD objects kept alive by a conservative native-stack false positive, whose reference fields can dangle into freed or unmapped memory. Bypassing cn1ConservativeResolve for those is exactly the SIGSEGV the guard exists to prevent, so the change reopened the crash it was meant to be safe around. Trust covers only the registry walk, which is where the authoritative references are. Every fresh entry is stamped and queued by then, so the drain needs none. The BiBOP half already cleared per-page before its drain; the legacy half now matches. Also from review: - Acquire-load the mark word in the legacy grace walk before dereferencing the header. The mark word is the publication point (see gcMarkObject); a plain read can let a weak-memory target observe the object without the preceding parentClsReference store. Pre-existing, but the resolve guard had been backstopping it for this population. - Revert gcAuditSnapshot to CN1_GRACE_AUDIT-only. Promoting it was leftover from a narrowing that no longer exists: an O(#pages) walk plus atomics in every build for a field only read under that flag, with a stale comment referring to a pass that was deleted. Re-validated: reproducer clean 3/3 over 149 collection cycles and 732M verified references; run-gc-verify.sh GREEN over all nine drivers with both fault self-tests firing; GcStress checksum unchanged. Co-Authored-By: Claude Opus 5 (1M context) --- vm/ByteCodeTranslator/src/cn1_globals.h | 12 +++--------- vm/ByteCodeTranslator/src/cn1_globals.m | 18 +++++++++++++----- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/vm/ByteCodeTranslator/src/cn1_globals.h b/vm/ByteCodeTranslator/src/cn1_globals.h index 5c74358d47e..77ff2afdec9 100644 --- a/vm/ByteCodeTranslator/src/cn1_globals.h +++ b/vm/ByteCodeTranslator/src/cn1_globals.h @@ -1343,19 +1343,13 @@ typedef struct CN1BibopPage { // idempotent across parallel markers) int gcGraceEpoch; // upper bound on survivor epochs as of the last // full walk (GC-thread only) - int gcAuditSnapshot; // bumpIndex at mark start. Slots at or above - // it were born DURING the mark, which is the - // only population the late-grace re-mark has - // to trace (issue 5425); scanning every fresh - // slot instead is O(all slots) per cycle and - // measured a 2.1x objectAllocation regression. - // Was QA-only when it served -DCN1_GRACE_AUDIT - // alone. - // Relaxed __atomic access everywhere -- the +#ifdef CN1_GRACE_AUDIT + int gcAuditSnapshot; // QA builds only: bumpIndex at mark start. // Relaxed __atomic access everywhere -- the // GC writes/reads it during marking while a // mutator can reformat the page from the // FREE pool +#endif } CN1BibopPage; // Per-thread current page per size class; defined in cn1_globals.m. Touched only diff --git a/vm/ByteCodeTranslator/src/cn1_globals.m b/vm/ByteCodeTranslator/src/cn1_globals.m index d4dd1da5667..c5abce15c6e 100644 --- a/vm/ByteCodeTranslator/src/cn1_globals.m +++ b/vm/ByteCodeTranslator/src/cn1_globals.m @@ -1481,14 +1481,23 @@ void codenameOneGCMark() { int gt = currentSizeOfAllObjectsInHeap; for(int gi = 0 ; gi < gt ; gi++) { JAVA_OBJECT go = allObjectsInHeap[gi]; - if(go != JAVA_NULL && go->__codenameOneGcMark == -1 + // ACQUIRE: the mark word is the publication point (see gcMarkObject); + // reading it plainly can let a weak-memory target observe the object + // without the preceding parentClsReference store. + if(go != JAVA_NULL + && __atomic_load_n(&go->__codenameOneGcMark, __ATOMIC_ACQUIRE) == -1 && go->__codenameOneParentClsReference != 0 && go->__codenameOneParentClsReference->markFunction != 0) { gcMarkObject(d, go, JAVA_FALSE); } } - gcMarkDrain(d); + // Trust covers ONLY the registry walk above. Every fresh entry is already + // stamped and queued, so the drain needs no trust -- and must not have it: + // it follows child words out of arbitrary mark functions, including those + // of dead objects kept alive by a conservative native-stack false positive, + // whose fields can dangle. That is precisely what the guard exists to stop. cn1GcTrustedRoots = 0; + gcMarkDrain(d); } #endif /* CN1_DISABLE_LEGACY_GRACE -- A/B escape hatch, mirrors CN1_DISABLE_SATB */ @@ -2265,9 +2274,7 @@ void cn1BibopBeginGcCycle(void) { // still sees the old latch and skips, so the fresh latch can never be // consumed by bytes just charged to the cycle that is starting. (void)atomic_exchange_explicit(&cn1LegacyGcScheduled, 0, memory_order_acq_rel); - // Snapshot every page's bump cursor at mark start. No longer QA-only: the - // late-grace re-mark uses it to visit ONLY slots born during this mark - // (issue 5425), instead of every fresh slot on every page. +#ifdef CN1_GRACE_AUDIT // QA builds only: snapshot every page's cursor at mark start. Slots below the // snapshot existed before the grace pass ran, so a complete grace pass must // have traced every one of them that is still fresh at pre-sweep time. @@ -2289,6 +2296,7 @@ void cn1BibopBeginGcCycle(void) { ap = atomic_load_explicit(&ap->nextAll, memory_order_acquire); } } +#endif } // Raw 64KB page memory comes from large arenas -- one posix_memalign per From d40371068996e9d52e2502f6560b11c69feed063 Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Tue, 28 Jul 2026 04:13:34 +0300 Subject: [PATCH 3/3] Address review: make the trusted-roots scope per-thread and nesting-safe cn1GcTrustedRoots was a process-wide global. Marking can run as a worker pool (gcMarkDrainParallel), so a concurrent worker would also bypass the conservative-resolve guard while a grace pass held the flag -- on pointers that are NOT authoritative, which is the precise case the guard exists for. It is safe today only because gcMarkResolveThreadCount() returns 1, and the comment there says that serial default is a temporary isolation experiment: the hazard would arm itself the moment parallel marking is restored, and would look unrelated when it fired. Make it __thread. Manual set/unset also is not nesting-safe -- the BiBOP half toggles it per page inside a loop, so an early return or a future nested trusted scope would leak a trusted window into unrelated marking. CN1_GC_TRUSTED_BEGIN/END save and restore the previous value instead of clearing to zero. Re-validated: reproducer clean 2/2 over 102 collection cycles and 492M verified references; run-gc-verify.sh GREEN over all nine drivers with both fault self-tests firing; GcStress checksum unchanged, including a -DCN1_GC_MARK_THREADS=4 build -- the parallel configuration this is about. Co-Authored-By: Claude Opus 5 (1M context) --- vm/ByteCodeTranslator/src/cn1_globals.m | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/vm/ByteCodeTranslator/src/cn1_globals.m b/vm/ByteCodeTranslator/src/cn1_globals.m index c5abce15c6e..c399435918b 100644 --- a/vm/ByteCodeTranslator/src/cn1_globals.m +++ b/vm/ByteCodeTranslator/src/cn1_globals.m @@ -1062,7 +1062,21 @@ static long cn1SatbTake(JAVA_OBJECT** out) { static JAVA_BOOLEAN cn1SweepRemoving; // Set ONLY around passes that iterate authoritative object registries // (allObjectsInHeap, the BiBOP page registry). See the guard in gcMarkObject. -static int cn1GcTrustedRoots = 0; +// +// PER THREAD, deliberately. Marking can run as a worker pool +// (gcMarkDrainParallel); a process-wide flag would let a CONCURRENT worker bypass +// the resolve guard on pointers that are not authoritative -- the precise case the +// guard exists for. Serial marking is the current default +// (gcMarkResolveThreadCount returns 1), so a global would happen to be safe today +// and would silently stop being safe the moment parallel marking is re-enabled, +// which the isolation comment there says is intended. +// +// Use CN1_GC_TRUSTED_BEGIN/END rather than assigning directly: they save and +// restore, so nesting and any future early return cannot leak a trusted window +// into unrelated marking. +static __thread int cn1GcTrustedRoots = 0; +#define CN1_GC_TRUSTED_BEGIN() int __cn1TrustSaved = cn1GcTrustedRoots; cn1GcTrustedRoots = 1 +#define CN1_GC_TRUSTED_END() cn1GcTrustedRoots = __cn1TrustSaved #endif void codenameOneGCMark() { @@ -1438,7 +1452,7 @@ void codenameOneGCMark() { memory_order_relaxed); #endif int gn = atomic_load_explicit(&gp->bumpIndex, memory_order_acquire); - cn1GcTrustedRoots = 1; // page-slot walk: authoritative references + CN1_GC_TRUSTED_BEGIN(); // page-slot walk: authoritative references for(int gi = 0 ; gi < gn ; gi++) { JAVA_OBJECT go = cn1BibopSlot(gp, gi); if(__atomic_load_n(&go->__codenameOneGcMark, __ATOMIC_ACQUIRE) == -1 @@ -1447,7 +1461,7 @@ void codenameOneGCMark() { gcMarkObject(d, go, JAVA_FALSE); } } - cn1GcTrustedRoots = 0; + CN1_GC_TRUSTED_END(); gp = atomic_load_explicit(&gp->nextAll, memory_order_acquire); } gcMarkDrain(d); @@ -1474,7 +1488,7 @@ void codenameOneGCMark() { // already walks in full, and only fresh entries are traced. #ifndef CN1_DISABLE_LEGACY_GRACE { - cn1GcTrustedRoots = 1; // walking allObjectsInHeap: authoritative references + CN1_GC_TRUSTED_BEGIN(); // walking allObjectsInHeap: authoritative references #ifdef CN1_GC_VERIFY { extern const char* cn1GcMarkPhase; cn1GcMarkPhase = "legacy-grace-pass"; } #endif @@ -1496,7 +1510,7 @@ void codenameOneGCMark() { // it follows child words out of arbitrary mark functions, including those // of dead objects kept alive by a conservative native-stack false positive, // whose fields can dangle. That is precisely what the guard exists to stop. - cn1GcTrustedRoots = 0; + CN1_GC_TRUSTED_END(); gcMarkDrain(d); } #endif /* CN1_DISABLE_LEGACY_GRACE -- A/B escape hatch, mirrors CN1_DISABLE_SATB */