Replace zend_object object with its header to prevent the "using flexible array in the middle of another struct" problem in its inheritted classes (static analyzer report)

[Snape3058]

Class zend_object is defined as a flexible array of length 1. The flexible array defined with size 1 and 0 is not the standard behavior. It is suggested to use the unsized definition (https://people.kernel.org/kees/bounded-flexible-arrays-in-c). Besides, not all its subclasses will use the array field properties_table of the zend_object class. If I understand the code correctly, when the properties_table[0] field is not used, it will store a ZVAL_UNDEF zval indicating the end of the iteration. Whereas when the properties_table[0] field is used, the flags and array length are checked first before accessing the data in the array.

If the properties_table[0] field is not used in these sub-classes, will it be better to replace the zend_object in these classes with only the header part of zend_object?

i.e. (as suggested in case 2 of https://lpc.events/event/18/contributions/1722/attachments/1591/3303/Wfamnae_lpceu2024.pdf)
We can define another struct with only the header part (let's name it zend_object_header_part), but leave the zend_object struct with both the header and the flexible array part.
When only the header is needed, we can use the zend_object_header_part (e.g. in the class inheritance), whereas for those requiring the array part, or using the object through a zend_object pointer, we can still use the full definition.

---

Usages of zend_object in the middle of other structs whose array field is potentially never used through the composite struct:

• php-src/Zend/zend_generators.h

  Lines 58 to 59 in c2fddac

  	struct _zend_generator {
  	zend_object std;

• php-src/Zend/zend_interfaces.c

  Lines 490 to 491 in c2fddac

  	typedef struct {
  	zend_object std;

• php-src/Zend/zend_closures.c

  Lines 31 to 32 in c2fddac

  	typedef struct _zend_closure {
  	zend_object std;

• php-src/Zend/zend_fibers.h

  Lines 102 to 104 in c2fddac

  	struct _zend_fiber {
  	/* PHP object handle. */
  	zend_object std;

• php-src/Zend/zend_iterators.h

  Lines 64 to 65 in c2fddac

  	struct _zend_object_iterator {
  	zend_object std;

• php-src/ext/opcache/jit/zend_jit_ir.c

  Lines 8451 to 8452 in c2fddac

  	typedef struct _zend_closure {
  	zend_object std;

• php-src/ext/com_dotnet/com_saproxy.c

  Lines 35 to 36 in c2fddac

  	typedef struct {
  	zend_object std;

• php-src/ext/com_dotnet/php_com_dotnet_internal.h

  Lines 28 to 29 in c2fddac

  	typedef struct _php_com_dotnet_object {
  	zend_object zo;

• php-src/ext/com_dotnet/com_persist.c

  Lines 278 to 279 in c2fddac

  	typedef struct {
  	zend_object std;

• php-src/ext/ffi/ffi.c

  Lines 169 to 170 in c2fddac

  	typedef struct _zend_ffi {
  	zend_object std;

• php-src/ext/ffi/ffi.c

  Lines 191 to 192 in c2fddac

  	typedef struct _zend_ffi_cdata {
  	zend_object std;

• php-src/ext/ffi/ffi.c

  Lines 199 to 200 in c2fddac

  	typedef struct _zend_ffi_ctype {
  	zend_object std;

• php-src/ext/pdo/php_pdo_driver.h

  Lines 645 to 646 in c2fddac

  	struct _pdo_row_t {
  	zend_object std;

• php-src/ext/zend_test/fiber.h

  Lines 24 to 25 in c2fddac

  	struct _zend_test_fiber {
  	zend_object std;

• php-src/ext/intl/normalizer/normalizer_class.h

  Lines 25 to 26 in c2fddac

  	typedef struct {
  	zend_object zo;

• php-src/ext/intl/locale/locale_class.h

  Lines 25 to 26 in c2fddac

  	typedef struct {
  	zend_object zo;

report ids: 250106-1639:1-6,8-17 (16 reports in total)

[cmb69]

Hmm, as of PHP 7.0.0, zend_object is supposed to be the last member (see https://www.npopov.com/2015/06/19/Internal-value-representation-in-PHP-7-part-2.html#objects-in-php-7). I thought only ext/com hasn't been changed in this regard, but apparently there are several other classes. I'll have a look at ext/com.

[cmb69]

php-src/Zend/zend_object_handlers.h

Line 206 in b14469b

/* offset of real object header (usually zero) */

Appears to be somewhat outdated.

[Snape3058]

Appears to be somewhat outdated.

Should we make changes to them? Or are they left for compatibility with the old code?

[cmb69]

Well, the comment should probably be updated, and the struct layouts should be changed.

[Snape3058]

I am not very familiar with the code base. I can draft patches for them but I still need other developers to help me to prevent possible mistakes.

I will try to resolve one of them in recent days and open a PR.

Thank you for your replies.

[cmb69]

See the following for a yet incomplete patch for com_saproxy`(ignore the changes to build.ninja):

 ext/com_dotnet/com_saproxy.c | 19 ++++++++++++-------
 win32/build/build.ninja      |  2 +-
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/ext/com_dotnet/com_saproxy.c b/ext/com_dotnet/com_saproxy.c
index ea0e9e47a1..3e12d695fe 100644
--- a/ext/com_dotnet/com_saproxy.c
+++ b/ext/com_dotnet/com_saproxy.c
@@ -33,7 +33,6 @@
 #include "Zend/zend_exceptions.h"
 
 typedef struct {
-	zend_object std;
 	/* the object we a proxying for; we hold a refcount to it */
 	php_com_dotnet_object *obj;
 
@@ -42,7 +41,7 @@ typedef struct {
 
 	/* this is an array whose size_is(dimensions) */
 	zval *indices;
-
+	zend_object std;
 } php_com_saproxy;
 
 typedef struct {
@@ -55,13 +54,19 @@ typedef struct {
 	LONG *indices;
 } php_com_saproxy_iter;
 
-#define SA_FETCH(zv)			(php_com_saproxy*)Z_OBJ_P(zv)
+static inline php_com_saproxy *php_com_saproxy_from_obj(zend_object *obj) {
+	return (php_com_saproxy *) ((char *) obj - XtOffsetOf(php_com_saproxy, std));
+}
+
+#define SA_FETCH(zv) php_com_saproxy_from_obj(Z_OBJ_P(zv))
 
 zend_object *php_com_saproxy_create_object(zend_class_entry *class_type)
 {
-	php_com_saproxy *intern = emalloc(sizeof(*intern));
-	memset(intern, 0, sizeof(*intern));
+	size_t block_len = sizeof(php_com_saproxy) + zend_object_properties_size(class_type);
+	php_com_saproxy *intern = emalloc(block_len);
+	memset(intern, 0, block_len);
 	zend_object_std_init(&intern->std, class_type);
+	object_properties_init(&intern->std, class_type);
 	return &intern->std;
 }
 
@@ -364,7 +369,7 @@ static zend_result saproxy_count_elements(zend_object *object, zend_long *count)
 
 static void saproxy_free_storage(zend_object *object)
 {
-	php_com_saproxy *proxy = (php_com_saproxy *)object;
+	php_com_saproxy *proxy = php_com_saproxy_from_obj(object);
 //???	int i;
 //???
 //???	for (i = 0; i < proxy->dimensions; i++) {
@@ -398,7 +403,7 @@ static zend_object* saproxy_clone(zend_object *object)
 }
 
 zend_object_handlers php_com_saproxy_handlers = {
-	0,
+	XtOffsetOf(php_com_saproxy, std),
 	saproxy_free_storage,
 	zend_objects_destroy_object,
 	saproxy_clone,
diff --git a/win32/build/build.ninja b/win32/build/build.ninja
index 71db89e375..45580ce028 100644
--- a/win32/build/build.ninja
+++ b/win32/build/build.ninja
@@ -23,7 +23,7 @@ PGOMGR="${PGOMGR}"
 PHP_BUILD=${PHP_BUILD}
 
 # See https://ninja-build.org/manual.html#_deps
-# msvc_deps_prefix = Hinweis: Einlesen der Datei:
+msvc_deps_prefix = Hinweis: Einlesen der Datei:
 
 MCFILE=${BUILD_DIR}\wsyslog.rc

[Girgias]

I've submitted #17606 for the pdo_row_t struct.

[Snape3058]

To make it clear, I added checkboxes to each case to be fixed.

[Snape3058]

I am working on _zend_generator now. I will check the box when I submit the PR.

[Snape3058]

@cmb69 I tried to make a change to zend_generator (Snape3058@ebe9ffc). However. it seems that the generators returned from a function (related to execute_data) cannot be addressed correctly after the change. Could you please help me with this patch? Where can I find the places creating and assigning the return values?

[cmb69]

@Snape3058, EX(return_value) is of type zval*, so I would assume that a zend_generator* is just cast to zval* somewhere. If I'm right, just sticking with the cast back to zend_generator* should work. Anyway, consider to file a draft PR, where such details can be better discussed.

[Snape3058]

@cmb69 The current version (Snape3058@ebe9ffc) contains the changes to all cases that I can find. But it cannot pass the test cases. Is it OK to draft a PR now?

[cmb69]

Is it OK to draft a PR now?

Yes, sure. :)

[Girgias]

I don't think tackling the Zend or Opcache ones is the easiest first step. Fixing the ext/intl are probably the easiest.

[bwoebi]

I thought we had the zend_object on purpose as the first member on those classes which don't use properties.
The purpose being avoiding useless offset computations and better locality for the trivial refcounting which is always in the first place when zend_object is the first item.

IIRC it was worth the extra 8 bytes allocated.

[cmb69]

I thought we had the zend_object on purpose as the first member on those classes which don't use properties.

Hmm, possibly. I always thought that the layout change has just been missed for com_dotnet (like a lot of other improvements).

If we want to stick with the order, we should at least document that (including a comment in the respective code).