Skip to content

Commit 2f08869

Browse files
Address review: tighter guards + lazy-object safety + engine-routed dynamic writes
Review feedback from @arnaudlb (PR #12): 1. Simplify dc_is_backed_declared_property(): !(pi->flags & ZEND_ACC_VIRTUAL) already implies pi->offset == ZEND_VIRTUAL_PROPERTY_OFFSET, and IS_HOOKED_PROPERTY_OFFSET() is only meaningful on offsets returned by zend_get_property_offset(), not on the raw pi->offset. Collapse to a single bitmask test. 2. Narrow the backed-enum scalar cast: Add `(ZEND_TYPE_PURE_MASK(pi->type) & ~MAY_BE_NULL) == 0` so the cast only fires for types of the form `Enum` or `?Enum`. Unions like `Enum|string|int` already accept the scalar literally — casting it to the enum case would be surprising. 3. Lazy-object safety in dc_write_backed_property(): Check !zend_lazy_object_initialized(obj) at the top and, when the flag is absent, route through zend_update_property_ex() which triggers the lazy realization hook. Direct OBJ_PROP slot writes on a lazy ghost left it in a half-initialized state. DEEPCLONE_HYDRATE_NO_LAZY_INIT keeps its existing opt-out fast path. 4. Dynamic-property fallback via zend_update_property_ex(): Swap zend_std_write_property() for zend_update_property_ex() so any overridden write_property handler on internal classes or extensions is respected. Picks up the appropriate scope via scope_ce ?: obj_ce. A2 (null → unset on non-nullable typed) and A3 (scalar → backed-enum cast) stay property-type-only, as discussed: the coercions are driven by the target type at hydration time, which is stable within a single execution.
1 parent 5116a1f commit 2f08869

1 file changed

Lines changed: 27 additions & 14 deletions

File tree

deepclone.c

Lines changed: 27 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -638,15 +638,12 @@ static zend_always_inline bool dc_class_allowed(HashTable *set, zend_string *nam
638638

639639
static zend_always_inline bool dc_is_backed_declared_property(zend_property_info *pi)
640640
{
641-
/* A "backed declared property" is a non-static, non-virtual property with
642-
* a real backing slot (offset >= ZEND_FIRST_PROPERTY_OFFSET). Hooked
643-
* non-virtual properties qualify (they have a real offset); virtual
644-
* properties and hook-metadata offsets do not. */
645-
return pi
646-
&& !(pi->flags & ZEND_ACC_STATIC)
647-
&& !(pi->flags & ZEND_ACC_VIRTUAL)
648-
&& pi->offset != ZEND_VIRTUAL_PROPERTY_OFFSET
649-
&& !IS_HOOKED_PROPERTY_OFFSET(pi->offset);
641+
/* Non-static, non-virtual property with a real backing slot. ZEND_ACC_VIRTUAL
642+
* is the definitive flag — virtual props also have pi->offset set to
643+
* ZEND_VIRTUAL_PROPERTY_OFFSET, and IS_HOOKED_PROPERTY_OFFSET() is only
644+
* meaningful on offsets returned by zend_get_property_offset(), not on
645+
* the raw pi->offset (per arnaudlb's review feedback). */
646+
return pi && !(pi->flags & (ZEND_ACC_STATIC | ZEND_ACC_VIRTUAL));
650647
}
651648

652649
static zend_always_inline bool dc_is_std_scope_property(zend_property_info *pi)
@@ -736,6 +733,15 @@ static bool dc_write_backed_property(zend_object *obj, zend_property_info *pi,
736733
#if PHP_VERSION_ID >= 80400
737734
bool call_hooks = (flags & DEEPCLONE_HYDRATE_CALL_HOOKS) != 0;
738735
bool no_lazy_init = (flags & DEEPCLONE_HYDRATE_NO_LAZY_INIT) != 0;
736+
737+
/* Lazy objects: a direct slot write would bypass the engine's realization
738+
* hook and leave the object in a half-initialized state. Route through
739+
* zend_update_property_ex, which triggers realization on the first write.
740+
* NO_LAZY_INIT has its own fast path below that deliberately opts out. */
741+
if (!no_lazy_init && UNEXPECTED(!zend_lazy_object_initialized(obj))) {
742+
zend_update_property_ex(pi->ce, obj, name, value);
743+
return !EG(exception);
744+
}
739745
#endif
740746
zval *slot = OBJ_PROP(obj, pi->offset);
741747

@@ -773,7 +779,11 @@ static bool dc_write_backed_property(zend_object *obj, zend_property_info *pi,
773779
bool enum_holder_used = false;
774780
if ((Z_TYPE_P(value) == IS_LONG || Z_TYPE_P(value) == IS_STRING)
775781
&& ZEND_TYPE_HAS_NAME(pi->type)
776-
&& !ZEND_TYPE_HAS_LIST(pi->type))
782+
&& !ZEND_TYPE_HAS_LIST(pi->type)
783+
/* Only cast for types of the form `Enum` or `?Enum` — reject unions like
784+
* `Enum|string|int` where the caller explicitly opted into accepting scalars
785+
* (per arnaudlb's review feedback). */
786+
&& (ZEND_TYPE_PURE_MASK(pi->type) & ~MAY_BE_NULL) == 0)
777787
{
778788
zend_class_entry *type_ce = zend_lookup_class_ex(
779789
ZEND_TYPE_NAME(pi->type), NULL, ZEND_FETCH_CLASS_NO_AUTOLOAD);
@@ -3337,10 +3347,13 @@ PHP_FUNCTION(deepclone_hydrate)
33373347
RETURN_THROWS();
33383348
}
33393349
} else {
3340-
/* Fallback: dynamic property or unknown name. Matches
3341-
* unserialize(): the engine rejects NUL-prefix names with
3342-
* \Error and accepts NUL-in-middle as raw dynamic props. */
3343-
zend_std_write_property(obj, prop_name, prop_val, NULL);
3350+
/* Fallback: dynamic property or unknown name. Goes through
3351+
* zend_update_property_ex so any custom write_property handler
3352+
* (internal classes, extensions overriding default handlers) is
3353+
* respected. Matches unserialize(): the engine rejects NUL-prefix
3354+
* names with \Error, NUL-in-middle is stored as a raw dynamic
3355+
* property. */
3356+
zend_update_property_ex(scope_ce ?: obj_ce, obj, prop_name, prop_val);
33443357
if (UNEXPECTED(EG(exception))) {
33453358
if (prop_name_owned) zend_string_release(prop_name);
33463359
EG(fake_scope) = old_scope;

0 commit comments

Comments
 (0)