Post-v0.50 review fixes: rv_clone_iterative cycle/CV leaves, blessed terminator, sandboxed-macOS test#3
Closed
atoomic wants to merge 43 commits into
Closed
Post-v0.50 review fixes: rv_clone_iterative cycle/CV leaves, blessed terminator, sandboxed-macOS test#3atoomic wants to merge 43 commits into
atoomic wants to merge 43 commits into
Conversation
Set COPYFILE_DISABLE=1 before tar in the dist target so that macOS extended attributes (com.apple.provenance, com.apple.lastuseddate, etc.) are not embedded in the CPAN tarball. These attributes cause tar errors on non-Apple systems (NetBSD, Linux) that do not understand them. COPYFILE_DISABLE=1 is macOS-specific and is a no-op on Linux/Windows. Fixes garu#85 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add AI_POLICY.md describing how AI tools are used in the project's development workflow. The policy establishes the principle that AI assists but humans decide — maintainers remain responsible for every review, merge, and release. Also update MANIFEST to include the new file and MANIFEST.SKIP to exclude dist tarballs (Clone-*.gz) and the local/ directory. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fix garu#82: Cloned DBI handles lack XS-level magic, causing DBI's DESTROY to dump pages of SV internals to STDERR. Circular refs inside the cloned handle tree also cause SEGVs during global destruction. The previous test 16 ($cloned->prepare) segfaulted on most platforms. Fix by running DBI clone tests in forked subprocesses: - Child redirects STDERR to /dev/null (suppresses all DBI noise) - Child uses POSIX::_exit(0) to skip destructors (avoids SEGV) - Child communicates results via pipe as key=value pairs - Parent relays results to Test::More assertions Also replace test 16's unsafe $cloned->prepare() call with a safe Scalar::Util::reftype structural check, and add $sth->finish before $dbh->disconnect to eliminate the "disconnect invalidates active statement handle" warning. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Refs garu#82: The DBI clone tests use fork() + pipe to isolate cloned DBI handle destruction (which causes SEGVs and STDERR noise). Strawberry Perl on Windows emulates fork() via ithreads which does not reliably support pipe+fork, causing "planned 18, ran 12" failures in CI. Add _can_fork() guard that returns false on MSWin32 and skips the 6 DBI tests with a clear message instead of dying with a bad plan count. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The file defines its own ok() sub (line 58) that returns undef instead of the test result. This caused the `or warn` on line 133 to always fire, printing "(?^:abcdefg) vs (?^:abcdefg)" to STDERR even when the test passed. Remove the dead diagnostic. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update version timestamps in Changes to match the actual git tag commit times for v0.32+. Normalize formatting to consistent single-space separators, remove double spaces and tab indentation, and convert old non-ISO dates (0.06, 0.01) to YYYY-MM-DD format. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
COPYFILE_DISABLE=1 (added for issue garu#85) prevents macOS tar from embedding ._AppleDouble sidecar files, but does not prevent it from writing PAX extended headers for xattrs that already exist on the files themselves (e.g. com.apple.provenance, which macOS sets on every file in a git-cloned repo). Add a PREOP step that runs 'xattr -cr $(DISTVNAME)' to strip all extended attributes from the distribution directory before tar is invoked. xattr(1) is macOS-specific; the '|| true' makes it a silent no-op on Linux and Windows. Fixes garu#91 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The documentation claimed 32,000 levels deep which hasn't been true since MAX_DEPTH was lowered to prevent SEGV on CPAN smokers. Update Clone.pm POD, README.md, and CLAUDE.md to reflect actual limits: 4000 rdepth units (~2000 nesting levels) on Linux/macOS, 2000 rdepth units (~1000 nesting levels) on Windows/Cygwin. Also document the iterative fallback for arrays and the shallow-copy warning for other types. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The switch cases for PERL_MAGIC_taint ('t'), PERL_MAGIC_backref ('<'),
and PERL_MAGIC_arylen_p ('@') each executed `continue` (resuming the
outer magic iteration for-loop) followed by a `break` that could never
be reached. Remove the dead `break` and add a comment clarifying that
`continue` resumes the outer loop, not the switch.
Fixes garu#97
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The `if (magic_ref) { ;; }` block was a confusing no-op whose empty body
read as a forgotten implementation. Replace with `if (!magic_ref) { ... }`
wrapping the HV/AV/RV branches, and add a comment explaining that tied
HV/AV skips direct element iteration because the tie magic handles the data.
Fixes garu#96
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
COPYFILE_DISABLE=1 (added for issue garu#85) prevents macOS tar from embedding ._AppleDouble sidecar files, but does not prevent it from writing PAX extended headers for xattrs that already exist on the files themselves (e.g. com.apple.provenance, which macOS sets on every file in a git-cloned repo). Add a PREOP step that runs 'xattr -cr $(DISTVNAME)' to strip all extended attributes from the distribution directory before tar is invoked. xattr(1) is macOS-specific; the '|| true' makes it a silent no-op on Linux and Windows. Fixes garu#91 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When rdepth exceeded MAX_DEPTH, the guard in sv_clone handled SVt_PVAV and RV->AV iteratively but fell through to SvREFCNT_inc(ref) for all other types, silently aliasing hash nodes into the clone. This broke the core contract of clone() for deeply nested hash structures. Fix by adding hv_clone_iterative() (mirroring av_clone_iterative) and routing SVt_PVHV and RV->HV through it in the rdepth > MAX_DEPTH block. The new function creates a fresh HV, registers it in hseen for circular- reference safety, and clones each value via sv_clone — which will re-enter the iterative path for any further nested HVs still above MAX_DEPTH. Add six new tests in t/10-deep_recursion.t covering: - deep hash structures at rdepth > MAX_DEPTH - clone-independence verification at depth 2500 (past the guard) - mixed array/hash structures past MAX_DEPTH Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Reduced `$deep_target` on Windows/Cygwin from 4000 to 2500 in `t/10-deep_recursion.t` to fix Windows CI stack overflow. Hash structures (HV) are significantly heavier per nesting level than arrays during Perl's recursive `SvREFCNT_dec` cleanup, causing stack overflow on Windows' 1MB default thread stack. The new value (2500) still exceeds `MAX_DEPTH` (2000 on Windows), ensuring the iterative clone path is exercised. The clone-independence check depth (`$depth_target = 1500`) remains well within range.
Fix garu#90 Add a security policy based on the CPAN Security Group's guidelines (v1.3.0) and modeled after WWW::Mechanize's SECURITY.md. Covers how to report vulnerabilities via GitHub's advisory form, response expectations, scope of the policy, and supported Perl versions.
Need to enable the github settings.
The "visible = 1" workaround was added 20+ years ago for Perl 5.9.x when mg_find(ref, '<') stopped detecting backref magic reliably. The root cause: Perl 5.10 moved HV weakref backrefs from PERL_MAGIC_backref to the HV's AUX struct (SvOOK), so SvMAGICAL alone misses HV targets. Replace with a precise condition that tracks SVs in hseen only when needed: shared SVs (refcount > 1), magical SVs (tied, non-HV weakref targets), or HVs with AUX data (SvOOK — covers HV weakref targets). Unique leaf scalars skip the hash lookup/store entirely. Add t/20-shared-sv.t with 31 tests covering shared refs, weak refs on HV/AV/scalar targets, and topology preservation after cloning. Co-Authored-By: Claude <noreply@anthropic.com>
When cloning deeply nested scalar references past MAX_DEPTH, Clone silently returns a shared reference (SvREFCNT_inc) instead of a deep copy. This adds a Perl_warn so callers know the clone is incomplete. Also corrects the LIMITATIONS pod: hashes now have an iterative fallback (added in GH garu#93), so only non-container types fall back to shared references. Co-Authored-By: Claude <noreply@anthropic.com>
Here's a summary of the changes: - **Added `$Clone::WARN` package variable** (defaults to `1`) in `Clone.pm` to let callers suppress the depth-limit warning by setting `$Clone::WARN = 0`, per reviewer request for a mechanism to disable warnings once acknowledged - **Made `Perl_warn()` conditional** in `Clone.xs`: checks `$Clone::WARN` before emitting the shallow-copy fallback warning; if the variable is false or set to 0, the warning is suppressed - **Documented `$Clone::WARN`** in the LIMITATIONS pod section showing `$Clone::WARN = 0` to silence the warning - **Added test 17** verifying that `local $Clone::WARN = 0` suppresses depth-limit warnings (test count bumped from 16 to 17)
) Tests 15-17 verify that cloning a deeply nested scalar-ref chain past MAX_DEPTH produces a genuine deep copy instead of a shared alias. They FAIL before the fix: the old code returns SvREFCNT_inc(ref) which aliases back into the original object. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…aru#107) Previously, when rdepth exceeded MAX_DEPTH for an RV that did not point to an AV or HV (e.g. a scalar-ref chain \\...\$val), sv_clone() fell back to SvREFCNT_inc(ref), returning the original SV as the "clone". This silently violated isolation: writes through the cloned reference would mutate the original object. The warning was suppressable via $Clone::WARN = 0, so production code could never rely on it. Fix: add rv_clone_iterative(), which walks the RV->RV->...->leaf chain without recursion (analogous to av_clone_iterative / hv_clone_iterative), clones the leaf, and rebuilds the reference chain from the bottom up. The MAX_DEPTH guard in sv_clone() now calls rv_clone_iterative() for any RV type instead of falling back to a shared alias. Non-RV, non-AV, non-HV SVs past MAX_DEPTH (PVGV, PVCV, etc.) are not deep-copyable regardless of depth and still fall through to the existing SvREFCNT_inc path with a warning. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…#106) The chain-walking loop in av_clone_iterative() never consulted hseen, so a circular AV (e.g. $a = [$a]) placed at the bottom of a 2001+ level deep wrapper chain caused an infinite loop and unbounded memory allocation when sv_clone() switched to iterative mode. Add a CLONE_FETCH guard at the top of the loop body: if inner_sv is already in hseen, link tail to the existing clone via newRV_inc and break out of the loop. This mirrors the guard that exists at the function's entry point and on the standard recursive sv_clone path. Adds t/21-circular-iterative.t with five tests covering: - no hang/die on deep circular arrays past MAX_DEPTH - circular reference is preserved in the clone - clone is independent from the original - edge case at exactly MAX_DEPTH+1 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When cloning an SV whose magic has mg_len == HEf_SVKEY, Clone.xs was calling SvREFCNT_inc(mg->mg_ptr) manually before passing mg_ptr to sv_magic(). sv_magic() already calls SvREFCNT_inc internally for HEf_SVKEY, so the manual increment was redundant and produced a permanent refcount leak (one increment was never matched by a decrement on cleanup). Remove the manual SvREFCNT_inc and let sv_magic() take sole ownership of the reference, matching Perl's own convention for this flag. Add t/21-hefsvkey-refcnt.t to exercise PVLV tied-element proxies (the most common carrier of HEf_SVKEY magic) and verify refcount stability across clone/destroy cycles. Direct pure-Perl reproduction of the exact buggy path is not possible on modern Perl (the GH garu#42 PVLV early-exit prevents it); the fix is verified by code inspection and the test guards the related invariants. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The LIMITATIONS section in Clone.pm incorrectly stated that deeply nested scalar references would be shared instead of deep-copied. Since the addition of rv_clone_iterative (GH garu#107), all reference types are handled by the iterative fallback. Update the docs to reflect the current behavior. Also fix the filehandle description — IO objects are shared by reference (SvREFCNT_inc), not deep-copied with a shared fd. Renumber t/21-hefsvkey-refcnt.t to t/22-hefsvkey-refcnt.t to fix duplicate test numbering (t/21-circular-iterative.t already uses 21). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The clone() XSUB allocates hseen (HV) and weakrefs (AV) in PREINIT, but only frees them at the end of PPCODE. If sv_clone() triggers croak() — via unknown SV type, CLONE_STORE failure, or unsupported magic_ptr — the longjmp skips the explicit hv_clear/SvREFCNT_dec calls, leaking both containers and all their contents. Switch to SAVEFREESV(), which registers both SVs for automatic cleanup on scope exit. Perl's stack unwinding invokes these during croak, preventing the leak. On the normal (non-croak) path, cleanup happens at scope exit just as before. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When cloning SVs with magic that has mg_len >= 0 and non-null mg_ptr (e.g. vstring magic 'V'), the code allocated an intermediate buffer via Newxz, copied the data, then passed it to sv_magic(). However, sv_magic() with non-negative namlen calls savepvn() internally to make its own copy — the intermediate buffer was never freed. Over 200K clone() calls of a vstring, this leaked ~3 MB. The fix passes mg->mg_ptr directly to sv_magic() and lets it handle the copy. Note: the mg_len == -1 paths (utf8 cache, HEf_SVKEY) are unaffected — sv_magic() stores those pointers directly without copying, so the explicit allocation in those paths is correct and necessary. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…#122) Under MacPorts the build user has no valid shell configured, so backtick `ps -o rss= -p $$` fails with "Can't exec 'ps': Operation not permitted" and pollutes the test output (the SKIP still fires, but the noise is confusing). Replace the backtick call with a list-form open() using the absolute path /bin/ps. This bypasses the shell entirely, so the invocation works regardless of the user's shell setting, and any exec failure is caught silently via the `or return undef` branch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Here's what I changed and why: - **Added `macos-latest` CI job** to `.github/workflows/test.yml` per reviewer request from @atoomic to add macOS to the CI workflow. The job runs on `macos-latest`, uses system Perl (which has threads support), installs test dependencies via cpm, and runs the full build+test cycle. This ensures macOS-specific issues like the `/bin/ps` shell invocation problem (GH garu#122) are caught in CI going forward.
When cloning structures deeper than MAX_DEPTH/2, the iterative fallback used SvREFCNT_inc for all non-container types, including simple scalars. This aliased leaf strings and integers between original and clone, causing mutations through a reference to the clone's leaf value to corrupt the original. Add a type switch before the SvREFCNT_inc fallback: scalar types (SVt_NULL through SVt_PVMG) now get newSVsv() for safe, independent copying. Non-clonable types (PVGV, PVCV, etc.) continue to use SvREFCNT_inc with warnings. Closes garu#109.
…aru#116) The iterative clone path for deeply nested single-element arrays ([[[...]]]) created new AVs via newAV() without checking SvOBJECT on the original. Blessed arrayrefs past MAX_DEPTH/2 nesting levels silently lost their class. Fix: bless the clone AV via sv_bless() when the original is an object. Co-Authored-By: Claude <noreply@anthropic.com>
Summary of changes: - **Renumbered test file `t/22-iterative-bless.t` → `t/23-iterative-bless.t`** to resolve numbering collision with `t/22-hefsvkey-refcnt.t` and `t/22-mgptr-memleak.t` that landed on master during the rebase. The project convention requires sequential test numbering (`t/NN-descriptive-name.t`). - **Updated MANIFEST** to reflect the new filename and corrected sort order. No review comments were present to address — the only change is fixing the test numbering collision introduced by the rebase.
MacPorts runs tests in a sandbox where even direct exec() of /bin/ps is blocked (EPERM). The child process still prints "Can't exec" to STDERR before dying, polluting test output even though the test already skips gracefully. Fix: redirect STDERR to /dev/null before the fork via `open(local *STDERR, '>', '/dev/null')`. The child inherits the redirected fd 2 so any exec- failure message is silently discarded. Perl's 'local' mechanism restores STDERR when the function returns. Fixes garu#122 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The rdepth > MAX_DEPTH block had three separate RV cases (RV-to-AV, RV-to-HV, RV-to-scalar) that duplicated logic already present in rv_clone_iterative. The first two cases also lacked SvWEAKREF handling, silently converting weak references into strong ones when cloning past MAX_DEPTH. Consolidate all three into a single rv_clone_iterative call, which already handles AV/HV referent dispatch, blessing preservation, and weakref propagation. This removes ~12 lines of duplicated code and fixes the weakref loss bug. Add tests verifying weakref preservation on deeply nested structures past MAX_DEPTH. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Ubuntu 20.04 (Focal) reached end-of-life in April 2025. Its package mirrors are increasingly unreliable, causing transient apt-get failures that produce noisy CI results unrelated to Clone itself. Replace with Ubuntu 22.04 (Jammy), supported until April 2027.
The Windows CI step ran `prove -l t` before `perl Makefile.PL && gmake`, meaning tests either used a pre-installed Clone (not the current code) or failed silently in PowerShell. Also changed `-l` (lib/) to `-b` (blib/) since the compiled XS module lives in blib/arch/, not lib/. Separated the monolithic run block into distinct Build and Run Tests steps for clarity, and removed the redundant `gmake test` since prove already runs all tests with the GitHub annotations formatter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Three test files shared the t/22- prefix, violating the convention documented in CLAUDE.md (sequential numbering, NN-descriptive-name.t). The prior fix in a3736cf renumbered only t/22-iterative-bless.t to t/23, leaving three siblings still colliding. Renumbering by creation order: - t/22-hefsvkey-refcnt.t stays 22 (oldest, 2026-03-29) - t/22-mgptr-memleak.t -> t/23-mgptr-memleak.t - t/22-deep-leaf-isolation.t -> t/24-deep-leaf-isolation.t - t/23-iterative-bless.t -> t/25-iterative-bless.t MANIFEST updated to match. All 345 tests pass.
rv_clone_iterative walked the SvRV chain with no cycle detection. A scalar-
ref cycle reached only past MAX_DEPTH (so the recursive sv_clone path never
registered it in hseen) caused the chain[] buffer to double via Renew()
forever until allocation aborted ("Out of memory!").
Check hseen for each referent before pushing onto chain[]; if cached, use
the existing clone as the leaf and stop walking. The leaf-cloning branch
runs only when no cached referent was found.
Add t/26-rv-iterative-cycle.t with a 5000-RV chain that closes a back-edge
deep in the chain — reproduces the OOM before the fix.
When the bottom of an RV chain past MAX_DEPTH was a non-scalar SV that sv_setsv refuses to copy (PVCV, PVGV, PVFM, PVIO, PVLV, REGEXP/BM), rv_clone_iterative called newSVsv() which croaked "Bizarre copy of CODE in subroutine entry" or produced a stringified ref. The regular sv_clone() switch already shares these types via SvREFCNT_inc; mirror that behavior on the iterative path. Add t/27-rv-iterative-noncloneable.t: deep chains terminating in CODE, GLOB, and IO leaves now clone without dying.
av_clone_iterative()'s post-loop fallback for the chain terminator wrapped the cloned AV in newRV_noinc() without re-blessing. A blessed multi-element array at the bottom of a [[[...]]] chain therefore came back unblessed. Apply the same SvOBJECT / sv_bless pattern already used inside the chain-walk loop, on the wrapping RV produced for the terminator. Extend t/25-iterative-bless.t with a multi-element terminator case that fails before this change.
Commit a5dc9e6 hardened get_rss_kb() in t/12-memleak.t against sandboxed macOS builds (e.g. MacPorts builder with no valid shell) by using the list form of open and redirecting STDERR to /dev/null. The same helper exists verbatim in t/23-mgptr-memleak.t and was missed. Apply the same patch so the vstring memleak harness does not emit "Can't exec" noise on sandboxed macOS hosts.
Owner
Author
|
Superseded by upstream PR (re-targeting at garu/Clone). |
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
2 participants
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
Four fixes surfaced by an audit of changes since v0.50, each as its own commit, all with regression tests:
rv_clone_iterativecycle guard — a scalar-ref cycle reachable only past MAX_DEPTH made the chain walk loop forever (chain[] doubled via Renew until OOM). Now consults hseen for each referent before pushing. (fa78264)rv_clone_iterative— a deep RV chain ending in a CODE / IO / FM / PVLV leaf callednewSVsv()and croaked "Bizarre copy of CODE in subroutine entry". Mirror the regularsv_clone()switch: share viaSvREFCNT_incfor the non-cloneable types. (612b11b)av_clone_iterative()'s post-loop fallback wrapped the cloned AV innewRV_noinc()without re-blessing, so a blessed multi-element array at the bottom of a[[[...]]]chain came back unblessed. (abb19f1)t/23-mgptr-memleak.tsandboxed-macOS noise — thepshardening from a5dc9e6 int/12-memleak.twas missed int/23. Same patch applied. (a176e33)Reproduction (before)
Test plan
make testclean on darwin / Perl 5.38.4 (359 tests pass, was 353)t/26-rv-iterative-cycle.t(3 tests) reproduces the OOM before fix Duplicate Magic mg_ptr correctly #1; passes aftert/27-rv-iterative-noncloneable.t(4 tests) reproduces the CV/IO croak before fix fix: warn on shallow-clone fallback at MAX_DEPTH #2; passes aftert/25-iterative-bless.t(+2 tests) reproduces the blessing loss before fix Post-v0.50 review fixes: rv_clone_iterative cycle/CV leaves, blessed terminator, sandboxed-macOS test #3; passes aftercc upstream review when ready.