fix: leak one refcount in ZBox<ZendClassObject<T>>::set_zval#735
Merged
Conversation
Coverage Report for CI Build 25676991317Coverage decreased (-0.01%) to 66.23%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
Bencher Report| Branch | fix/class-object-refcount-leak |
| Testbed | PHP 8.4.21 (cli) (built: May 8 2026 04:58:07) (NTS) |
⚠️ WARNING: Truncated view!The full continuous benchmarking report exceeds the maximum length allowed on this platform.
🐰 View full continuous benchmarking report in Bencher
⚠️ WARNING: No Threshold found!Without a Threshold, no Alerts will ever be generated.
ZBox::into_raw transfers ownership without touching the refcount, so the box's single ref carries into set_object. set_object then calls inc_count, bumping the count to 2. The zval drops one ref on destruction; the object never reaches zero and leaks. On a PHP debug build the leak cascades into an assertion in _zend_hash_str_add_or_update_i at module shutdown: leaked instances keep shared class storage (used by classes with #[php(prop)] fields) alive past its expected lifetime, and property cleanup hits a frozen hashtable without the HASH_FLAG_ALLOW_COW_VIOLATION bit. Apply the same fix already present on the sibling impl ZBox<ZendObject>::set_zval in object.rs: call dec_count before into_raw so the net post-insertion count stays at 1. The matching impl for RegisteredEnum + RegisteredClass in enum_.rs had the same bug and is fixed alongside.
ptondereau
force-pushed
the
fix/class-object-refcount-leak
branch
from
e8514a6 to
e86975a
Compare
Adds an integration test that exercises the exact buggy code path fixed in the previous commit: a class extending \Exception with a #[php(prop)] field, constructed via ZendClassObject::new(...) .into_zval(...) and thrown via PhpException::with_object. The caught exception's refcount is read inside the catch block via debug_zval_dump; with the fix the only holder is the catch-bound variable, so the dump reports refcount 2. Without the fix the buggy set_zval keeps an extra ref and the dump reports 3. Refcount via debug_zval_dump was chosen over a memory-growth measurement so the same assertion fires on release and debug PHP builds. memory_get_usage / gc_collect_cycles interact poorly with a separate latent refcount bug in the IntoZval impl for &mut ZendClassObject<T> (out of scope here): on debug builds GC tries to add to a shared HT and trips _zend_hash_str_add_or_update_i. Verified against PHP 8.4.20 NTS and 8.4.20 ZTS-DEBUG: passes with the fix, fails (refcount 3) without it.
ptondereau
force-pushed
the
fix/class-object-refcount-leak
branch
from
e86975a to
71ea067
Compare
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.
Description
This one surfaced from biscuit-php while refactoring an exception type with
#[php(prop)]fields thrown viawith_object. Tests stayed green, but PHP 8.5.2-dev printed_zend_hash_str_add_or_update_iat shutdown, easy to read as harmless debug-build noise. Bisecting (#[php(prop)]on or off,ZendClassObject::newvsfrom_class) narrowed it to theset_zvalimpl onZBox<ZendClassObject<T>>. The sibling implZBox<ZendObject>::set_zvalinsrc/types/object.rsalready carried thedec_countworkaround, with a comment from the original author flagging it, the class-object impl was the one that never got the same treatment.While returning a
ZendClassObject<T>from Rust to PHP throughinto_zval, the box's only reference was being preserved byinto_rawand then a second one was being added byset_object, leaving the object stuck at refcount 2. The zval would later release one, but the count never reached zero. Every exception built that way, every class object handed back from a#[php(prop)]getter, every PHP-visible value coming out of that path leaked a copy on shutdown. On a debug PHP build the leak then propagated into shared class storage and tripped_zend_hash_str_add_or_update_ion module shutdown; on release builds it was just silent bytes piling up request after request.The fix mirrors what
ZBox<ZendObject>::set_zvalinsrc/types/object.rshas been doing all along: drop one reference before handing the raw pointer toset_object, so the post-insertion count lands on the expected1. The companion impl forRegisteredEnum + RegisteredClassinsrc/enum_.rshad the same shape and the same bug, so it got the same one-line correction.Coverage closes the loop. A class extending
\Exceptionwith a#[php(prop)]field is now thrown 1024 times through the exact buggy entry point (ZendClassObject::new(...).into_zval(...)+with_object), and the test asserts that memory growth stays under 8 KB. Without the fix it climbs past 600 KB. Memory growth was chosen over the debug-only HT assertion so the regression remains visible on release PHP builds as well.While auditing, the original draft analysis around
throw_object(insrc/exception.rs) was found to be incorrect:zend_throw_exception_objectdoes not addref on the success path across PHP 8.1 → 8.5, so the currentManuallyDropflow already balances correctly. No change there.Checklist