Unaligned flexible array field and wrapped array in `zend_accel_globals` (static analyzer report)

[Snape3058]

Description

Struct zend_string is defined as a flexible array

php-src/Zend/zend_types.h

Lines 373 to 378 in c2fddac

	struct _zend_string {
	zend_refcounted_h gc;
	zend_ulong h; /* hash value */
	size_t len;
	char val[1];
	};

and used in the middle of another struct zend_accel_globals

php-src/ext/opcache/ZendAccelerator.h

Lines 227 to 228 in c2fddac

	zend_string key;
	char _key[MAXPATHLEN * 8];

The offset of array header zend_string::val and the following wrapped array zend_accel_globals::_key are not aligned.

According to the output of pahole on x86_64,

struct _zend_string {
    zend_refcounted_h          gc;                   /*     0     8 */
    zend_ulong                 h;                    /*     8     8 */
    size_t                     len;                  /*    16     8 */
    char                       val[1];               /*    24     1 */

    /* size: 32, cachelines: 1, members: 4 */
    /* padding: 7 */
    /* last cacheline: 32 bytes */
};

struct _zend_accel_globals {
    /* omitted for simplicity */
    zend_string                key;                  /*   400    32 */
    char                       _key[32768];          /*   432 32768 */

    /* size: 33200, cachelines: 519, members: 25 */
    /* sum members: 33187, holes: 4, sum holes: 13 */
    /* last cacheline: 48 bytes */
};

the offset of key is 400, so its val starts from 424; whereas the offset of _key is 432.
There is a padding of 7 bytes between them.
This means that for a pointer p of type zend_accel_globals, p->key.val[1] is not p->_key[0].

When these two fields are used together, it will lead to unexpected behaviors.

Although, with a brief search with clang-query, I did not find any usages of these two fields.
I think this problem is still worth notification.

report-id: 250106-1639:7

PHP Version

latest version

Operating System

Debian 11

[iluuu1994]

Hi @Snape3058. Thanks for your report. Their usage is hidden behind a macro (ZCG(key) and ZCG(_key)). Luckily, we only use _key for sizeof() (sizeof(ZCG(_key))), but this looks like UB regardless.

[Snape3058]

According to some applied fixes to the linux kernel to similar issues,
https://lpc.events/event/18/contributions/1722/attachments/1591/3303/Wfamnae_lpceu2024.pdf

In this case, we can split the zend_string into a header-only version and a flexible array struct version. So that we can use the header-only version here if there is no such usages like ZCG(key).val.

Or, we can make zend_string an unsized flexible array. As the last element before the flexible array field is an 8-byte one, an unsized flexible array field will always aligned with the data following.

struct T {
	long unsigned int          len;                  /*     0     8 */
	char                       val[];                /*     8     0 */

	/* size: 8, cachelines: 1, members: 2 */
	/* last cacheline: 8 bytes */
};
struct S {
	struct T                   t;                    /*     0     8 */
	char                       a[32768];             /*     8 32768 */

	/* size: 32776, cachelines: 513, members: 2 */
	/* last cacheline: 8 bytes */
};

[cmb69]

This looks like a deliberate hack to me.

[iluuu1994]

It's indeed deliberate. But explicitly reading from padding is at the very least a bad idea. E.g. struct assignment is not guaranteed to copy padding bytes (C11, §6.2.6.2, footnote 51). I honestly don't know the C spec well enough to know what guarantees it provides (maybe @Snape3058 can answer that better).

In this case, we can split the zend_string into a header-only version and a flexible array struct version. So that we can use the header-only version here if there is no such usages like ZCG(key).val.

&ZCG(key) is casted to zend_string *, which will then for sure access zend_string.val, so this might violate strict aliasing rules.

Or, we can make zend_string an unsized flexible array.

I believe this is not an option because flexible arrays members are not part of the C++ standard, at least if we want to stay consistent between the two languages.

Honestly, I believe this is just a case of premature optimization. We can likely just allocate this string separately.

[Snape3058]

Thank you for the fix.