fix: accept explicit null $options and close cycle-collector use-after-free (#108) - #135
Conversation
…r-free (#108) Reported in #108: passing an explicit null as $options to DefaultSession::prepare()/execute()/executeAsync()/prepareAsync() threw an InvalidArgumentException, even though the signature is `array|ExecutionOptions|null`. Z_PARAM_ZVAL yields a non-null IS_NULL zval, so the `if (options)` guard entered the type check and rejected it. Switch to Z_PARAM_ZVAL_OR_NULL so an explicit null collapses to nullptr and behaves like omitting the argument. While validating the fix against the full suite under AddressSanitizer, a separate cycle-collector use-after-free surfaced: the collection/type value objects keep their data (element values, sub-types, the value's type) in the C struct, but their get_gc handlers fell back to zend_std_get_gc, which invokes a get_properties that rebuilds object->properties on every call. Because the collector calls get_gc several times per run, it decremented one set of child refcounts and re-incremented a freshly rebuilt set, corrupting refcounts and prematurely freeing a shared type's CassDataType (crash on teardown). Give every affected handler a real zend_get_gc_buffer over its actual C-struct zvals: Set, Map, Collection, Tuple, UserTypeValue, their Cassandra\Type\* counterparts, Decimal, Varint, and the cluster Builder. No zend_std_get_gc remains. Also fixed while getting the suite green cross-platform: - Numbers: Tinyint/Smallint string overflow reported the 32-bit bounds because parse_int threw (clobbering errno) before the caller could narrow the range; parse_int now signals overflow via errno without throwing. Bigint's double path used a raw (int64_t) cast that is UB at 2^63 and saturates differently per platform; use zend_dval_to_lval. - cmake: on Apple, resolve libuv-static's "-l:libuv.a" (GNU ld name-form, which ld64 cannot parse) to an absolute archive path so static libuv links. Tests: add NullOptionsTest (regression for #108) and GarbageCollectionTest (GC smoke over every fixed handler). Add a test-asan CI job that runs the full suite under AddressSanitizer (LD_PRELOAD + USE_ZEND_ALLOC=0) — red before the get_gc fix, green after; a plain run is not a reliable guard since the fault is heap-layout dependent and need not crash on x86. Verified: full suite green on PHP 8.3/8.4/8.5, ASan-clean, and the get_gc revert deterministically reproduces the crash.
|
Tick the box to add this pull request to the merge queue (same as
|
|
Git push to origin failed for v2.x with exitcode 1 |
|
Git push to origin failed for v1.x with exitcode 1 |
There was a problem hiding this comment.
Pull request overview
This PR fixes a userland API inconsistency in DefaultSession where passing an explicit null $options incorrectly threw, and addresses a release-critical PHP cycle-collector use-after-free by replacing zend_std_get_gc fallbacks with explicit GC buffers over the extension’s internal zvals. It also includes a couple of cross-platform correctness/build fixes (numeric parsing and Apple static libuv linking) and adds CI coverage with an AddressSanitizer job.
Changes:
- Accept explicit
nullfor$optionsinDefaultSession::{execute,executeAsync,prepare,prepareAsync}by parsing withZ_PARAM_ZVAL_OR_NULL. - Fix GC root reporting for several value/type objects (and numeric types) to prevent GC refcount corruption and use-after-free.
- Add regression tests + an ASan CI job; adjust numeric parsing behavior and Apple static libuv linking.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Unit/GarbageCollectionTest.php | Adds GC smoke tests that exercise the updated get_gc paths via gc_collect_cycles(). |
| tests/Feature/Sessions/NullOptionsTest.php | Adds regression coverage ensuring explicit null $options behaves like omission. |
| src/DefaultSession.c | Switches session option parsing to Z_PARAM_ZVAL_OR_NULL to handle explicit null. |
| src/Collection.c | Implements explicit GC buffer reporting for collection value internals. |
| src/Set.c | Implements explicit GC buffer reporting for set value internals. |
| src/Map.c | Implements explicit GC buffer reporting for map value internals. |
| src/Tuple.c | Implements explicit GC buffer reporting for tuple value internals. |
| src/UserTypeValue.c | Implements explicit GC buffer reporting for UDT value internals. |
| src/Type/Collection.c | Implements explicit GC buffer reporting for collection type internals. |
| src/Type/Set.c | Implements explicit GC buffer reporting for set type internals. |
| src/Type/Map.c | Implements explicit GC buffer reporting for map type internals. |
| src/Type/Tuple.c | Implements explicit GC buffer reporting for tuple type internals. |
| src/Type/UserType.c | Implements explicit GC buffer reporting for UDT type internals. |
| src/Numbers/Decimal.c | Avoids zend_std_get_gc for Decimal (no zval roots). |
| src/Numbers/Varint.c | Avoids zend_std_get_gc for Varint (no zval roots). |
| src/Numbers/NumberParser.c | Changes int overflow signaling to preserve errno for Tinyint/Smallint range errors. |
| src/Numbers/Tinyint.c | Updates comments/behavior to rely on errno==ERANGE overflow signaling from parse_int. |
| src/Numbers/Smallint.c | Updates comments/behavior to rely on errno==ERANGE overflow signaling from parse_int. |
| src/Numbers/Bigint.c | Uses zend_dval_to_lval for defined double→int64 conversion behavior. |
| src/Cluster/BuilderHandlers.c | Updates cluster builder get_gc to use GC buffer (but see review comment). |
| cmake/FindLibuv.cmake | Adds Apple-specific resolution of -l:libuv.a to an absolute archive path for ld64. |
| .github/actions/build-extension/action.yml | Adds a sanitize_address input and wires it to sanitizer CMake flags. |
| .github/workflows/test.yml | Adds an ASan job running the full test suite under AddressSanitizer on PHP 8.4/8.5. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| zend_get_gc_buffer *buffer = zend_get_gc_buffer_create(); | ||
| zend_get_gc_buffer_add_zval(buffer, &self->default_timeout); | ||
| zend_get_gc_buffer_use(buffer, table, n); | ||
| return nullptr; |
Summary
Fixes #108, and closes a cycle-collector use-after-free found while validating that fix under AddressSanitizer.
#108 — explicit
null$optionsthrewDefaultSession::prepare()/execute()/executeAsync()/prepareAsync()declarearray|ExecutionOptions|null $options = null, but passing an explicitnullthrewInvalidArgumentException.Z_PARAM_ZVALyields a non-nullIS_NULLzval, so theif (options)guard entered the type check and rejected it. Switched toZ_PARAM_ZVAL_OR_NULL, so an explicitnullcollapses tonullptrand behaves exactly like omitting the argument.Cycle-collector use-after-free (release-critical)
The collection/type value objects keep their data (element values, sub-types, the value's
type) in the C struct, but theirget_gchandlers fell back tozend_std_get_gc, which invokes aget_propertiesthat rebuildsobject->propertieson every call. Because the collector callsget_gcseveral times per run, it decremented one set of child refcounts and re-incremented a freshly rebuilt set → refcount corruption → a shared type'sCassDataTypefreed while still referenced → crash on teardown.Every affected handler now builds a real
zend_get_gc_bufferover its actual C-struct zvals:Set,Map,Collection,Tuple,UserTypeValue, theirCassandra\Type\*counterparts, plusDecimal,Varint, and the clusterBuilder. Nozend_std_get_gcremains.Also fixed to get the suite green cross-platform
Tinyint/Smallintstring overflow reported the 32-bit bounds becauseparse_intthrew (clobberingerrno) before the caller could narrow the range —parse_intnow signals overflow viaerrnowithout throwing.Bigint's double path used a raw(int64_t)cast that is UB at 2^63 and saturates differently per platform — now useszend_dval_to_lval.libuv-static's-l:libuv.a(GNU-ld name-form thatld64can't parse) to an absolute archive path so static libuv links.Tests / CI
tests/Feature/Sessions/NullOptionsTest.php— regression for Prepare of statements with $options set to null causes Exception #108.tests/Unit/GarbageCollectionTest.php— GC smoke over every fixed handler.test-asanCI job runs the full suite under AddressSanitizer (LD_PRELOAD+USE_ZEND_ALLOC=0) on PHP 8.4/8.5. This is the real guard: red before theget_gcfix, green after. A plain run is not reliable — the fault is heap-layout dependent and need not crash on x86 (which is why it slipped past CI).Verification
CollectionType::~CollectionType←php_scylladb_type_tuple_free).get_gchandlers reproduces the crash deterministically (exit 139 at the same test); restoring them is clean.