[realFlowControl]

On x86_64 a zend_string is a 24-byte header, then the characters, then \0. zend_string_alloc() rounds that size up to a multiple of 8: ALIGN(24 + len + 1)

zend_string_safe_alloc() and zend_string_safe_realloc() should do the same, but they round up only part of the size and add the rest on top: n * m + ALIGN(24 + l + 1)

So these strings can be up to 7 bytes too small. That matters because the x86_64 asm in zend_string_equal_val() compares 8 bytes at a time. It expects the rounded size and reads up to that end. The same problem exists in the 32-bit x86 asm, which reads 4 bytes at a time.

Example

implode(",", ["abc", "d", "efg"]); // "abc,d,efg", 9 chars

implode() calls zend_string_safe_alloc(2, 1, 7): 2 separators of 1 byte each, plus 7 bytes of pieces.

So we need 34 bytes of memory (24 (header) + 9 (chars) + 1 (NULL)). In current master we do 2 * 1 + ALIGN(24 + 7 + 1) = 34 (24 + 7 + 1 = 32 is already a multiple of 8, but adding the two separator bytes gives 34, which is not). What we'd expect and what zend_string_equal_val() relies on is 40 bytes: ALIGN(2 * 1 + 24 + 7 + 1).

So the asm in zend_string_equal_val() reads 6 bytes past the end of the block.

Why did we not notice

The extra bytes are almost always there anyway:

• ZendMM puts 34 bytes in the 40-byte bin
• glibc malloc (USE_ZEND_ALLOC=0) gives 40 usable bytes for a 34-byte request
• ASAN does not check inline asm
• Valgrind replaces zend_string_equal_val() with a memcmp version

Reproducer

Electric Fence puts each allocation at the end of a page, followed by a page we can't read, forcing a segfault if we do.

docker run --rm -ti --platform linux/amd64 php:8.5-cli bash
# inside the container
apt-get update -qq && apt-get install -y -qq electric-fence
cat > /t.php <<"EOF"
<?php
$a = implode(",", ["abc", "d", "efg"]);
$b = implode(",", ["abc", "d", "efg"]);
var_dump($a === $b);
EOF
USE_ZEND_ALLOC=0 EF_ALIGNMENT=1 LD_PRELOAD=/usr/lib/libefence.so php -n /t.php

Checking the resulting core file with gdb:

Core was generated by `/usr/local/bin/php '' '''.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x0000004000820d1b in zend_string_equal_val ()

Bonus

Less memory: For example a str_repeat("x", 25) to str_repeat("x", 31) now use a 56-byte bin instead of the 64 byte bin

Thanks @morrisonlevi for pointing this out.

[morrisonlevi]

There are a bunch of failing tests, maybe rebase or merge in the latest master? Not sure why the msan job failed in particular, this branch shouldn't have affected these results, I think.

That aside, the fix looks correct. ZEND_MM_ALIGNMENT - 1 will add +7 (64-bit), and then & ZEND_MM_ALIGNMENT_MASK clears the lower 3 bits (64-bit), which throws away extra. For example 51+7 = 58 which is 0b111010, then clearing the lower 3 bits makes it 0b111000 which is 56.

Theoretically the addition could overflow but I do not see how it could in practice (and this theoretical concern exists in the previous code as well).

I'll also re-iterate that on "real" production systems the problem fixed by this PR isn't an issue. Anyone replacing ZMM will typically need to conform to alignment requirements, and since this is C where alignment isn't an explicit parameter, it probably needs to use alignof(max_align_t) for allocations which are 9+ bytes. Nonetheless, I think it's good hygiene to actually request the amount of bytes you intend to read, so we should make sure we actually request it, rather than rely on the lower level allocator to provide it.

[ndossche]

Incredible find.
This problem may exist in other places too where allocation is used directly in combination with _ZSTR_STRUCT_SIZE, did you check that?

[realFlowControl]

This problem may exist in other places too where allocation is used directly in combination with _ZSTR_STRUCT_SIZE, did you check that?

I did not, but checked just now and found another spot in the mbstring extension:

docker run --rm -ti --platform linux/amd64 php:8.5-cli bash
# inside the container
apt-get update -qq && apt-get install -y -qq electric-fence
cat > /t.php <<"EOF"
<?php
foreach (["abcdefghi", "\xE9"] as $input) {
  $a = mb_convert_encoding($input, "UTF-8", "ISO-8859-1");
  $b = mb_convert_encoding($input, "UTF-8", "ISO-8859-1");
  var_dump($a === $b);
}
EOF
USE_ZEND_ALLOC=0 EF_ALIGNMENT=1 LD_PRELOAD=/usr/lib/libefence.so php -n /t.php

abcdefghi segfaults because mb_convert_buf_init()

php-src/ext/mbstring/libmbfl/mbfl/mbfl_encoding.h

Line 158 in 3571d36

buf->str = emalloc(_ZSTR_STRUCT_SIZE(initsize));

does not round up to 8.
The string "\xE9" takes the growth path in MB_CONVERT_BUF_ENSURE

php-src/ext/mbstring/libmbfl/mbfl/mbfl_encoding.h

Line 170 in 3571d36

zend_string *newstr = erealloc((buf)->str, _ZSTR_STRUCT_SIZE(newsize)); \

and also segfaults, because it does not request a size divisible by 8. I mean the script above segfaults in the first iteration already, but you get the point.

OPcache and JIT wrap _ZSTR_STRUCT_SIZE in ZEND_MM_ALIGNED_SIZE and ZEND_MM_ALIGNED_SIZE_EX macros, for example:

php-src/ext/opcache/jit/zend_jit_helpers.c

Line 1802 in 3571d36

result_str = perealloc(Z_STR_P(op1), ZEND_MM_ALIGNED_SIZE(_ZSTR_STRUCT_SIZE(result_len)), 0);

php-src/ext/opcache/ZendAccelerator.c

Line 430 in 3571d36

ZEND_MM_ALIGNED_SIZE_EX(_ZSTR_STRUCT_SIZE(ZSTR_LEN(s)) + sizeof(zend_string_table_pos_t), ZEND_STRING_TABLE_POS_ALIGNMENT)

they are fine.

[ndossche]

CI test failures seem related

[realFlowControl]

CI test failures seem related

Jup, looks like it. I'm on vacation now, will have a look in a week and ping you once it's fixed. Thanks for having a look 🙇

[realFlowControl]

Not completely sure I like the fix in 814300a myself. zend_mm_delayed_error_or_panic() currently aborts when its first argument is false, so the ZEND_UNREACHABLE() is correct today. But we are expressing that guarantee in the caller rather than in the error-reporting API. If the handler ever returned on that path, reaching ZEND_UNREACHABLE() would be undefined behaviour in release builds, we could instead call abort() and make CI fail loud 😉

We cannot mark the existing function itself ZEND_NORETURN, because its delayed-error branch does return.

The compiler does not see that passing in false makes that function not return, it only sees the declaration in the included header and that has no ZEND_NORETURN, so from the compilers PoV it might return.

@ndossche could you have a look please?

[ndossche]

I think this is cleaner and should handle related overflow issues:

diff --git a/Zend/zend_alloc.c b/Zend/zend_alloc.c
index 2b52babc98e..d0c6f751e51 100644
--- a/Zend/zend_alloc.c
+++ b/Zend/zend_alloc.c
@@ -1825,6 +1825,10 @@ static zend_never_inline void *zend_mm_realloc_huge(zend_mm_heap *heap, void *pt
 #if ZEND_DEBUG
 	real_size = size;
 	size = ZEND_MM_ALIGNED_SIZE(size) + ZEND_MM_ALIGNED_SIZE(sizeof(zend_mm_debug_info));
+	if (UNEXPECTED(size < real_size)) {
+		zend_mm_delayed_error_or_panic(heap->fallible, "Possible integer overflow in memory allocation (%zu + %zu)", ZEND_MM_ALIGNED_SIZE(real_size), ZEND_MM_ALIGNED_SIZE(sizeof(zend_mm_debug_info)));
+		return NULL;
+	}
 #endif
 	if (size > ZEND_MM_MAX_LARGE_SIZE) {
 #if ZEND_DEBUG
@@ -1835,10 +1839,15 @@ static zend_never_inline void *zend_mm_realloc_huge(zend_mm_heap *heap, void *pt
 		 * We allocate them with 2MB size granularity, to avoid many
 		 * reallocations when they are extended by small pieces
 		 */
-		new_size = ZEND_MM_ALIGNED_SIZE_EX(size, MAX(REAL_PAGE_SIZE, ZEND_MM_CHUNK_SIZE));
+		size_t alignment = MAX(REAL_PAGE_SIZE, ZEND_MM_CHUNK_SIZE);
 #else
-		new_size = ZEND_MM_ALIGNED_SIZE_EX(size, REAL_PAGE_SIZE);
+		size_t alignment = REAL_PAGE_SIZE;
 #endif
+		new_size = ZEND_MM_ALIGNED_SIZE_EX(size, alignment);
+		if (UNEXPECTED(new_size < size)) {
+			zend_mm_delayed_error_or_panic(heap->fallible, "Possible integer overflow in memory allocation (%zu + %zu)", size, alignment);
+			return NULL;
+		}
 		if (new_size == old_size) {
 #if ZEND_DEBUG
 			zend_mm_change_huge_block_size(heap, ptr, new_size, real_size ZEND_FILE_LINE_RELAY_CC ZEND_FILE_LINE_ORIG_RELAY_CC);
@@ -1920,6 +1929,10 @@ static zend_always_inline void *zend_mm_realloc_heap(zend_mm_heap *heap, void *p
 		size_t real_size = size;
 
 		size = ZEND_MM_ALIGNED_SIZE(size) + ZEND_MM_ALIGNED_SIZE(sizeof(zend_mm_debug_info));
+		if (UNEXPECTED(size < real_size)) {
+			zend_mm_delayed_error_or_panic(heap->fallible, "Possible integer overflow in memory allocation (%zu + %zu)", ZEND_MM_ALIGNED_SIZE(real_size), ZEND_MM_ALIGNED_SIZE(sizeof(zend_mm_debug_info)));
+			return NULL;
+		}
 #endif
 
 		ZEND_MM_CHECK(chunk->heap == heap, "zend_mm_heap corrupted");
diff --git a/Zend/zend_string.h b/Zend/zend_string.h
index 5bd805aa8dd..bb7da1d5410 100644
--- a/Zend/zend_string.h
+++ b/Zend/zend_string.h
@@ -131,6 +131,16 @@ static zend_always_inline zend_string *ZSTR_KNOWN(size_t idx) {
 #define ZSTR_MAX_OVERHEAD (ZEND_MM_ALIGNED_SIZE(_ZSTR_HEADER_SIZE + 1))
 #define ZSTR_MAX_LEN (SIZE_MAX - ZSTR_MAX_OVERHEAD)
 
+/* Offset for safe_*alloc(n, m, offset) to size the string like zend_string_alloc(n * m + l). */
+static zend_always_inline size_t zend_string_safe_offset(size_t n, size_t m, size_t l)
+{
+	if (UNEXPECTED(l > ZSTR_MAX_LEN)) {
+		return SIZE_MAX; /* Rejected by the allocator. */
+	}
+	/* Works even if n * m wraps, allocator detects that overflow. */
+	return ZEND_MM_ALIGNED_SIZE(_ZSTR_STRUCT_SIZE(n * m + l)) - n * m;
+}
+
 #define ZSTR_ALLOCA_ALLOC(str, _len, use_heap) do { \
 	(str) = (zend_string *)do_alloca(ZEND_MM_ALIGNED_SIZE_EX(_ZSTR_STRUCT_SIZE(_len), 8), (use_heap)); \
 	GC_SET_REFCOUNT(str, 1); \
@@ -209,7 +219,7 @@ static zend_always_inline zend_string *zend_string_alloc(size_t len, bool persis
 /* Fallible variant of zend_string_safe_alloc */
 static zend_always_inline zend_string *zend_string_safe_falloc(size_t n, size_t m, size_t l, bool persistent, bool fallible)
 {
-	zend_string *ret = (zend_string *)safe_fpemalloc(n, m, ZEND_MM_ALIGNED_SIZE(_ZSTR_STRUCT_SIZE(l)), persistent, fallible);
+	zend_string *ret = (zend_string *)safe_fpemalloc(n, m, zend_string_safe_offset(n, m, l), persistent, fallible);
 	if (ZEND_CONST_COND(fallible, true) && UNEXPECTED(ret == NULL)) {
 		return ret;
 	}
@@ -394,7 +404,7 @@ static zend_always_inline zend_string *zend_string_safe_frealloc(zend_string *s,
 
 	if (!ZSTR_IS_INTERNED(s)) {
 		if (GC_REFCOUNT(s) == 1) {
-			ret = (zend_string *)safe_fperealloc(s, n, m, ZEND_MM_ALIGNED_SIZE(_ZSTR_STRUCT_SIZE(l)), persistent, fallible);
+			ret = (zend_string *)safe_fperealloc(s, n, m, zend_string_safe_offset(n, m, l), persistent, fallible);
 			if (ZEND_CONST_COND(fallible, true) && !ret) {
 				return NULL;
 			}
diff --git a/ext/mbstring/libmbfl/mbfl/mbfl_encoding.h b/ext/mbstring/libmbfl/mbfl/mbfl_encoding.h
index e71737318bf..7d2aab7cf33 100644
--- a/ext/mbstring/libmbfl/mbfl/mbfl_encoding.h
+++ b/ext/mbstring/libmbfl/mbfl/mbfl_encoding.h
@@ -155,7 +155,7 @@ typedef zend_string* (*mb_cut_fn)(unsigned char *str, size_t from, size_t len, u
 static inline void mb_convert_buf_init(mb_convert_buf *buf, size_t initsize, uint32_t repl_char, unsigned int err_mode)
 {
 	buf->state = buf->errors = 0;
-	buf->str = emalloc(_ZSTR_STRUCT_SIZE(initsize));
+	buf->str = emalloc(ZEND_MM_ALIGNED_SIZE(_ZSTR_STRUCT_SIZE(initsize)));
 	buf->out = (unsigned char*)ZSTR_VAL(buf->str);
 	buf->limit = buf->out + initsize;
 	buf->replacement_char = repl_char;
@@ -167,8 +167,8 @@ static inline zend_result mb_convert_buf_ensure(mb_convert_buf *buf, unsigned ch
 	ZEND_ASSERT(*out <= *limit);
 	if ((size_t)(*limit - *out) < needed) {
 		size_t oldsize = *limit - (unsigned char*)ZSTR_VAL((buf)->str);
-		size_t newsize = oldsize + MAX(oldsize >> 1, needed);
-		zend_string *newstr = safe_ferealloc((buf)->str, 1, newsize, _ZSTR_STRUCT_SIZE(0), /* fallible */ true);
+		size_t growth = MAX(oldsize >> 1, needed);
+		zend_string *newstr = safe_ferealloc((buf)->str, 1, oldsize, zend_string_safe_offset(1, oldsize, growth), /* fallible */ true);
 		if (!newstr) {
 			/* An earlier growth may have moved the buffer. */
 			buf->out = *out;
@@ -176,7 +176,7 @@ static inline zend_result mb_convert_buf_ensure(mb_convert_buf *buf, unsigned ch
 			return FAILURE;
 		}
 		*out = (unsigned char*)ZSTR_VAL(newstr) + (*out - (unsigned char*)ZSTR_VAL((buf)->str));
-		*limit = (unsigned char*)ZSTR_VAL(newstr) + newsize;
+		*limit = (unsigned char*)ZSTR_VAL(newstr) + oldsize + growth;
 		(buf)->str = newstr;
 	}
 	return SUCCESS;
diff --git a/ext/standard/tests/strings/chunk_split_variation2_32bit.phpt b/ext/standard/tests/strings/chunk_split_variation2_32bit.phpt
index c83a37f00ed..1f378a739d5 100644
--- a/ext/standard/tests/strings/chunk_split_variation2_32bit.phpt
+++ b/ext/standard/tests/strings/chunk_split_variation2_32bit.phpt
@@ -16,4 +16,4 @@
 --EXPECTF--
 *** Testing chunk_split() : unexpected large 'end' string argument variation 2 ***
 
-Fatal error: Possible integer overflow in memory allocation (65537 * 65537 + %r65556|65560%r) in %s on line %d
+Fatal error: Possible integer overflow in memory allocation (65537 * 65537 + %d) in %s on line %d