Skip to content

Don't leak changes to textdomain and friends across requests#6641

Closed
cmb69 wants to merge 1 commit intophp:PHP-7.4from
cmb69:cmb/gettext-reset
Closed

Don't leak changes to textdomain and friends across requests#6641
cmb69 wants to merge 1 commit intophp:PHP-7.4from
cmb69:cmb/gettext-reset

Conversation

@cmb69
Copy link
Member

@cmb69 cmb69 commented Jan 26, 2021

For textdomain(), this is trivial; we just retrieve the default value in MINIT, and store it in a true global, and set it in RINIT.

For bind_textdomain() it is not quite as trivial; we need a module global HashTable where we store all changed domains with their original directory, and reset everything in RINIT.

For bind_textdomain_codeset(), however, there seems to be no solution, since the default value of the codeset is NULL, but when passing NULL as codeset to bind_textdomain_codeset(3), the function returns the current codeset.

I'm not sure how to proceed with this. Any ideas?

Also we should probably document that ext/gettext is not thread safe …

@cmb69
Copy link
Member Author

cmb69 commented Feb 2, 2021

Any thoughts? Just leave it as it is, and document the issue?

@nikic
Copy link
Member

nikic commented Feb 2, 2021

My inclination here is probably "leave it as is", especially in conjunction with your thread-safety comment. As far as I can tell, gettext() is actually thread-safe in that, say, calling textdomain() from multiple threads will not result in undefined behavior -- the global state modification occurs behind a global lock.

This means that using gettext in a multi-threaded context is still "safe" if all threads call textdomain() (or other functions) with the same value. However, if we were to start restoring values on RINIT, then threads would randomly see the value change, even if they have consistent calls to textdomain().

There's really no good way to handle this, as the underlying library is simply bad, but sticking with the current behavior seems like the least bad approach at least.

php-pulls pushed a commit that referenced this pull request Feb 8, 2021
gettext leaks global state across requests, so don't repeat these
tests. See also GH-6641.
php-pulls pushed a commit to php/doc-en that referenced this pull request Feb 8, 2021
@cmb69
Copy link
Member Author

cmb69 commented Feb 8, 2021

Ah, right!

Documented as php/doc-en@59e6c12.

@cmb69 cmb69 closed this Feb 8, 2021
@cmb69 cmb69 deleted the cmb/gettext-reset branch February 8, 2021 09:30
php-pulls pushed a commit to php/doc-ja that referenced this pull request Feb 8, 2021
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