symfony: FFICasterTest::testCastNonTrailingCharPointer() breaks PHPs community build

Symfony version(s) affected

6.2

Description

https://github.com/symfony/symfony/blob/e6d6bedc8466c88d639395aa0ef1728deb1e30da/src/Symfony/Component/VarDumper/Tests/Caster/FFICasterTest.php#L194-L205

See https://github.com/iluuu1994/php-src/actions/runs/3106907448/jobs/5034370701.

How to reproduce

Possible Solution

The NUL byte should be part of the array, otherwise this is writing to unrelated memory. The memory sanitizer complains about $pointer[$actualLength] = "\x01";, because the assignment happens to unowned memory. The string should be allocated as $string = \FFI::new('char['.($actualLength + 1).']'); to allow this, although that likely defeats the purpose of the test as there is probably another NUL byte right after. Printing char* is likely unsafe because user allocated C strings are not guaranteed to be NUL-terminated. Just reading from the unowned memory is also unsafe and can cause SEGFAULTs.

Additional Context

No response

About this issue

Original URL
State: closed
Created 2 years ago
Comments: 21 (17 by maintainers)

Most upvoted comments

We could add @group ffi on these tests and run the community builds with --exclude-group ffi on the PHP side?

Does var_dump also crash on the same data? print_r? Because if not, we could leverage them? About filtering such values to prevent a crash, casters accept a 5th argument that should be used for that (see eg ReflectionCaster::castClosure(). We could add a new filtering const as Caster::EXCLUDE_something.

The filter can be given when calling AbstractCloner::cloneVar(). This is not readdly comfigurable from the outside yet but #48667 might be a nice way forward for this.

nicolas-grekas on Jan 19, 2023

In the case of dump() segfaults can happen even if the data you’re dumping is completely valid.

In this case, the var_dump only reads and displays the first char. You gave me an idea. I can do (send PR with this feature I mean) the same. Then it will be another option to solve the problem.

SerafimArts on Sep 24, 2022

@SerafimArts Thanks for your response!

However, using var_dump behaves similarly in such cases (can access to “foreign memory” to display data).

True. However in this case, at least the pointer is pointing to invalid memory. In the case of dump() segfaults can happen even if the data you’re dumping is completely valid.

That is, this “mistake” was deliberately made there with the hope that a piece of zval structure would be read, and not someone else’s process

Since PHP runs in user space there is not really a concern of reading the memory of another process (all memory addresses are local, only the kernel can refer to global addresses) but rather accessing “unmapped” addresses which don’t necessarily have to be far away from the address you’re reading. In most cases you’ll probably be “lucky” and read data from the same page or PHP will have already allocated the page next to it. However:

The neighboring data it is not guaranteed to contain a NUL byte, reading further off bounds increases the chances for a segfault
The neighboring address might be on a page that gets unmapped at a later point in time, which will cause a segfault
The data you’re reading by chance might be sensitive. While it’s rather unlikely for this code to be used on production systems it is not impossible.

Unfortunately, I don’t think there’s a good way to solve that. I doubt most people would expect that dump()ing some CData could cause a segfault. It’s a bummer because it’s a really convenient feature.

iluuu1994 on Sep 25, 2022