Skip to content

Conversation

DimitryAndric
Copy link
Contributor

@DimitryAndric DimitryAndric commented May 21, 2021

Fix for https://bugs.php.net/bug.php?id=81068.

If ZTS is enabled, this can cause cwd_globals_ctor() to be called multiple times, each with a freshly allocated virtual_cwd_globals instance. At shutdown time however, cwd_globals_dtor() will call realpath_cache_clean(), which then possibly cleans up the same realpath_cache instance more than once. Using AddressSanitzer, this shows up as a heap use-after-free.

To avoid this, add a helper function to do the actual work on one instance of a realpath_cache, and call it both from cwd_globals_dtor() and realpath_cache_clean(). The former uses the virtual_cwd_globals parameter passed in via the destructor, the latter uses the CWDG() macro.

If ZTS is enabled, this can cause cwd_globals_ctor() to be called
multiple times, each with a freshly allocated virtual_cwd_globals
instance. At shutdown time however, cwd_globals_dtor() will call
realpath_cache_clean(), which then possibly cleans up the same
realpath_cache instance more than once. Using AddressSanitzer, this
shows up as a heap use-after-free.

To avoid this, add a helper function to do the actual work on one
instance of a realpath_cache, and call it both from cwd_globals_dtor()
and realpath_cache_clean(). The former uses the virtual_cwd_globals
parameter passed in via the destructor, the latter uses the CWDG()
macro.
@DimitryAndric
Copy link
Contributor Author

Alternative approach in 9d38056, but this required adding a ZEND_TSRMG_FAST_BULK macro to zend.h, and is somewhat more invasive. The end result looks a bit nicer, though.

Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks reasonable for now, though it's rather annoying that you can't use normal global access in globals_dtor, as it may get run from a different thread :/

@nikic
Copy link
Member

nikic commented May 25, 2021

Merged as 99a2085.

@nikic nikic closed this May 25, 2021
@DimitryAndric
Copy link
Contributor Author

I think this looks reasonable for now, though it's rather annoying that you can't use normal global access in globals_dtor, as it may get run from a different thread :/

I guess this is because realpath_cache_clean() is an externally available function? I.e. it is called from ext/opcache/ZendAccelerator.c and ext/standard/filestat.c. These can have any random thread context, so then you'll have to use CWDG(), at least if I understand correctly. But there are probably historical reasons why this function exists. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants