#255: allocate the literals the way the platform frees them - #258
Merged
Conversation
op_array_to_emalloc laid the literals inside the opcode block. That is what pass_two() leaves on 64-bit, where destroy_op_array frees only the opcode block. Under ZEND_USE_ABS_CONST_ADDR the compiler keeps the two apart and destroy_op_array frees op_array->literals on its own, so the allocator got an interior pointer and the block was freed twice. Reproduced on x86-64 by forcing ZEND_USE_ABS_JMP_ADDR and ZEND_USE_ABS_CONST_ADDR to 1: 35 of 102 thread_pool tests failed before the change, none after. #251 made the copy unconditional, which is why the count is that high.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Closes #255.
What was wrong
op_array_to_emallocallocated opcodes and literals as one block and pointedop_array->literalsat the tail of it. That is the layoutpass_two()leaves on 64-bit, wheredestroy_op_arrayfrees onlyop_array->opcodes.Under
ZEND_USE_ABS_CONST_ADDR—SIZEOF_SIZE_T == 4, i.e. 32-bit — the compiler keeps the two in separate allocations, anddestroy_op_arrayfrees both:So on 32-bit every materialized closure with at least one literal handed the allocator a pointer into the middle of a block it never issued, and then freed the block a second time. The opline fix-up already branched on the macro; the allocation did not.
#251 made
op_array_to_emallocunconditional, which turned this from some closures into every closure that reaches a worker.What changed
The copy follows the same rule as the compiler on both platforms: one block under
!ZEND_USE_ABS_CONST_ADDR, whereRT_CONSTANTaddresses the literal by anint32_toffset from the opline and separate allocations would overflow it, two blocks otherwise.Verification
No 32-bit runner exists here, so the configuration was reproduced on x86-64 by forcing
ZEND_USE_ABS_JMP_ADDRandZEND_USE_ABS_CONST_ADDRto 1 inZend/zend_compile.hand rebuilding the engine. In that build:thread_pooltests fail;In the ordinary 64-bit build the layout is unchanged:
tests1191 passed / 0 failed,fuzzy-tests/_generated724 / 0. The one warning is the stale--XFAIL--on030, removed by #256.Build: ZTS DEBUG, TrueAsync ABI v0.24.0.