PHP extension 4.0 - cast from zend_obj

[remicollet]

Casting zend_object to Message * (or any other internal struct) is not the proper way

example:

+/* Message from zend_object */
+static inline Message *fetch_message(zend_object *obj) {
+       return (Message*)((char*)(obj) - XtOffsetOf(Message, std));
+}
+
+/* Message from zval */
+#define Z_MESSAGE_P(zv) fetch_message(Z_OBJ_P((zv)))
+

-  Message* intern = (Message*)obj;
+  Message* intern = fetch_message(obj);

-  Message* intern = (Message*)Z_OBJ_P(obj);
+  Message* intern = Z_MESSAGE_P(obj);

[remicollet]

P.S. no PR because of damned CLA.

[haberman]

Thanks for taking a look at the new extension!

Did this lead to an actual issue for you (eg. a SIGSEGV) or is this something you just saw from inspecting the code?

I know extensions are supposed to use the pattern above. But we force the property count to zero, which prevents PHP from writing properties after the zend_object structure. So in practice it seems to work, at least in the cases I have tried.

[haberman]

To leave a bit more info: in my testing, adding this line prevented PHP from writing to any memory after the zend_object member of struct Message:

protobuf/php/ext/google/protobuf/message.c

Lines 71 to 72 in ffb2b53

	// XXX(haberman): verify whether we actually want to take this route.
	class_type->default_properties_count = 0;

This is probably not a totally kosher approach. I could imagine that it might break on a different major version of PHP, like PHP 8. If so, we should implement your fix.

But if not, does it cause any harm to leave in for now?

[haberman]

Also a note that if we do this, we also need to move the zend_object std member of the Message struct to be at the end instead of at the beginning.

[remicollet]

Did this lead to an actual issue for you (eg. a SIGSEGV) or is this something you just saw from inspecting the code?

From code review, checking for PHP 8 compatibility

we also need to move the zend_object std member of the Message struct to be at the end instead of at the beginning.

Yes, the zend_object must be first member in 5.x and last in 7.x

Following PHP common practice seems better for code legibility, ad may also help for future version

Ex:

+#if PHP_VERSION_ID < 80000
 static zval *Message_read_property(zval *obj, zval *member, int type,
                                    void **cache_slot, zval *rv) {
-  Message* intern = (Message*)Z_OBJ_P(obj);
+  Message* intern = Z_MESSAGE_P(obj);
+#else
+static zval *Message_read_property(zend_object *obj, zend_string *member, int type,
+                                   void **cache_slot, zval *rv) {
+  Message* intern = fetch_message(obj);
+#endif

[haberman]

I would rather avoid #ifdef.

I'm reluctant to change the current code if it works in practice. But if we do need to change it, I'd rather change it for all PHP versions, rather than use #ifdef.

[remicollet]

The #if is mandatory, Message_read_property protocol must be different in php 7 (accepting a zval) and php 8 (accepting a zend_object).

[TeBoring]

In php 8, the writer_property handler needs to return reference to the internal property.
In our new implementation, we don't have a zval property internally. Any suggestion?

[TeBoring]

@remicollet any idea?

[TeBoring]

@dstogov could you help here?

[haberman]

I missed @TeBoring's point before. To clarify, in PHP7 the write_property function prototype was:

void Message_write_property(zval *obj, zval *member, zval *val, void **cache_slot)

Whereas in PHP8 it is:

zval *Message_write_property(zend_object *object, zend_string *member, zval *value, void **cache_slot);

Our message objects do not internally store zval values, so we can't return a pointer to our internal storage. But would it work ok if we just returned the value parameter?

[remicollet]

The change for return value was not in 8.0, but in 7.4

From UPGRADING.INTERNALS

 m. The write_property() object handler now returns the assigned value (after
     possible type coercions) rather than void. For extensions, it should
     usually be sufficient to return whatever was passed as the argument.

[haberman]

The write_property issue was fixed in #7793.

The cast from zend_obj seems to be working fine for now. We can change it if it causes a problem later.