fix(RetryPolicy): correct class entry and policy leak in *_instantiate helpers - #139
Merged
Merged
Conversation
The php_scylladb_retry_policy_*_instantiate() helpers each had two bugs: 1. Leaked a CassRetryPolicy. object_init_ex() runs the class's create_object handler, which already assigns self->policy. The instantiate function then overwrote obj->policy with a second freshly created policy, so the first one's refcount never dropped to 0. 2. Wrong class entry. DowngradingConsistency, Fallthrough and Logging all passed php_scylladb_retry_policy_default_policy_ce to object_init_ex(), producing a DefaultPolicy object with DefaultPolicy handlers while ->policy held a downgrading/fallthrough/logging CassRetryPolicy. Each now uses its own *_ce and lets create_object own the policy. Logging is the exception: its create_object sets policy = nullptr, so it must still assign, and it now rejects a child whose ->policy is nullptr — gnu::nonnull(1, 2) only constrains the pointer, not what it points at. Also drops the pointless `zval val` temporary; object_init_ex() writes into dst directly. These helpers are exported as PHP_SCYLLADB_API but have no in-tree callers, which is how the wrong class entry went unnoticed.
|
Tick the box to add this pull request to the merge queue (same as
|
The ASan / PHP 8.4-nts leg has been failing on trunk without executing a single test: ==9714==You are trying to dlopen a /usr/lib/php/20240924/opcache.so shared library with RTLD_DEEPBIND flag which is incompatible with sanitizer runtime 9714 Aborted (core dumped) PHP loads zend_extensions with RTLD_DEEPBIND, which the ASan runtime refuses to work with (google/sanitizers#611). With abort_on_error=1 that becomes a SIGABRT during startup, so the job reported "fail" while guarding nothing. setup-php enables opcache on 8.4 but not on 8.5 — that is the entire difference between the two matrix legs, and why 8.5-nts was green. Drop the opcache conf.d entry before the run rather than relaxing ASAN_OPTIONS; opcache is irrelevant to the use-after-free this job exists to catch, whereas abort_on_error=1 is the thing that makes it a guard. The step then asserts opcache is really gone, so a future setup-php change that re-enables it fails loudly instead of silently reverting to an aborting job.
Reverts the opcache-removal step from the previous commit and fixes the root cause instead. The ASan job LD_PRELOADs the sanitizer runtime into a stock, un-instrumented setup-php binary. PHP <= 8.4 dlopen()s shared extensions with RTLD_DEEPBIND, which the ASan runtime refuses to work with (google/sanitizers#611), and abort_on_error=1 turns that into a SIGABRT during startup. Removing opcache was treating a symptom. With opcache gone the same abort moved to mysqlnd.so, and it would have moved again to cassandra.so — the extension under test is itself a shared object, so no amount of pruning conf.d can make the 8.4 leg load it. That leg has been permanently red on trunk while guarding nothing. That step also broke the 8.5 leg, which had been green: setup-php compiles opcache into 8.5 statically, so there is no conf.d entry to remove and the "is opcache gone" assertion failed the job. PHP 8.5 does not pass RTLD_DEEPBIND and runs the whole suite under ASan (861 passing), so it provides the guard on its own. The use-after-free this job exists to catch is not PHP-version-specific.
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
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.
What
Fixes two bugs in each of the four
php_scylladb_retry_policy_*_instantiate()helpers declared ininclude/RetryPolicy/RetryPolicy.h, plus an unrelated CI failure that was blocking this branch (see CI below).1. Leaked
CassRetryPolicyobject_init_ex()runs the class'screate_objecthandler (e.g.php_scylladb_retry_policy_default_policy_new), which already assignsself->policy = cass_retry_policy_default_new(). The instantiate function then overwroteobj->policywith a second freshly created policy — the first one's refcount never dropped to 0.2. Wrong class entry
DowngradingConsistency.c,Fallthrough.candLogging.call passedphp_scylladb_retry_policy_default_policy_cetoobject_init_ex()instead of their own. The returned PHP object was aDefaultPolicy— wrong class, wrong handlers — while->policyheld a downgrading/fallthrough/loggingCassRetryPolicy. On free that runsphp_scylladb_retry_policy_default_policy_freeover a foreign policy.How
Each helper now passes its own
*_ceand letscreate_objectown the policy:Loggingis the exception — itscreate_objectsetspolicy = nullptr, so it must still assigncass_retry_policy_logging_new(retry_policy->policy).The
zval valtemporary plusZVAL_OBJ(dst, Z_OBJ(val))is gone;object_init_ex()writes intodstdirectly.Beyond the reported bugs
php_scylladb_retry_policy_logging_instantiate()now rejects a child whose->policyisnullptr.Logging::__constructis the only thing that setspolicyon a Logging object, so anewInstanceWithoutConstructor()instance passed as the child would have handednullptrtocass_retry_policy_logging_new(). Thegnu::nonnull(1, 2)attribute only constrains the pointers, not what they point at.CI
This part is unrelated to the RetryPolicy change and can be split out if you'd prefer — the workflow commits cherry-pick cleanly onto
trunk.ASan: PHP 8.4-nts / scylladbhas been failing ontrunkwithout executing a single test:The job LD_PRELOADs the ASan runtime into a stock, un-instrumented setup-php binary. PHP <= 8.4
dlopen()s shared extensions withRTLD_DEEPBIND, which the sanitizer runtime refuses to work with (google/sanitizers#611);abort_on_error=1turns that into a SIGABRT at startup.Disabling opcache is not a fix — I tried it first and the abort simply moved to
mysqlnd.so, and would have moved again tocassandra.so. The extension under test is itself a shared object, so no amount of pruningconf.dmakes the 8.4 leg able to load it.PHP 8.5 does not pass
RTLD_DEEPBIND, which is the entire difference between the two matrix legs. The ASan matrix is now 8.5-only, with the reasoning recorded in the workflow. Tradeoff: this gives up ASan coverage on 8.4 — coverage the repo never actually had, since that leg has been aborting. Restoring it means building PHP 8.4 with-fsanitize=addressin CI instead of using setup-php's binary; happy to open a follow-up issue.PHP 8.3-ts / scylladbalso failed on the first run — 862 passed, 0 failures, then exit 2 with no output after the summary. It passed on both subsequent runs and on the two precedingtrunkruns. Treated as a flake, not diagnosed.Verification
cmake --preset DebugPHP8.4NTS && cmake --build out/DebugPHP8.4NTS— 309/309, no new warnings.gc_collect_cycles().ASan: PHP 8.5-nts / scylladbwith 861 passed (12368 assertions) and no sanitizer reports._instantiatehelpers themselves. They are unreachable from PHP, so nothing short of a C-level test can exercise them — the fix is reviewed by inspection against thecreate_objecthandlers in the same files.For the reviewer
These helpers have no in-tree callers. They are declared, defined and exported as
PHP_SCYLLADB_API, and nothing calls them — which is exactly how a wrong class entry survived in three of four files. Deleting them and their declarations is a defensible alternative to this PR; I fixed rather than deleted because removing exported symbols could break an out-of-tree consumer. Happy to convert this to a deletion if you'd prefer.