Fix stream_wrapper_unregister() resource leak

[iluuu1994]

Closes GH-8548

The changes are bigger than I would've liked to. Let me know if you'd like me to change anything, I'm open to suggestions. The approach of this PR:

• Make php_user_stream_wrapper.wrapper the first member so that we can cast between php_user_stream_wrapper and php_stream_wrapper
• Add a discriminator to php_stream_wrapper (INTERNAL or USER) so that we know which is which
• Store a pointer to the reference in php_user_stream_wrapper so that we can deallocate it when the user stream wrapper is removed

To actually make this testable, I had to introduce a AUTO_CLOSE_RESOURCE_LIST env variable that circumvents automatically closing all open resources at the end of the request, as that hides resource leaks. This way the leak is reported both with zmm and asan.

This PR is not ABI compatible, so it's targeting master.

[mvorisek]

To actually make this testable, I had to introduce a AUTO_CLOSE_RESOURCE_LIST env variable that circumvents automatically closing all open resources at the end of the request, as that hides resource leaks.

in zend_test ext any internal state can be returned and tested, or you can use memory_get_usage

[cmb69]

Thank you for the PR!

Indeed, we cannot do this for any of the stable branches, and given that we want to migrate to objects hopefully with PHP 9.0, I have doubts that this change is worth to be done; after all, there are many external extensions which have custom stream wrappers, and all these would need to be updated (possibly in non-trivial ways for BC reasons) for PHP 8.2, and then again for PHP 9.0.

I am not sure, though, whether the API break is necessary at all. Instead of introducing the php_stream_wrapper.type member, can't we just check whether .wops == &user_stream_wops? If that is given, we could cast (which requires the restructuring of the internal struct _php_user_stream_wrapper, but that "only" constitutes an ABI break, so extensions would not be affected at all).

[iluuu1994]

Instead of introducing the php_stream_wrapper.type member, can't we just check whether .wops == &user_stream_wops?

@cmb69 Oh, indeed! I'll make the necessary changes. Do you think the AUTO_CLOSE_RESOURCE_LIST makes sense? I'd also be happy with memory_get_usage() too (although we probably need more than one iteration then, as the first one will allocate certain things like the resource hashmap which will increase memory).

[cmb69]

The AUTO_CLOSE_RESOURCE_LIST might make sense if there are more similar issues to check, but for now I'd go with a test checking memory_get_usage(); even if that requires multiple (un)registrations, the test could be treated as slow test (SKIP_SLOW_TESTS).

[iluuu1994]

@cmb69 Thanks. I will investigate if AUTO_CLOSE_RESOURCE_LIST is useful in a separate PR. Unfortunately, 5 iterations were needed before memory stopped increasing. Why that particular number, I'm not sure. The test is very fast, so I don't think we need SKIP_SLOW_TESTS here.

[cmb69]

Unfortunately, 5 iterations were needed before memory stopped increasing. Why that particular number, I'm not sure.

It might even make sense to increase this number, just to be sure that the test works in all circumstances.

Anyhow, the PR looks good to me now. The only issue would be get_resources(), but that is already documented as debuging tool, with respective caveats.

[iluuu1994]

The only issue would be get_resources(), but that is already documented as debuging tool, with respective caveats.

Can you clarify? Do you mean get_resources() no longer containing the unregistered stream wrappers?

[cmb69]

The general problem with get_resources() is that you can access internal resources, and manipulate them at will. In this case, someone could store a stream factory resource in a variable, and when stream_wrapper_unregister() is called, the refcount assertion would fail (and maybe more serious issues could happen; see #1318). But as I said, this is nothing we should be really concerned about.

[iluuu1994]

Ah, of course. We can also delay the freeing until any reference to it goes away, that should work fine. I added the assert because I didn't think there was a way the refcount could increase.

[cmb69]

I added the assert because I didn't think there was a way the refcount could increase.

In my opinion, there shouldn't be a way. :)

[iluuu1994]

If you're ok with this I would still like to change it, as this could lead to memory corruption otherwise (even if get_resources() is essentially unsupported).

[cmb69]

Yeah, sure, feel free to improve. :)

[nikic]

Can't you use something more standard like zend_list_delete here?

[iluuu1994]

Ah, I didn't realize EG(regular_list) had a destructor attached to it that will free the resource. Thanks!

[cmb69]

phpitnernalsbook.com has a good chapter about resources.