Harden PHP C++ Code Against Non-Factory Constructions

[InsertCreativityHere]

Our C++ PHP wrapper classes need to be constructed through our own factories, so their ptr values are non-null.
This PR fixes a handful of ways that they could be constructed outside of our factories:

1. deserialization is now blocked by setting ZEND_ACC_NOT_SERIALIZABLE for all the types which were affected by this: IcePHP_Communicator, IcePHP_Connection, IcePHP_Endpoint, IcePHP_Logger, IcePHP_Properties, IcePHP_TypeInfo, IcePHP_ExceptionInfo, ObjectPrx, .*EndpointInfo
2. reflection is blocked for some of the types by setting ZEND_ACC_FINAL, which also disables subclasses getting around our checks. This was set for all the above types except for ObjectPrx and the .*EndpointInfo which are public types.
3. direct instantiation through new was blocked for IcePHP_TypeInfo and IcePHP_ExceptionInfo by creating a private __construct which always throws. All the other types already did this.

Then as an absolute fallback to cover the edge cases not worth having dedicated code to fight, this PR adds a null-check to the Wraper::value function. So if the pointer is ever null, it will only trigger a PHP error, instead of a dereferencing crash.

---

All of these types were internal except for ObjectPrx and .*EndpointInfo. And the only change to those 2 is that now they cannot be serialized or deserialized. It is unlikely anyone is ever doing this, so it's fine to remove in a patch release IMO.

[Copilot]

Pull request overview

Hardens PHP native wrapper objects against unsafe deserialization.

Changes:

• Marks native-state wrappers as non-serializable.
• Marks non-inheritable wrappers as final.
• Adds deserialization regression coverage.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
php/test/Ice/info/Client.php	Tests rejection of crafted serialized objects.
php/src/Types.cpp	Hardens type and exception metadata wrappers.
php/src/Proxy.cpp	Disables proxy serialization.
php/src/Properties.cpp	Hardens properties wrappers.
php/src/Logger.cpp	Hardens logger wrappers.
php/src/Endpoint.cpp	Hardens endpoint and endpoint-info wrappers.
php/src/Connection.cpp	Hardens connection wrappers.
php/src/Communicator.cpp	Hardens communicator wrappers.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[pepone]

Add a private __construct to _typeInfoMethods / _exceptionInfoMethods (php/src/Types.cpp:3651). Both tables are empty, so plain new is not blocked by final, and IcePHP_defineSequence('::Foo::Seq', new IcePHP_TypeInfo()) still segfaults on this branch — that's the fourth door in #6379 (comment), not reflection.

Drop the six per-subclass ZEND_ACC_NOT_SERIALIZABLE assignments in php/src/Endpoint.cpp. Zend propagates the flag through inheritance and the base is flagged before the subclasses are registered, so the one on Ice\EndpointInfo already covers the hierarchy.

In the new test, use test(false) after the call and catch Exception, like the clone tests 30 lines below it. The empty catch (Throwable) currently also passes when unserialize fails for some unrelated reason.

One correction for the description, since it's what the next reader will go by: ZEND_ACC_FINAL does close the reflection door. Zend refuses newInstanceWithoutConstructor() when a class is internal, has a create_object handler, and is final, so the seven classes you marked are already covered — Ice\ObjectPrx and the endpoint-info classes are the ones still open. I've recorded the full door matrix on #6379 along with a fifth door (subclass declaring a public __construct), and why final is the wrong tool for those two: they are the only public names here that are concrete classes rather than interfaces, so it would take PHPUnit mocking away.

[externl]

Built the extension on this branch and on its base against PHP 8.5 and ran the hostile constructions against both — the unserialize hole is real and this closes it (unserialize('O:13:"Ice\ObjectPrx":0:{}') then ice_toString() segfaults pre-PR, throws after). Three gaps though, and the first needs neither reflection nor unserialize.

• new IcePHP_TypeInfo() and new IcePHP_ExceptionInfo() still segfault from plain PHP. Types.cpp:3651 and :3654 declare empty method tables ({{0,0,0}}), so unlike every other wrapper class these two have no constructor at all, and ZEND_ACC_FINAL doesn't prevent new. IcePHP_stringify("hi", new IcePHP_TypeInfo()) and IcePHP_defineSequence("::Foo::Seq", new IcePHP_TypeInfo()) both crash on the PR branch.
• ZEND_ACC_FINAL is the reflection fix, and it's already here for 7 of the 15 classes. PHP rejects newInstanceWithoutConstructor() on a final internal class that has a create_object handler, so the 7 you marked final now throw ReflectionException — that half of option 2 is done. The 8 that got only NOT_SERIALIZABLE — Ice\ObjectPrx and the seven Ice\*EndpointInfo — still segfault, and not only via reflection: class Evil extends Ice\ObjectPrx { public function __construct() {} } then ice_toString() crashes too, since a private parent constructor is redeclarable in a subclass. Ice\ObjectPrx looks safe to make final (slice2php emits *PrxHelper static classes and dispatches through handleGetMethod rather than subclassing), as do the leaf *EndpointInfo classes; Ice\EndpointInfo and Ice\IPEndpointInfo are extended inside the extension so they'd need something else.
• ZEND_ACC_NOT_SERIALIZABLE doesn't exist before PHP 8.1, and php/BUILDING.md:22 still lists 8.0 as supported. CI installs the current PHP so it won't catch this. Config.h:73 already has a PHP_VERSION_ID >= 80200 guard as precedent, if the floor isn't just being moved instead.

Two smaller things: the flag blocks serialize() as well as unserialize(), which is a behavior change for the *EndpointInfo classes since they carry genuine public data (host, port, rawBytes) — and it leaves Ice\ConnectionInfo fully serializable while its sibling getInfo() result type throws, with no changelog fragment. And in php/test/Ice/info/Client.php, a class name that rots gives __PHP_Incomplete_Class, which isn't instanceof $className, so the assertion passes silently; the test also covers only unserialize, not the new/reflection/subclass paths above.

Clone is clean — Properties and ObjectPrx have clone_obj handlers that build fresh C++ objects, and every other wrapper sets clone_obj = nullptr so clone throws.

[InsertCreativityHere]

@pepone

Add a private __construct to _typeInfoMethods / _exceptionInfoMethods (php/src/Types.cpp:3651)...

I added private __construct methods to IcePHP_TypeInfo and IcePHP_ExceptionInfo to close the direct construction hole.

Drop the six per-subclass ZEND_ACC_NOT_SERIALIZABLE assignments in php/src/Endpoint.cpp...

This is correct, that the flags are inherited, and removing them would not change anything.
My Claude flagged this, but it advised to keep them anyways for the following reasons.
I'm not married to this choice though. If you'd prefer we remove them, just say so again and I'll remove them!

1. It serves a documentation purpose: you can see that this class is non-serializable without having to check superclasses.
2. It ensures the class is always non-serializable. Relying on inheritance makes it dependant on declaration ordering

In the new test, use test(false) after the call and catch Exception, like the clone tests 30 lines below it...

Fixed as suggested!

One correction for the description, since it's what the next reader will go by: ZEND_ACC_FINAL does close the reflection door...

I substantially updated the description.

[InsertCreativityHere]

@externl

new IcePHP_TypeInfo() and new IcePHP_ExceptionInfo() still segfault from plain PHP...

Fixed this by adding a __construct function to each of these types. The function is private and the implementation throws.

ZEND_ACC_FINAL is the reflection fix, and it's already here for 7 of the 15 classes...

This comment (#6379 (comment)) explains why we shouldn't extend ZEND_ACC_FINAL to these other classes better than I can here, and I agree with it.

Either way, it's unnecessary. This PR adds a catch-all null check to Wrapper::value, so no matter
how the object is improperly constructed, it will always cause a PHP error, not a dereference crash.

ZEND_ACC_NOT_SERIALIZABLE doesn't exist before PHP 8.1

This was a documentation bug. In reality, the minimum PHP version we support is 8.4, so it's fine to use here!
See: #6563

the flag blocks serialize() as well as unserialize(), which is a behavior change for the *EndpointInfo...

I thought that this was minor enough to not warrant a changelog fragment. If you disagree and think this is a real case,
let me know and we can of course add a fragment.

And in php/test/Ice/info/Client.php...

I updated the test to ensure both direct-construction and unserialization are rejected.
I don't think there's a point to checking the reflection, since this was never an exploitable bug, just a correctness
fix. Making sure that users cannot construct our inner library types via reflection just can't be worth a test IMO.

[Copilot]

🟡 Changes recommended

Several ObjectPrx paths still dereference forged wrappers without the new null check and can crash PHP.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Balanced

[pepone]

Please drop the changelog fragment. This is defense-in-depth against objects fabricated outside the supported APIs, not a change to an expected usage pattern.

One remaining gap is ObjectPrx: handleClone, handleGetMethod, handleCompare, and fetchProxy bypass Wrapper::value and can still dereference a null ptr. If routing these paths through a checked helper is straightforward, let us include it here. Otherwise, I am fine deferring it to a focused follow-up issue/PR that records these paths and adds regression coverage.

[externl]

Looks good. Agree with @pepone.

[InsertCreativityHere]

I fixed the holes you mentioned, and checked for every use of Wrapper::extract and Wrapper::fetch. All the remaining uses only appear when deleting the obj->ptr, or when setting it during object initialization.

The other call sites were re-routed to go through Wrapper::value which checks for null.

[Copilot]

🟢 Approval recommended

The defensive checks are consistently applied and the affected construction and comparison paths are covered by tests.

Review details

• Files reviewed: 10/10 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[pepone]

@InsertCreativityHere A few more things after another pass over 6049790. The first two block merging; the rest I would like in this PR unless noted otherwise.

1. PHP 8.0 compatibility (blocking)

Coming back to the PHP 8.0 question from the Copilot thread. I don't think we can treat 8.4 as the minimum for this change.

php-ice-3.8.2-1.el9 in our public repo is built against RHEL 9's stock php-devel 8.0.30 (the el9 builder image is plain ubi9 with no PHP module stream), and php/BUILDING.md at the 3.8.2 release said "PHP 8.0 or higher". Raising the required PHP version in a patch release is too much, so 3.8.x stays on 8.0. The el9 nightly on main builds against the same 8.0.30 today, so as-is this PR breaks that job on main as well, not only on the backport.

The only 8.1+ construct in the diff is ZEND_ACC_NOT_SERIALIZABLE. PHP 8.0 has an equivalent, which is what core used for Closure before the flag existed. Both handlers were removed in 8.1, so it needs a version guard. A helper in Util.h covers both and replaces the repeated flag lines:

inline void denySerialization(zend_class_entry* ce)
{
#if PHP_VERSION_ID >= 80100
    ce->ce_flags |= ZEND_ACC_NOT_SERIALIZABLE;
#else
    ce->serialize = zend_class_serialize_deny;
    ce->unserialize = zend_class_unserialize_deny;
#endif
}

One behavior difference for the new test in php/test/Ice/info/Client.php: on 8.0, serialize() and the C: unserialize form throw the same "not allowed" exception, but the O: form the test uses emits an E_WARNING ("Erroneous data format for unserializing ...") plus an "Error at offset" notice and returns false without creating an object. TestHelper.php installs no error handler, so the catch (Exception) branch falls through to test(false) there. This passes on every version:

try {
    test(@unserialize('O:' . strlen($className) . ':"' . $className . '":0:{}') === false);
} catch (Exception $ex) {
}

Nothing else in CI has PHP 8.0: ubuntu-24.04 is 8.3 and macOS is 8.4+. The only place it shows up is the el9 RPM job, which runs in ghcr.io/zeroc-ice/ice-rpm-builder-el9:3.9 (built from packaging/rpm/docker/el9/Dockerfile, invoked by build-rpm-packages.yml). I ran this in that image (php-devel 8.0.30): 6049790 fails to compile in all seven files with 'ZEND_ACC_NOT_SERIALIZABLE' was not declared in this scope, and nothing else in the diff trips on 8.0. With the helper above on the nine base classes plus the test change, the extension builds and the Ice/info and Ice/proxy suites pass; without the test change Ice/info fails at line 63 with the warning and notice above. Please rerun it there once the branch is updated. Note the harness needs dnf install python3.12 in that image, since the stock 3.9 cannot parse scripts/Expect.py.

2. Docs

Please also revert the php/BUILDING.md change from #6565 so it says "PHP 8.0 or higher" again. The packaging never moved to 8.4, so the doc should stay at 8.0. That applies to main and to the 3.8 cherry-pick of #6565 (a1a3471).

3. The fatal in Wrapper::value leaks resources (php/src/Util.h:55)

zend_error_noreturn(E_ERROR) bails out with a longjmp, so C++ objects on the stack at that point are never destroyed. Several call sites already hold one when value() runs: _this (a CommunicatorInfoIPtr) in proxyToString/proxyToProperty/setDefaultRouter/setDefaultLocator, the Ice::ObjectPrx in handleCompare, the ConnectionPtr in handleConnectionCompare, and ProxyInfo::marshal inside an invocation. Only ~ActiveCommunicator calls communicator->destroy(), so in a long-lived SAPI worker the communicator is never destroyed once this fires. In a php-cgi -b worker the thread count went 4 → 7 → 10 → 13 over three requests of $communicator->proxyToString($bad) (with $bad from newInstanceWithoutConstructor()) and never came back down. Before this PR that was a segfault, which at least freed everything. Check both wrappers' ptr before constructing any C++ object in the handler, or make it a catchable Error (zend_throw_error) and return, which also lets catch (Throwable) in user code see it.

4. Empty shared_ptr is not covered (php/src/Proxy.cpp:1274)

value() checks the heap T*, not the shared_ptr inside it. ice_getConnection() returns null for a collocated proxy, and line 1274 passes it straight to createConnection, while line 1300 in ice_getCachedConnection has the !con || guard. Repro: Ice.Admin.Endpoints=tcp -h 127.0.0.1 -p 14711, then $con = $c->stringToProxy('foo:tcp -h 127.0.0.1 -p 14711')->ice_getConnection(); $con->toString(); → SIGSEGV on this branch, and $con == $con2 hits the assert(con1) kept at Connection.cpp:292 in a debug build. No reflection or misuse needed, so this is exactly the null-pointer class the PR is about. Mirror the guard at 1274.

5. extract() still trusts its argument (php/src/Util.h:46)

value(zval*) → extract() does pointer arithmetic on whatever extractWrapper returns, with no IS_OBJECT or class check. A null, a non-object, a plain object, or a wrapper of the wrong kind yields a non-null garbage Wrapper* and still crashes: IcePHP_stringify(null, 1), IcePHP_stringify(null, new stdClass), IcePHP_defineSequence('::S', new stdClass), IcePHP_stringify(null, Ice\initialize()) all SIGSEGV on this branch. These are the internal IcePHP_* globals, so exposure is low, but the description's "if the pointer is ever null, it will only trigger a PHP error" is not true as written. Cheap fix at the same layer: have extract() return nullptr unless zv is an object whose ce matches, and treat !w like !w->ptr in value(Wrapper<T>*). Otherwise please narrow the description.

6. fetchProxy now copies a shared_ptr per call (php/src/Proxy.cpp:1716)

value() returns T by value from an lvalue, so every Slice invocation (Operation.cpp:779) and every marshaled proxy parameter (Types.cpp:2648) pays a copy and destroy of the ProxyPtr that the old raw-pointer read did not. Return const T& instead: ptr is assigned only at creation and deleted only in free_obj, and every other call site copies into a local anyway, so nothing else changes.

7. Tests

• php/test/Ice/proxy/Client.php:432: test(new stdClass() != $cl) never reaches handleCompare. Zend dispatches to the left operand's handler, so stdClass answers. The branch the new ZEND_COMPARE_OBJECTS_FALLBACK adds (proxy on the right of a non-object) has no assertion; test($cl->ice_toString() == $cl) covers it. Same for the connection test.
• php/test/Ice/info/Client.php:30: "cannot be created outside the extension" overclaims for Ice\ObjectPrx and the *EndpointInfo classes, which stay open by design. Please reword, and since final is doing the work for the other seven, assert isFinal() on them so a future edit cannot drop it silently.

8. Per-subclass flags in Endpoint.cpp

Yes, please drop the six (lines 255, 271, 279, 294, 303, 313). The ordering concern does not apply: zend_register_internal_class_ex takes the parent ce, so the parent is always registered, and flagged, first. With the helper above that is one call on Ice\EndpointInfo.

Minor, your call

• The fatal's get_active_function_name() prefix names the wrong frame in the compare/clone/get_method/cast handlers (main():, sort():, is_callable():) and prints (null)(): under php -r or eval. ZSTR_VAL(w->zobj.ce->name) is available in value(Wrapper<T>*) and is right everywhere.
• The two new private throwing __construct bodies (and the eight existing ones) never run: the engine rejects a private constructor before the method is called. A shared get_constructor handler is what core uses for Closure and Generator, and it would also make new on a userland subclass of Ice\ObjectPrx fail with a catchable Error instead of the fatal on first use. Fine as a follow-up.

Out of scope, for a follow-up issue: IcePHP_Endpoint has no compare handler, so $eps[0] == $eps[1] is true for any two endpoints (std handler on property-less objects). Every other mapping compares endpoints by value.