Conversation
The large-run branch of zend_mm_realloc_heap() decodes the run length from the
page map entry and, in the in-place shrink case, releases the tail pages with a
direct zend_mm_bitset_reset_range():
old_size = ZEND_MM_LRUN_PAGES(info) * ZEND_MM_PAGE_SIZE;
...
zend_mm_bitset_reset_range(chunk->free_map, page_num + new_pages_count, rest_pages_count);
The reset reaches up to page_num + old_pages_count, and old_pages_count comes
straight from the 10-bit ZEND_MM_LRUN_PAGES field, so it can be as large as 1023
while a chunk only holds 512 pages. free_map is a 512-bit (8-word) bitset
followed in the chunk header by the map[] array, so an out-of-range count makes
the reset write past free_map into the header, turning a corrupted map entry
into an out-of-bounds write.
This is the same class as the bound recently added to zend_mm_free_pages_ex(),
but this path resets the bitset directly and never goes through that sink, so it
was left unprotected. The growth sub-branch just below already guards
page_num + new_pages_count <= ZEND_MM_PAGES before touching the bitset; the
shrink branch was simply asymmetric.
Add the check where the run length is decoded, so it covers the in-place, shrink
and grow sub-branches at once. A real large run always fits in its chunk, so
page_num + ZEND_MM_LRUN_PAGES(info) <= ZEND_MM_PAGES holds and the check only
fires on a corrupted heap. page_num and info are likely already in registers, so
it costs a test and a branch.
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.
The large-run branch of zend_mm_realloc_heap() decodes the run length from the page map entry and, in the in-place shrink case, releases the tail pages with a direct zend_mm_bitset_reset_range():
The reset reaches up to page_num + old_pages_count, and old_pages_count comes straight from the 10-bit ZEND_MM_LRUN_PAGES field, so it can be as large as 1023 while a chunk only holds 512 pages. free_map is a 512-bit (8-word) bitset followed in the chunk header by the map[] array, so an out-of-range count makes the reset write past free_map into the header, turning a corrupted map entry into an out-of-bounds write.
This is the same class as the bound recently added to zend_mm_free_pages_ex(), but this path resets the bitset directly and never goes through that sink, so it was left unprotected. The growth sub-branch just below already guards page_num + new_pages_count <= ZEND_MM_PAGES before touching the bitset; the shrink branch was simply asymmetric.
Add the check where the run length is decoded, so it covers the in-place, shrink and grow sub-branches at once. A real large run always fits in its chunk, so page_num + ZEND_MM_LRUN_PAGES(info) <= ZEND_MM_PAGES holds and the check only fires on a corrupted heap. page_num and info are likely already in registers, so it costs a test and a branch.