Looks like this is not upstream code, but something we're patching in ourselves:

This looks pretty bogus to me. pcre_get_compiled_regex_cache() uses BG(locale_string), not the actual locale that setlocale sets, so I suspect that the current setlocale call doesn't actually do anything at all as far as PCRE is concerned.

I think the correct way to fix this would be to extend the pcre_get_compiled_regex_cache API to accept the locale string as a parameter, so we can just directly pass the C locale without changing any global state.

I think the correct way to fix this would be to extend the pcre_get_compiled_regex_cache API to accept the locale string as a parameter, so we can just directly pass the C locale without changing any global state.

I could do that, if you don't mind?

But! There is another place where setlocale is used and it is not related to PCRE.

That still needs fixing. (We could theoretically use Intl extension but that will create unnecessary dependency and it will complicate code as whole). What is your decision on that account?

I think the correct way to fix this would be to extend the pcre_get_compiled_regex_cache API to accept the locale string as a parameter, so we can just directly pass the C locale without changing any global state.

I could do that, if you don't mind?

Feel free :) I would suggest not changing pcre_get_compiled_regex_cache() itself, but adding a new function like pcre_get_compiled_regex_cache_with_locale() and making the old function call it with BG(locale_string). ZSTR_CHAR('C') can be used to pass the C locale without allocating a string.

But! There is another place where setlocale is used and it is not related to PCRE.

php-src/ext/fileinfo/libmagic/readcdf.c

Lines 118 to 132 in 08d8623

	private const char *
	cdf_app_to_mime(const char *vbuf, const struct nv *nv)
	{
	size_t i;
	const char *rv = NULL;
	(void)setlocale(LC_CTYPE, "C");
	for (i = 0; nv[i].pattern != NULL; i++)
	if (strcasestr(vbuf, nv[i].pattern) != NULL) {
	rv = nv[i].mime;
	break;
	}
	(void)setlocale(LC_CTYPE, "");
	return rv;
	}

That still needs fixing. (We could theoretically use Intl extension but that will create unnecessary dependency and it will complicate code as whole). What is your decision on that account?

Though backing up the old locale would be an improvement, locale changes are still fundamentally not thread-safe, so it would be best to avoid it. We don't seem to have a ready-made function for strcasestr in particular, but an easy way to emulate it is to call zend_str_tolower_dup (which is not locale-sensitive) on both strings and use strstr. This can be made more efficient using stack-allocated strings, but this code does not look particularly performance-critical to me. Once we do that, we can also drop the strcasestr fallback implementation altogether. I think this issue would be best split into a separate PR from the PCRE one.

@turchanov can we get a status update here please ?

Sorry for late reply. Will look into that ASAP.

I originally created pull request against master branch by accident. So I rebased my PR against PHP-7.2 branch. Plus I decided to squash first commit (where I save current locale before setlocale("C")) since it is no longer relevant to the current solution and will clutter the commits history.

Belatedly noticed two cosmetic flaws. Pushed another commit. I can squash again to one commit if needed.

So.... What do you think?

So... Will you merge it?

Whoops, this dropped off my radar, again. Now merged as c62cd9a.