[VarDumper] Fix FFI caster test #57397
Conversation
alexandre-daubois
force-pushed
the
ffi-caster-fix
branch
from
b42a137
to
bf76480
Compare
|
cc @xabbuh this fixes CI on 8.4. I don't know if you've seen it, but just in case so you don't pull your hair out trying to figure out failure 😄 |
|
@alexandre-daubois 👍 thanks, I have seen it but I have no idea around this code. That's why I let @nicolas-grekas do the review here. :) |
|
I don't have the knowledge to review this PR. |
| $pointer = \FFI::addr($string[0]); | ||
|
|
||
| \FFI::memcpy($pointer, $actualMessage, $actualLength); | ||
|
|
||
| // Remove automatically addition of the trailing "\0" and remove trailing "\0" |
There was a problem hiding this comment.
This comment can be removed as it's incorrect
| $ffi = \FFI::cdef(<<<C | ||
| bool is_zend_mm(void); | ||
| bool is_zend_ptr(const void *ptr); | ||
| C); | ||
|
|
||
| for ($i = 0; $i < self::MAX_STRING_LENGTH; ++$i) { | ||
| $result[$i] = $data[$i]; | ||
| if ($ffi->is_zend_ptr($data) && $ffi->is_zend_mm()) { | ||
| $result[$i] = $data[$i]; | ||
|
|
||
| if ("\0" === $result[$i]) { | ||
| return implode('', $result); | ||
| if ("\0" === $data[$i]) { | ||
| return implode('', $result); | ||
| } | ||
| } else { | ||
| break; |
There was a problem hiding this comment.
While testing I found that is_zend_ptr() is broken and may crash in some conditions. So my suggestion of using is_zend_ptr() is not good, sorry.
A safe alternative is to ensure that you only access memory in the same page as $data[0]: (it's safe as long as accessing $data[0] is safe)
$ffi = \FFI::cdef(<<<C
size_t zend_get_page_size(void);
C);
$pagesize = $ffi->zend_get_page_size();
/* Get the address of $data */
$start = $ffi->cast("uintptr_t", $ffi->cast("char*", $data))->cdata;
/* Accessing memory in the same page as $start is safe */
$max = min(self::MAX_STRING_LENGTH, ($start | ($pagesize - 1)) - $start);
for ($i = 0; $i < $max; ++$i) {
$result[$i] = $data[$i];
if ("\0" === $data[$i]) {
return implode('', $result);
}
}There was a problem hiding this comment.
I couldn't find a proper way to get the page size. This looks good, thanks Arnaud!
| public function testCastNonTrailingCharPointer() | ||
| { | ||
| $actualMessage = 'Hello World!'; | ||
| $actualLength = \strlen($actualMessage); | ||
|
|
||
| $string = \FFI::cdef()->new('char['.$actualLength.']'); | ||
| $string = \FFI::cdef()->new('char['.($actualLength + 1).']'); |
alexandre-daubois
force-pushed
the
ffi-caster-fix
branch
from
bf76480
to
7a8c5b3
Compare
alexandre-daubois
force-pushed
the
ffi-caster-fix
branch
from
7a8c5b3
to
8594b2f
Compare
|
Thank you @alexandre-daubois. |
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.