Fix alignment in zend_string_safe_alloc()/zend_string_safe_realloc()

[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.