Fix #81070: Integer underflow when memory limit is exceeded

[pvandommelen]

When the memory limit is reduced using an ini_set("memory_limit", ..)
below the currently allocated memory. Further allocations should
cause a memory limit exceeded error.
This fixes the regression introduced in 7.2.23

I have targeted the 7.3 branch as this bug allows for unlimited memory usage in some scenarios.

[cmb69]

Thanks for the PR! I don't think this qualifies as security issue, so please target PHP-7.4. Also note that the test fails due to slight differences in the error message.

[pvandommelen]

Targeted 7.4 and fixed the test for non-debug builds. Thanks.
I don't think the remaining build failure is related to my change, not sure what is wrong there.

[cmb69]

The AppVeyor failure is because the PR is still build against PHP-7.3, since you didn't rebase onto PHP-7.4 before changing the branch. IMO not a real issue.

[nikic]

I think a better way to approach this is to forbid reducing the memory limit below the current usage. That would point out the error at the point it is introduced, and avoid additional checks in the allocator.

It should be possible to do this by adjusting zend_set_memory_limit to return FAILURE if the limit is below current usage and checking the return value in

php-src/main/main.c

Line 274 in c40231a

zend_set_memory_limit(PG(memory_limit));

.

[pvandommelen]

👍 I'll see if I can implement it like that this weekend.

[pvandommelen]

PR now contains the other approach as suggested by nikic: zend_alloc's zend_set_memory_limit verifies the value and returns FAILURE when the requirement is not met. PHP_INI_MH(OnChangeMemoryLimit) will transform this into a warning ("Failed to set memory limit to %d bytes (Current memory usage is %d bytes)") and not result in any change when this occurs.

The only other call to zend_set_memory_limit within php_error_cb does not check the return value on whether the change was succesful. I think this is ok. This reset-call may even be unnecessary as I haven't been able to find out where the limit is initially changed.

A warning is used instead of an error. I think that makes the most sense, but I'm not sure if there are any similar cases which do throw an error.

The warning message contains a fairly generic error without a mention of the target memory being lower than the current. The error message could be moved into the zend_set_memory_limit where we would have this information, but that causes problems with the error handler which recursively calls zend_set_memory_limit.

I'm not certain if we have to attempt to run zend_mm_gc when the initial comparison fails, this implementation doesn't. I have not been able to produce a testcase which would be affected by running it and I'm not sure how that could be achieved. Is that still relevant or is is zend_mm_gc affecting available memory related to an old situation which has been superceded by other GC improvements? There is another issue with cycles not being removed, but this is also an issue for the regular out of memory error (https://bugs.php.net/bug.php?id=60982).

When merging into 8.0/master you'll encounter a conflict with fa8d9b1, it'll probably be ok.

============

By forbidding the limit to be lowered below the current memory two other issues have also surfaced where this was (likely) done incorrectly in tests.

One is a test which sets the limit to 512KB, lower than the initial memory usage. I have increased the memory used in this test.

Another is an ancient test and accompanying fix that don't have the correct behaviour.
https://bugs.php.net/bug.php?id=45392
fcc0fdd

For this PR I have fixed the test so that it will fail as it probably should and marked it as XFAIL. The problem is that the memory limit in main's PG(memory_limit) differs from the one in zend_alloc's AG(mm_heap)->limit. In zend_alloc this is defined as max(passed_value, ZEND_MM_CHUNK_SIZE). The check which is made on an unclean shutdown with if (PG(memory_limit) < zend_memory_usage(1)) should never be true (especially so when this PR is included) unless unless the memory limit is set below ZEND_MM_CHUNK_SIZE. This was the case for the test.

It gets beyond the scope of this particular PR but the following code in main/main.c should probably be adaptered or removed:

if (CG(unclean_shutdown) && PG(last_error_type) == E_ERROR &&
    (size_t)PG(memory_limit) < zend_memory_usage(1)
) {
    send_buffer = 0;
}

Removing the check on the memory limit (and error type?) entirely makes a lot of sense to me, but I can imagine that it's better to apply such a change on master instead.

[nikic]

For PHP-7.4 I've landed a modified version of this patch in 1b3b5c9, which makes rounds up the limit to the chunk size, and thus avoids impacting those two tests. Let's do that change only for master.

For PHP-8.0 I had to introduce a zend_set_memory_limit_ex() API to avoid an ABI break, which is dropped on master again with 1aafed5.

[nikic]

Remaining change together with more test adjustments landed in master as 3a4ea6c.

[nikic]

The output buffering issue should be addressed by e9b0051. I've moved discarding output into the error handler, which allows us to print the memory limit error to normal output. Otherwise you don't see anything at all unless you do something like display_errors=stderr, as that test was originally doing, but which is rarely used in practice.

I think with that, all issues here are resolved.

[iBotPeaches]

Guess our system(s) was doing some naughty things since we are noticing crashes after bumping up to 7.4.21. I guess we were trying to ini set like 512M, but the system was already running around ~600M and now the warning bubbles up to something not handled very well.

Very interesting background after following the 7.4.21 change log to a related entry about memory to this.