GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Fix GH-23089: stack overflow in array_merge_recursive() with deep arrays by lazerg · Pull Request #23091 · php/php-src

Conversation

@lazerg

@lazerg lazerg commented Aug 6, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

php_array_merge_recursive() recurses once per nesting level with no stack check, so array_merge_recursive() on a deeply nested array exhausts the native stack and the process dies with a segfault.

This adds the same stack limit check ext/standard already uses in var.c and http.c, so the call throws an Error instead of crashing.

array_replace_recursive() has the same unguarded recursion. I kept this to the reported crash, but can cover it here too if you'd rather have both in one go.

Fixes #23089.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@lazerg
Fix phpGH-23089: stack overflow in array_merge_recursive() with deep …
9150095
…arrays
@lazerg
lazerg requested a review from bukka as a code owner August 6, 2026 14:04
@github-actions github-actions Bot added the Extension: standard label Aug 6, 2026
@lazerg lazerg mentioned this pull request Aug 8, 2026
Fix GH-23113: stack overflow in array_replace_recursive() with deep arrays #23124
Merged
@lazerg

lazerg commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Hi @devnexen, just checking if you have had a chance to look at this crash fix. Happy to make any changes needed.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Sjord

Sjord commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

array_replace_recursive has the same problem.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@lazerg

lazerg commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Probably I need to create seperate PR for that?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Sjord

Sjord commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Yes, that is not a blocker for this PR, just something I realized while looking at this PR. I could have been more clear, sorry about that.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@lazerg

lazerg commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

array_replace_recursive() already got this same stack check in af1abdf (#23124, fixing GH-23113), merged on 2026-08-10. So it should already be covered on master.

Sjord reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Sjord

Sjord commented Sep 5, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

This exposes a memory leak. When php_array_merge_recursive returns 0, tmp is not destructed. This bug is not introduced by this PR, but returning 0 exposes it. Around line 4095:

ret = php_array_merge_recursive(Z_ARRVAL_P(dest_zval), Z_ARRVAL_P(src_zval));
if (thash) {
	GC_TRY_UNPROTECT_RECURSION(thash);
}
if (!ret) {
	return 0;
}

This is missing zval_ptr_dtor(&tmp);.

Test case:

--TEST--
GH-23089 (Memory leak in array_merge_recursive on stack overflow with object)
--SKIPIF--
<?php
if (ini_get('zend.max_allowed_stack_size') === false) {
    die('skip No stack limit support');
}
if (getenv('SKIP_ASAN')) {
    die('skip ASAN needs different stack limit setting due to more stack space usage');
}
?>
--INI--
zend.max_allowed_stack_size=256K
--FILE--
<?php
$obj = new stdClass();
$deep = [];
for ($i = 0; $i < 30000; $i++) {
    $deep = ['k' => $deep];
}
$obj->val = $deep;
$a = ['obj' => $obj];
try {
    array_merge_recursive($a, $a);
} catch (Throwable $e) {
    echo $e::class, ": ", $e->getMessage(), "\n";
}
echo "done\n";
?>
--EXPECTF--
Error: Maximum call stack size of %d bytes (zend.max_allowed_stack_size - zend.reserved_stack_size) reached. Infinite recursion?
done

This outputs things like:

[Sat Sep  5 17:36:00 2026]  Script:  '/home/sjoerd/php-src/ext/standard/tests/array/gh23089_leak.phpt'
/home/sjoerd/php-src/Zend/zend_hash.c(291) :  Freeing 0x0000fc2ec9203480 (56 bytes), script=/home/sjoerd/php-src/ext/standard/tests/array/gh23089_leak.phpt
Last leak repeated 30000 times
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@lazerg
Fix memory leak on php_array_merge_recursive() bail-out paths
c425d88
@lazerg

lazerg commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Confirmed. I built PHP with --enable-debug and ran your test. It reported 60002 leaks.

I added zval_ptr_dtor(&tmp); before both return 0; paths that run after tmp is initialized. The "Recursion detected" bail-out sits above ZVAL_UNDEF(&tmp), so it does not need one.

The leaks are gone now, and ext/standard plus Zend give 8033 passed, 0 failed. I also added your test case as ext/standard/tests/array/gh23089_leak.phpt.

Commit: c425d88

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Sjord

Sjord commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Thanks. This looks good to me!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@lazerg

lazerg commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

Closing this due to no activity.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@lazerg lazerg closed this Sep 24, 2026
@akasakariko akasakariko mentioned this pull request Nov 12, 2026
Fix GH-23089: stack overflow in array_merge_recursive() #25406
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bukka bukka Awaiting requested review from bukka bukka is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

Extension: standard

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@lazerg @Sjord
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.