Fix GH-17951: memory_limit ini value not being overwritten by max_memory_limit

[manuelm]

Make sure to always duplicate max_memory_limit ini value. Otherwise the alter ini routine may assume the value hasn't been overwritten, resulting in the user-specified value being set after the on_modify handler has run.

[iluuu1994]

Thanks for the patch. This was a bit short-sighted of me, I thought we could avoid breaking the ZEND_INI_MH API. In hindsight, it might have been better to add a bool *modified parameter to ZEND_INI_MH(), given it's abstracted behind a macro anyway. I think this patch is ok (I'll have to check how GH-19964 is related first), but I'd still like to check with @php/release-managers-85 whether such an API break behind an abstraction is ok. on_modify() is not called in too many places. I can create a patch first so this decision is easier to make.

To clarify for the RMs, as part of GH-17951 I added this code:

php-src/Zend/zend_ini.c

Lines 406 to 411 in 65b9306

	/* Skip assigning the value if the handler has already done so. */
	if (ini_entry->value == prev_value) {
	ini_entry->value = duplicate;
	} else {
	zend_string_release(duplicate);
	}

which skips assignment of the ini-value if on_modify has already assigned a new value. However, this fails to handle the case where the current value should stay the same, with the new one being discarded. If on_modify could instead set *modified = true;, the caller would know not to update the ini value.

[manuelm]

Yes, a signature change would be the superior solution.

[DanielEScherzer]

Thanks for the patch. This was a bit short-sighted of me, I thought we could avoid breaking the ZEND_INI_MH API. In hindsight, it might have been better to add a bool *modified parameter to ZEND_INI_MH(), given it's abstracted behind a macro anyway. I think this patch is ok (I'll have to check how GH-19964 is related first), but I'd still like to check with @php/release-managers-85 whether such an API break behind an abstraction is ok. on_modify() is not called in too many places. I can create a patch first so this decision is easier to make.

8.5 has already branched - are you suggesting that this patch should target 8.5 rather than master?

[iluuu1994]

The question is whether a change to ZEND_INI_MH() is acceptable in 8.5, which shouldn't but still may break extensions, iff they call on_modify(). In php-src, there's only one call from php-fpm outside of Zend/main.

But I'm happy to merge this fix for now and make the aforementioned improvements in master instead.

[DanielEScherzer]

The question is whether a change to ZEND_INI_MH() is acceptable in 8.5, which shouldn't but still may break extensions, iff they call on_modify(). In php-src, there's only one call from php-fpm outside of Zend/main.

But I'm happy to merge this fix for now and make the aforementioned improvements in master instead.

Is this a fix for something that broke in 8.5? If the problematic behavior predates 8.5 we should probably leave the fix for master, but I'll check with @edorian

[iluuu1994]

Is this a fix for something that broke in 8.5?

Effectively, yes. The lines I linked above were added in 8.5, but contain an issue that is cleanest to solve with the small API break. It's solvable without it, but requires an extra allocation.

Just to avoid confusion, I'm not asking for permission for this patch. This one has no API break. Adjusting ZEND_INI_MH() would have an API break for anybody calling on_modify() directly. Let me just create the patch so we have something concrete to discuss.

[iluuu1994]

master...iluuu1994:php-src:gh-19963-alt

Nevermind, this turned out to be way more involved since I didn't consider that all the calls to OnUpdate(String|Bool|Long|etc.) need to be updated. I'd prefer this patch in that case.