/cc @SerafimArts any advice here? could anyone please send a fix?

@nicolas-grekas why was this closed ? I can't find any related code change

Probably by accident. I sent a different fix for the FFI test. 🙂

Oops :)

What would be the preferred solution here? To fully remove char* support or to just remove the test? I can have a look.

Yes this is true. In some cases, string rendering can fit into "foreign memory" and cause a SIGSEG/etc errors. This is a rather rare case, but I have already encountered it.

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

var_dump(\FFI::cast('ptrdiff_t*', 0xDEAD_BEEF));

// Process finished with exit code 139 (interrupted by signal 11: SIGSEGV)

It is impossible to solve this problem in a simple way, so I offer two options:

1. Remove string (char*) rendering (cause we can't make sure the string ends by "\0").
2. Add support for environment variables to display "unsafe" variables, like:

if ($_SERVER['SYMFONY_VAR_DUMPER_FFI_STRINGS'] ?? false) {
    // Usafe accessing
}

Tests for the behavior of displaying such values will also have to be removed.

P.S. In theory, we can check that a string is safe by calling smth like new Process(['php', '-r', '<?php \FFI::string(' . $ptr . ')']). But it seems like an overengineering =)

Just reading from the unowned memory is also unsafe and can cause SEGFAULTs.

Yep. It just checks the behavior when the string does not end with "\0" and the dumper gets into non-string memory.

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

@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.

In most cases you'll probably be "lucky"

Yes, that's why I added a comment there that this test might break =(

@nicolas-grekas Are you satisfied with the PR of removing tests for displaying string vars (char*) with the addition of reading the env variable (like SYMFONY_VAR_DUMPER_FFI_STRINGS), when the developer wants to see even "unsafe" data that can lead to segfault?

Or reading the environment vars inside the FFICaster is not the right way to fine-tune it?

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.

Printing the first char should be fine memory wise. Thank you!

But the functionality with displaying full strings, although not always safe, is convenient. I don't want to get rid of it completely.

In general, if @nicolas-grekas approves of such an option (or suggests a more symfony-way), then I would prefer to leave it with the ability to somehow turn it on leaving the default behavior with displaying only the first character.

If I can disable it in the PHP community build I'm happy. 🙂 Of course, I can also just disable the entire test but I thought I'd let you know that this triggered an error in our build.

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.

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

Yes! This is pretty much what I've done in our CI for now.

https://github.com/php/php-src/blob/bff7a56d0008c0915977048d675860a4ad04e85c/.github/workflows/nightly.yml#L349

Does var_dump also crash on the same data? print_r? Because if not, we could leverage them?

The problem is that the length of the string is unknown so we can't safely construct a PHP string either.

As I understand that it makes sense to wait for the application of the PR #48667 and continue to use this functionality (of subj PR) to control the output of unsafe char* data.

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

We ignored this test on our side, so feel free to close this issue if you don't want to fix it.

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

Friendly reminder that this issue exists. If I don't hear anything I'll close this.

Hey,

I didn't hear anything so I'm going to close it. Feel free to comment if this is still relevant, I can always reopen!