Skip to content

Fix GH-24492 (partial): route caller-sized string builders through the fallible allocator - #24548

Open
VijhnAustin wants to merge 1 commit into
php:PHP-8.5from
VijhnAustin:fix-gh24492-fallible-string-builders
Open

VijhnAustin wants to merge 1 commit into
php:PHP-8.5from
VijhnAustin:fix-gh24492-fallible-string-builders

Conversation

@VijhnAustin

Copy link
Copy Markdown

Fixes part of #24492.

Problem

Several ext/standard functions grow their output buffer with the non-fallible string allocators. When the requested size crosses the soft memory_limit, the allocation is not reported back to the caller; it escalates to the hard limit (memory_hard_limit_percent) and the process is terminated via _exit(137), so registered shutdown functions never run and the catchable "Allowed memory size … exhausted" fatal that 8.5.11 produced for the same input is lost. A single userland call with an attacker-influenced size is enough.

Fix

Route these sites through the fallible allocation API already in the tree (zend_string_fextend / zend_string_falloc / zend_string_safe_falloc with fallible = true), propagate the NULL, and surface the standard engine OOM via RETURN_OOM() at the userland boundary:

  • sprintf/printf/vprintf/vsprintf/fprintf/vfprintf (formatted_print.c; the php_sprintf_append* helpers now return bool)
  • pack (pack.c)
  • bin2hex and utf8_encode (string.c)
  • number_format (math.c; tmpbuf released on the failure path)
  • convert_uuencode (uuencode.c)
  • quoted_printable_encode (quot_print.c)

Scope

This is a partial fix. Other sites named in the issue — base64_encode, urlencode/rawurlencode, var_export, the bcmath family, and the main/spprintf.c PAD_CHAR path — route their result through PHPAPI helpers whose internal callers currently dereference the result without a NULL check (they never see a failed huge allocation today because the process is killed first). Converting those safely needs a caller audit, so they are intentionally left for follow-up. The ext/xml/Expat variant is already handled in #24516.

Reproduction

With memory_limit=16M and memory_hard_limit_percent=200:

register_shutdown_function(static fn() => print("shutdown\n"));
sprintf("%40000000s", "");   // or pack("x40000000"); number_format(1.0, 100000000); etc.

Before (stock): Hard memory limit of 33554432 bytes exhausted …, exit 137, no shutdown.
After (patched): catchable Allowed memory size of 16777216 bytes exhausted …, shutdown runs, exit 255.

Tests

Adds 17 .phpt under ext/standard/tests/{strings,math}/gh24492_*, each pinning memory_hard_limit_percent=200 and asserting both the catchable fatal and that a shutdown function still runs (so a failure while preparing the input cannot masquerade as a reproduced bug). Verified on a Windows x64 build (17/17 passing); the patch applies cleanly to current PHP-8.5. Also reproduces on 8.4.27 per the issue — happy to retarget to PHP-8.4 if that is preferred.

🤖 Generated with Claude Code

… the fallible allocator

Several ext/standard functions grow their output buffer with the non-fallible
string allocators. When the requested size crosses the soft memory_limit the
allocation is not reported back to the caller; it escalates to the hard limit
(memory_hard_limit_percent) and the process is terminated via _exit(137), so
registered shutdown functions never run and the catchable "Allowed memory size
exhausted" fatal that 8.5.11 produced for the same input is lost.

Route these sites through the fallible allocation API already in the tree
(zend_string_fextend / zend_string_falloc / zend_string_safe_falloc with
fallible=true), propagate the NULL return, and surface the standard engine OOM
via RETURN_OOM() at the userland boundary:

  - sprintf/printf/vprintf/vsprintf/fprintf/vfprintf (formatted_print.c; the
    php_sprintf_append* helpers now return bool)
  - pack (pack.c)
  - bin2hex and utf8_encode (string.c)
  - number_format (math.c; tmpbuf released on the failure path)
  - convert_uuencode (uuencode.c)
  - quoted_printable_encode (quot_print.c)

Adds 17 .phpt regression tests, each pinning memory_hard_limit_percent=200 and
asserting both the catchable fatal and that a shutdown function still runs.

Other sites named in the issue (base64_encode, urlencode/rawurlencode,
var_export, the bcmath family, and the main/spprintf.c PAD_CHAR path) require
auditing their internal callers for NULL-safety before conversion and are
intentionally left for follow-up.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant