fix!: Soundness of Zend API

[ptondereau]

Description

Why

Safe APIs let user code read past a string, keep a value the iterator already freed, hand the engine a pointer it misreads, or mutate a table the GC is walking. Nothing crashes on the happy path, so it lasted. Compiling without unsafe should mean something again.

How

Every change checked against php-src 8.1 to 8.5.

gc_collect_cycles() tripped HT_ASSERT_RC1. Not a refcount leak on $this returns: the GC reaches our get_properties through zend_std_get_gc, several times per cycle, while holding its own reference on the table we rewrite.

sequenceDiagram
    participant GC as collector
    participant H as get_properties (ours)
    participant T as obj->properties
    GC->>H: pass 1
    H->>T: merge Rust props (rc 1)
    GC->>T: hold ref (rc 2)
    GC->>H: pass 2
    H->>T: merge on shared table
    Note over T: assert on debug, corruption on release

Loading

Fix: own get_gc returning the engine storage untouched, so get_properties only runs for user-facing reads, where the engine duplicates before sharing.

The rest makes signatures honest:

• packed slice reads are unsafe; Zval::binary_slice checks length and alignment;
• iterators yield owned values, the engine frees the current one each step;
• Zval::set_ptr is unsafe, the engine reads IS_PTR as typed pointers;
• ZBox::into_raw returns a pointer, not &'static mut;
• ownerless into_raw constructors and a dead deprecated flag removed.

Alignment guard: no measurable cost on binary_slice_reads.

Migration: guide/src/migration-guides/v0.16.md.

Checklist

• I have read the contribution guidelines and the code of conduct.
• I have added tests that prove my code works as expected.
• I have added documentation if applicable.
• I have added a migration guide if applicable.

[coveralls]

Coverage Report for CI Build 34054483238

Coverage increased (+0.04%) to 68.667%

Details

• Coverage increased (+0.04%) from the base build.
• Patch coverage: 43 uncovered changes across 8 files (33 of 76 lines covered, 43.42%).
• 6 coverage regressions across 5 files.

Uncovered Changes

File	Changed	Covered	%
src/zend/handlers.rs	26	0	0.0%
src/builders/class.rs	4	0	0.0%
src/types/iterable.rs	3	0	0.0%
src/types/object.rs	3	0	0.0%
src/types/zval.rs	22	19	86.36%
src/enum_.rs	2	0	0.0%
src/alloc.rs	3	2	66.67%
src/types/class_object.rs	1	0	0.0%
Total (12 files)	76	33	43.42%

Coverage Regressions

6 previously-covered lines in 5 files lost coverage.

File	Lines Losing Coverage	Coverage
src/builders/class.rs	2	77.15%
src/types/iterable.rs	1	0.0%
src/types/object.rs	1	24.09%
src/zend/handlers.rs	1	0.0%
src/zend/ini_entry_def.rs	1	0.0%

---

Coverage Stats

Relevant Lines:	13647
Covered Lines:	9371
Line Coverage:	68.67%
Coverage Strength:	42.74 hits per line

---

💛 - Coveralls

[ptondereau]

@upsun-dispatch review

[codspeed-hq]

Merging this PR will not alter performance

✅ 27 untouched benchmarks
🆕 3 new benchmarks

Performance Changes

	Benchmark	BASE	HEAD	Efficiency
🆕	binary_slice_reads[1]	N/A	125.1 ms	N/A
🆕	binary_slice_reads[10]	N/A	125.1 ms	N/A
🆕	binary_slice_reads[100000]	N/A	324.3 ms	N/A

---

_{Comparing fix/v016-soundness-batch (2d47185) with master (d56f8dc)}

[upsun-dispatch]

Note

Reviewed — No blocking findings · 🔵 3 minor points

🔍 Full review · 33 files reviewed

🔵 Minor points

• src/zend/handlers.rs:72 — get_gc replaces the engine handler with a hand-written copy of zend_std_get_gc's standard-object branch only. Two consequences. (1) It is version-independent Rust standing in for an engine function that has changed across 8.1-8.5: on PHP 8.4+ a Rust-backed class can be made lazy (ZendObject::make_lazy / zend_object_make_lazy is wired up and is_lazy_proxy is tested), and a lazy object keeps its initializer closure (and, for a proxy, the real instance) in slots allocated past ce->default_properties_count; reporting only default_properties_count entries hides them from the collector, so a cycle through a lazy Rust object's initializer is never collected. (2) Any zval owned by the Rust struct itself (a #[php(prop)] pub f: Zval holding an object) is never reported, so those cycles leak silently. Neither case corrupts memory, but both are permanent leaks that the previous handler at least attempted to cover.
• src/zend/handlers.rs:305 — The COW separation calls zend_array_dup on the object's own property table. That table contains IS_INDIRECT entries pointing into obj->properties_table for every declared property (all #[php(prop)] fields are declared through zend_declare_property, and a Rust class extending Exception inherits message/code/file/line/trace slots). zend_array_dup resolves IS_INDIRECT when copying, so the table installed as (*object).properties is a snapshot detached from the slots: later engine writes to those declared properties go to the slot and are invisible to every later get_properties consumer (var_dump, (array), json_encode) and, because get_gc returns that same table with *n = 0, invisible to the GC as well. The trigger is obj->properties having refcount > 1 when the handler runs (a caller that shared the table rather than duplicating it, e.g. via zend_proptable_to_symtable fast path, then a second property fetch on the same object).
• src/zend/handlers.rs:299 — let ht = &*props; dereferences the result of zend_std_get_properties unconditionally. The previous code tolerated a null return (.as_mut().or_else(...)); the new code turns any null into an immediate null-reference deref inside an extern "C" handler. The SAFETY comment asserts non-null for a live object, but that is an assumption about engine internals across 8.1-8.5 rather than something checked here; a cheap if props.is_null() { return props; } keeps the guarantee local.

Verification

• Zval::binary_slice checks both the length multiple and val's alignment before the now-unsafe PackSlice::unpack_into, and the new unit tests cover the 7-byte rejection.
• The new get_gc matches zend_std_get_gc's standard branch shape: *table=NULL,*n=0 plus properties when set, else properties_table with ce->default_properties_count.
• Every ZBox::into_raw call site was converted to a raw-pointer deref ((*obj).std, (*raw).std, (*this).get_mut_zend_obj()), preserving the dec_count/set_object refcount balance.
• Pack::unpack_into now reads with read_unaligned, so dropping its alignment claim while marking only PackSlice::unpack_into unsafe is consistent.
• tests/sapi.rs keeps the SapiModule local alive (shadowed, not moved) until after cleanup_sapi_allocations, so removing Box::from_raw does not free the name strings early.
• The mago pin is bumped identically in .github/workflows/build.yml and flake.nix (1.47.6).

The diff adds unit tests for Zval::binary_slice (partial element, packed u64), a new PHP integration fixture tests/src/integration/binary_slice/binary_slice.php wired into tests/src/lib.rs, and GC/refcount assertions in class/class.php; these run under the Build and Test, Test with embed and test-asan jobs in .github/workflows/build.yml, and the Lint job runs clippy pedantic plus mago lint. Nothing in the diff exercises the new get_properties copy-on-write branch (no test makes obj->properties shared) or get_gc with a lazy object.

Review details

• Commit: 5b089bc
• Model: claude-opus-5

---

View the full run