Everything

php

extended objects with read_property and get_property_ptr_ptr

php.internals

Rob Richards

22 years ago

I was playing around with Bug #28817 and seem to have run into an issue when overriding the read_property handler with an extended object (this case a DomDocument object).

In order to support standard properties correctly, such as $obj->myprop['a'] = 'b', the internal object needs to support get_property_ptr_ptr. As the internal object here overrides the read_property handler, it would also need to override the get_property_ptr_ptr handler to make that standard property work correctly, which is where I have run into some problems.

At first, I implemented a get_property_ptr_ptr handler, but found that for the base internal class properties, this method needs to return NULL so that cases like zend_pre_incdec_property would fall back to the read_property/write_property handling. Next was to return NULL for all properties of the base internal class and return the result of the standard get_property_ptr_ptr for non base class properties. This fixed the standard properties, but returning NULL for the internal base properties breaks calls like:
$document->documentElement->ownerDocument;
because zend_fetch_property_address_inner will not fall back to the read_property handler if NULL is returned get_property_ptr_ptr.
Rather the error "Cannot access undefined property for object with overloaded property access" is returned.

Is this is a pitfall of the engine or is there something I am overlooking which would allow this to work across all cases?

Thanks,

Rob

Marcus Börger

22 years ago

Hello Rob,

Tuesday, July 6, 2004, 12:15:31 PM, you wrote:

> I was playing around with Bug #28817 and seem to have run into an issue
> when overriding the read_property handler with an extended object (this
> case a DomDocument object).

> In order to support standard properties correctly, such as
> $obj->myprop['a'] = 'b', the internal object needs to support
> get_property_ptr_ptr. As the internal object here overrides the
> read_property handler, it would also need to override the
> get_property_ptr_ptr handler to make that standard property work
> correctly, which is where I have run into some problems.

> At first, I implemented a get_property_ptr_ptr handler, but found that
> for the base internal class properties, this method needs to return NULL
> so that cases like zend_pre_incdec_property would fall back to the
> read_property/write_property handling. Next was to return NULL for all
> properties of the base internal class and return the result of the
> standard get_property_ptr_ptr for non base class properties. This fixed
> the standard properties, but returning NULL for the internal base
> properties breaks calls like:
> $document->documentElement->ownerDocument;
> because zend_fetch_property_address_inner will not fall back to the
> read_property handler if NULL is returned get_property_ptr_ptr.
> Rather the error "Cannot access undefined property for object with
> overloaded property access" is returned.

> Is this is a pitfall of the engine or is there something I am
> overlooking which would allow this to work across all cases?

Sounds more like small oversight. Could you try to make a patch for that?

Best regards,
Marcus                            mailto:helly@php.net

Rob Richards

22 years ago

Attached is a patch for zend_execute.c which will fall to read_property if
get_property_ptr_ptr returns NULL in zend_fetch_property_address_inner.
Hopefully this is the correct fix as it didnt break any tests and resolves
the issue I hit.

Rob

From: Marcus Boerger

Zeev Suraski

22 years ago

At 16:32 07/07/2004, Rob Richards wrote:

>Attached is a patch for zend_execute.c which will fall to read_property if
>get_property_ptr_ptr returns NULL in zend_fetch_property_address_inner.
>Hopefully this is the correct fix as it didnt break any tests and resolves
>the issue I hit.

It's not a correct fix :I  get_ptr_ptr must return the address of an
allocated zval *, one which can later be separated,
etc.  T(result->u.var).var.ptr doesn't fall into that category - it can be
gone in the next opcode...

The only safe way to return a zval ** is for the underlying object model to
implement get_ptr_ptr...

Zeev

Rob Richards

22 years ago

From: Zeev Suraski

> It's not a correct fix :I  get_ptr_ptr must return the address of an
> allocated zval *, one which can later be separated,
> etc.  T(result->u.var).var.ptr doesn't fall into that category - it can be
> gone in the next opcode...
>
> The only safe way to return a zval ** is for the underlying object model

to

> implement get_ptr_ptr...

What is this then a catch 22? If get_ptr_ptr is implemented and returns an
allocated zval **, then you can run into problems using pre/post_incdec if
you need to rely on the read_property and write_property of those functions,
which are only hit if you return NULL from get_ptr_ptr, which then brings
one back full circle to getting an error in
zend_fetch_property_address_inner when returning a NULL zval **.

So, my question boils down to is this a problem in the engine, the extension
or a combination of both.

Thanks,

Rob

Zeev Suraski

22 years ago

At 22:27 07/07/2004, Rob Richards wrote:

>  From: Zeev Suraski
>
> > It's not a correct fix :I  get_ptr_ptr must return the address of an
> > allocated zval *, one which can later be separated,
> > etc.  T(result->u.var).var.ptr doesn't fall into that category - it can be
> > gone in the next opcode...
> >
> > The only safe way to return a zval ** is for the underlying object model
>to
> > implement get_ptr_ptr...
>
>What is this then a catch 22? If get_ptr_ptr is implemented and returns an
>allocated zval **, then you can run into problems using pre/post_incdec if
>you need to rely on the read_property and write_property of those functions,
>which are only hit if you return NULL from get_ptr_ptr, which then brings
>one back full circle to getting an error in
>zend_fetch_property_address_inner when returning a NULL zval **.

I guess so, yes, it's a bit of a catch 22.  get_ptr_ptr is used in very
rare cases - assign-by-reference is one of them, and pre/post incdec is
another.  The assumption is that if you provide get_ptr_ptr, the engine can
perform incdec on its own, without calling read_/write_property.  It's
basically an optimization for standard PHP objects.  So if you want to
support assign-by-ref and also override incdec, I guess it cannot be done
right now.

The correct solution, I think, would be adding a boolean function (or even
just a bit) to the handlers structure, that instructs the engine whether it
should use get_ptr_ptr for incdec or not, since it's an
optimization.  Maybe we can just check whether we're dealing with PHP
objects and perform this optimization only then.  I need to look into it.

Zeev

Dominik del Bondio

22 years ago

Zeev Suraski <zeev <at> zend.com> writes:

> The correct solution, I think, would be adding a boolean function (or even
> just a bit) to the handlers structure, that instructs the engine whether it
> should use get_ptr_ptr for incdec or not, since it's an
> optimization.  Maybe we can just check whether we're dealing with PHP
> objects and perform this optimization only then.  I need to look into it.
>
> Zeev
>

I just wanted to point you to bug http://bugs.php.net/bug.php?id=28444 which
would get fixed with this patch as well, so only fixing this for post/incdec is
probably a bad idea :)

I think this should be targeted to be fixed for the 5.0 release because i ran
across this behaviour very quick after using __get & __set and it could produce
a _bit_ confusion

Rob Richards

22 years ago

From: Zeev Suraski

> The correct solution, I think, would be adding a boolean function (or even
> just a bit) to the handlers structure, that instructs the engine whether

it

> should use get_ptr_ptr for incdec or not, since it's an
> optimization.  Maybe we can just check whether we're dealing with PHP
> objects and perform this optimization only then.  I need to look into it.

I dont see the problem as being with incdec. In my particular case at least,
I dont want to implement get_ptr_ptr. The only reason it was needed was to
support properties on an extended object which are not from the base class.
All the properties of the base class want/need to be handled with the
read/write_property handlers. If a property is not of the base class, then
get_ptr_ptr is needed so that the standard get_ptr_ptr handler can be called
for those properties, while all base class properties would return NULL from
the get_ptr_ptr handler (thus in most cases defaulting to calling the
read_property handler).

zend_fetch_property_address_inner is the only function that does not fall
back on the read_property if NULL is returned from get_ptr_ptr. Looking at
the other bug mentioned (28444), standard objects also fall into a similar
problem I hit when returning NULL from get_ptr_ptr.
zend_std_get_property_ptr_ptr assumes that if it returns NULL, then it
should try with the normal __get handler (which also errors out since
zend_fetch_property_address_inner doesnt fall back to the other handler).

If there is a problem with always forcing zend_fetch_property_address_inner
to use the read_property handler when get_ptr_ptr returns NULL, it should at
least allow it to fall back to the read_property handler when the object is
not a standard object. In this case the developer knows they are returning
NULL or a zval ** for a reason and are handling things in the read_property
handler if needed, though it still leaves open the issue with standard
objects.

Rob