Repository navigation
Fix three correctness bugs: corrupt-region crashes, fork safety, string serialization - #54
Merged
Merged
Conversation
Corrupt MORH/MORS root headers and MORL directory entries with inflated or negative lengths passed map_shared() and segfaulted on first read. mori_open_vector() and mori_open_string() now validate headers against the mapped size, mori_make_string() checks the offset table fits and records the packed-string area size, mori_string_elt_shm() bounds-checks each offset-table entry, and mori_unwrap_element() checks element length against data_size and rejects negative attrs_size.
A fork()ed child (e.g. parallel::mclapply) inherits the host extptr of a parent's shared object; when the child dropped the reference and GC ran, the inherited finalizer unlinked the parent's live region, so new consumers could no longer map it. Store the creator PID in mori_shm at create time and unlink only when it matches getpid(), mirroring the existing guard in mori_log_release(). Benchmarked at 20k share+finalize cycles: no measurable cost.
A materialized shared string vector and an SHM identifier were both emitted as a bare STRSXP state, so a materialized length-1 string whose content parsed as an identifier was misrouted on unserialize — returning the named region's contents, or erroring when the region was gone. Materialized string states are now wrapped in a length-1 VECSXP, so a bare STRSXP state is always an identifier; the string class gets its own mori_string_Unserialize, and the shared mori_Unserialize errors on a STRSXP state that fails to open instead of falling through. The compact identifier form is unchanged.
Three MORL tests crafted 56-byte regions whose directory entries claimed data at [56, 64), so the offset bounds check fired before the length/attrs/string-table checks they meant to exercise — and on Linux the string-table test failed, expecting 'invalid string data' but getting the offset check's 'invalid element data'. Pad those regions to 64 bytes so the intended checks fire, and use length = 0 in the vector-attrs test so its attrs_size check fires. Layouts verified against shm_open-crafted equivalents on macOS.
mori_shm_name() called getpid() for the name and mori_shm_create() called it again for shm->pid; take the PID as a parameter instead so create fetches it once. This also lets the Windows create path initialize shm->pid from GetCurrentProcessId() (and 0 for consumers in mori_shm_open), so the field is defined on all four create/open paths and mori_shm_wrap_producer()'s memcpy never copies uninitialized bytes. The value is still read only on POSIX, where the host finalizer's fork guard lives.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes three independent bugs found in a code review, one commit per fix.
1. Corrupted shared memory regions could crash R.
map_shared()checked a region's magic bytes and directory offsets, but never checked that claimed vector and string lengths fit inside the mapped region. A corrupt region with an inflated length field made R read out of bounds and segfault. All header and directory fields are now validated against the mapped size, so corrupt input always produces a clean R error.2. A forked child could destroy its parent's live regions. With
parallel::mclapply(or anyfork()), if the child garbage-collected an inherited shared object, the child's finalizer would unlink the parent's region — existing readers kept working, but no new process could map it. The region now records its creator's PID and only that process may unlink it. Benchmarked at 20k share+finalize cycles: no measurable performance cost.3. A shared string could be mistaken for its own identifier. Shared objects serialize compactly as their shared-memory name, but a materialized character vector was also serialized as a plain string — so a length-1 shared string whose content looked like a name (e.g. a stored
shared_name()value) would unserialize to the wrong object, or fail with "region not found". Materialized string states now use a wrapped form that can never be confused with an identifier. The compact identifier form is unchanged.Testing