FFICasterTest::testCastNonTrailingCharPointer relies on undefined behaviors #57387
Comments
|
I'll have a look at a fix with all your hints and details. Thank you Arnaud for this exhaustive bug report! |
This PR was merged into the 6.4 branch. Discussion ---------- [VarDumper] Fix FFI caster test | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix #57387 | License | MIT Fixes the invalid memory writing thanks to Arnaud's hints. Not easy to understand exactly what's the point of `testCastNonTrailingCharPointer`, so I hope the test is still valid like this. Commits ------- 8594b2f [VarDumper] Fix FFI caster test
|
This test was originally written to make sure that even when outputting non-C-strings with "garbage data" at the end, the var-dumper would not crash and at least do something, continuing to work correctly in most cases (That's why there was a comment informing that the test might not be stable). However, it seems to me that the problem is initially that the var-dumper tries to output |
Symfony version(s) affected
7.2
Description
I believe that the test FFICasterTest::testCastNonTrailingCharPointer
relies on undefined behavior, and will break depending on what is allocated just after
\FFI::cdef()->new('char['.$actualLength.']').The assignment
$pointer[$actualLength] = "\x01";writes 1 byte just after the memory block allocated by\FFI::cdef()->new('char['.$actualLength.']'). Valgrind detects this invalid write when running withUSE_ZEND_ALLOC=0 valgrind ./phpunit ...:Additionally, FFICaster::castFFIStringValue() treats
char*pointers a NUL-terminated C strings, but there is no guarantee that a NUL byte will be found near that address. As a result, in the test, the function will dump any data that happens to exist after$string. In theory this may also crash if this causes the function to reach an unreadable memory region.How to reproduce
Commit 25360ef24951f1c6b83f8bf85fbdcaff4a1a40e1 in php-src changes the memory layout slightly, which causes the test output to change:
(https://github.com/symfony/symfony/actions/runs/9497428991/job/26173977979)
Running any php version under valgrind reports an invalid write and read of size 1:
Possible Solution
Allocate more memory to make room for the
$pointer[$actualLength] = "\x01";. However, this assignment is not strictly necessary as the null byte after$actualMessageis not copied by\FFI::memcpy($pointer, $actualMessage, $actualLength).There is no way to prevent
FFICaster::castFFIStringValue()from dumping unrelated data, but if that's ok, it may be possible to prevent it from reaching unreadable memory and crashing (providing thatchar*points to a readable memory address) by not stepping over page boundaries. E.g. don't dump past$addr | (4096-1).It may be possible to find the size of the memory block if its allocated by zend_mm, and if
$addrpoints to the beginning of the block, by using the zend_mm API. E.g. useis_zend_mm() && is_zend_ptr($addr)to find if$addrpoints to the beginning of a block allocated by zend_mm, and_zend_mm_block_size()to find its size. However I do not necessarily recommend this.Additional Context
The issue was detected in #57379, and reported by @alexandre-daubois in php/php-src#14546 (comment)
The text was updated successfully, but these errors were encountered: