Skip to content

Release shared_mutex with unlock_shared() in GrowSurface loops (UB; breaks on macOS/libc++)#1077

Open
robert-kofler42 wants to merge 1 commit into
ScrollPrize:mainfrom
robert-kofler42:fix/growsurface-shared-mutex-unlock
Open

Release shared_mutex with unlock_shared() in GrowSurface loops (UB; breaks on macOS/libc++)#1077
robert-kofler42 wants to merge 1 commit into
ScrollPrize:mainfrom
robert-kofler42:fix/growsurface-shared-mutex-unlock

Conversation

@robert-kofler42

Copy link
Copy Markdown

Bug

optimize_surface_mapping and the grow loop in GrowSurface.cpp guard read-only critical sections with a std::shared_mutex acquired via lock_shared(), but release it with unlock() instead of unlock_shared():

  • GrowSurface.cpp:1307 lock_shared() → released by unlock() at :1309 and :1312
  • GrowSurface.cpp:2752 lock_shared() → released by unlock() at :2764

Per [thread.sharedmutex.requirements], a lock acquired with lock_shared() must be released with unlock_shared(). Calling unlock() while holding shared ownership is undefined behavior:

  • libstdc++ (Linux) happens to tolerate it (shared/exclusive state share an atomic), which is why it has gone unnoticed;
  • libc++ (macOS) tracks shared vs. exclusive ownership separately, so unlock() corrupts the lock state — and this repo targets Ubuntu + macOS (a hard portability requirement in AGENTS.md).

All other lock()/unlock() pairs on this mutex are correct exclusive locks and are left unchanged.

Fix

Change the three unlock() calls that pair with lock_shared() to unlock_shared() (3 lines). The exclusive write sections (lock()/unlock()) are untouched.

Verification

  • vc_core compiles clean.
  • Correctness is by the std::shared_mutex contract (standard-conformance fix), confirmed by inspecting every lock/unlock site on the mutex: after the change, the 2 lock_shared() sites are released only via unlock_shared() (3 release points across mutually-exclusive branches) and every lock() only via unlock().
  • The affected runtime path is the segmentation grower (vc_grow_seg_*), which requires scroll data to exercise end-to-end; this is a pure standard-conformance correction with no behavior change on libstdc++.

…e loops

optimize_surface_mapping and the grow loop take read critical sections with
std::shared_mutex::lock_shared() but released them with unlock() instead of
unlock_shared() (GrowSurface.cpp:1309, 1312, 2764).

Per [thread.sharedmutex.requirements], a lock acquired with lock_shared()
must be released with unlock_shared(); calling unlock() on a shared
ownership is undefined behavior. libstdc++ happens to tolerate it, but
libc++ (macOS) does not — it corrupts the lock state — and the repo targets
macOS + Ubuntu.

Change the three offending unlock() calls (the ones paired with
lock_shared()) to unlock_shared(). The exclusive lock()/unlock() pairs in
the same functions are correct and left unchanged.
@vercel

vercel Bot commented Jun 29, 2026

Copy link
Copy Markdown

Someone is attempting to deploy a commit to the scroll Team on Vercel.

A member of the Team first needs to authorize it.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant