Fix array going away during sorting

[iluuu1994]

Fixes GH-16648

[dstogov]

You may try to move this locking code into asort() and others.

[iluuu1994]

We'll have to (re)move HT_ASSERT_RC1(ht); from zend_hash_sort_ex(), but I think this should work.

[dstogov]

moving HT_ASSERT_RC1(ht); is bad :(

[iluuu1994]

@dstogov We could introduce another variant of zend_hash_sort_ex() that omits it, which can be used explicitly for cases like this. We could also add HASH_FLAG_ALLOW_COW_VIOLATION, but since user-code may run during sorting this is likely worse.

[dstogov]

I think, zend_array_sort() with the new protection should work.

[iluuu1994]

Since this now adds new API, it's likely better to just fix in master.

[bwoebi]

@iluuu1994 Adding new API has never been a blocker. (Changing API, yes.) And it does fix a real bug.

[dstogov]

@iluuu1994 Adding new API has never been a blocker. (Changing API, yes.) And it does fix a real bug.

Actually, it's better not to add new API calls.
I suggest to remove ZEND_API for PHP-8.2/8.3 and re-add it for PHP-8.4 and master.
Everything else looks fine.

[iluuu1994]

Right. What I was referencing was this scenario: Extensions start using the new API, the user updates to the new extension without updating PHP, after which dynamic loading of the extension would fail. This can be prevented by the approach Dmitry described. Another upside is that the API may actually still be changed until master is eventually released, if we were to find some flaw in it.

[bukka]

It's ok to add a new API but probably a bit nicer for extension authors not to add it in if it's not necessary. So agree that keeping it internal in general might be better even though it's already in exported headers and it's just a hint for most users (all except Windows where ZEND_API actually matters). However there is extra bit in this case because we usually take inline functions in headers as part of the ABI so using function that is not part of the ABI in it might be a bit surprising

[iluuu1994]

all except Windows where ZEND_API actually matters

FWIW, ZEND_API matters for all extensions that are dynamically built. That would include basically all user extensions, and optional extensions on most distributions. In those cases, loading the extension will immediately fail, so it should be obvious that these functions shouldn't be used. Statically linked extensions should be able to use the functions without any problems.

Edit: And just as I'm writing this, I realized we shouldn't use this function for ext/intl/collator/collator_sort.c yet. 🙂