Fix alignment in zend_string_safe_alloc()/zend_string_safe_realloc() - #23869
Draft
realFlowControl wants to merge 2 commits into
Draft
realFlowControl wants to merge 2 commits into
realFlowControl wants to merge 2 commits into
Conversation
realFlowControl
force-pushed
the
florian/zstr-safe-alloc-align
branch
from
September 24, 2026 07:05
e779de2 to
49938ad
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On x86_64 a
zend_stringis 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()andzend_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()callszend_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
masterwe do2 * 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 whatzend_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:
USE_ZEND_ALLOC=0) gives 40 usable bytes for a 34-byte requestzend_string_equal_val()with amemcmpversionReproducer
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.
Checking the resulting core file with
gdb:Bonus
Less memory:
str_repeat("x", 25)tostr_repeat("x", 31)now use a 56-byte block instead of 64.Thanks @morrisonlevi for pointing this out.