Skip to content

Add VM reentry limit#5135

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nikic wants to merge 1 commit into from
Closed

Add VM reentry limit #5135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 84 additions & 0 deletions Zend/tests/vm_reentry_limit.phpt
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
--TEST--
VM reentry limit
--INI--
zend.vm_reentry_limit=20
--FILE--
<?php

class Test1 {
public function __destruct() {
new Test1;
}
}

class Test2 {
public function __clone() {
clone $this;
}
}

try {
new Test1;
} catch (Error $e) {
echo $e, "\n";
}

echo "\n";

try {
clone new Test2;
} catch (Error $e) {
echo $e, "\n";
}

?>
--EXPECTF--
Error: VM reentry limit of 20 reached. Infinite recursion? in %s:%d
Stack trace:
#0 %s(%d): Test1->__destruct()
#1 %s(%d): Test1->__destruct()
#2 %s(%d): Test1->__destruct()
#3 %s(%d): Test1->__destruct()
#4 %s(%d): Test1->__destruct()
#5 %s(%d): Test1->__destruct()
#6 %s(%d): Test1->__destruct()
#7 %s(%d): Test1->__destruct()
#8 %s(%d): Test1->__destruct()
#9 %s(%d): Test1->__destruct()
#10 %s(%d): Test1->__destruct()
#11 %s(%d): Test1->__destruct()
#12 %s(%d): Test1->__destruct()
#13 %s(%d): Test1->__destruct()
#14 %s(%d): Test1->__destruct()
#15 %s(%d): Test1->__destruct()
#16 %s(%d): Test1->__destruct()
#17 %s(%d): Test1->__destruct()
#18 %s(%d): Test1->__destruct()
#19 %s(%d): Test1->__destruct()
#20 %s(%d): Test1->__destruct()
#21 {main}

Error: VM reentry limit of 20 reached. Infinite recursion? in %s:%d
Stack trace:
#0 %s(%d): Test2->__clone()
#1 %s(%d): Test2->__clone()
#2 %s(%d): Test2->__clone()
#3 %s(%d): Test2->__clone()
#4 %s(%d): Test2->__clone()
#5 %s(%d): Test2->__clone()
#6 %s(%d): Test2->__clone()
#7 %s(%d): Test2->__clone()
#8 %s(%d): Test2->__clone()
#9 %s(%d): Test2->__clone()
#10 %s(%d): Test2->__clone()
#11 %s(%d): Test2->__clone()
#12 %s(%d): Test2->__clone()
#13 %s(%d): Test2->__clone()
#14 %s(%d): Test2->__clone()
#15 %s(%d): Test2->__clone()
#16 %s(%d): Test2->__clone()
#17 %s(%d): Test2->__clone()
#18 %s(%d): Test2->__clone()
#19 %s(%d): Test2->__clone()
#20 %s(%d): Test2->__clone()
#21 {main}
1 change: 1 addition & 0 deletions Zend/zend.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -192,6 +192,7 @@ ZEND_INI_BEGIN()
#endif
STD_ZEND_INI_BOOLEAN("zend.exception_ignore_args", "0", ZEND_INI_ALL, OnUpdateBool, exception_ignore_args, zend_executor_globals, executor_globals)
STD_ZEND_INI_ENTRY("zend.exception_string_param_max_len", "15", ZEND_INI_ALL, OnSetExceptionStringParamMaxLen, exception_string_param_max_len, zend_executor_globals, executor_globals)
STD_ZEND_INI_ENTRY("zend.vm_reentry_limit", "1000", ZEND_INI_ALL, OnUpdateLong, vm_reentry_limit, zend_executor_globals, executor_globals)

@TysonAndreTysonAndreFeb 3, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would a small minimum value make sense?

Right now, it's possible to set it to 0, which would prevent __destruct (etc) from being called at all, e.g. during request cleanup.

Values such as 0 may lead to false positive bug reports from users who misconfigured it, e.g. when setting this when debugging other issues and misreading the documentation.

LGTM other than the question.

@beberleibeberleiFeb 4, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this tricky especially with extensions that overwrite zend_execute_ex the limit is certainly going to be hit by larger frameworks and template engines regularly, whereas for the goto vm mode it will never go very deep.

I believe 1000 could be too small.

@TysonAndreTysonAndreFeb 4, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another suggestion: adding some form of logging (of a severity not catchable by set_error_handler, because the error handler itself is re-entering?) before throwing an exception might help avoid confusion with catch blocks that don't perform any logging.

Without the additional logging, there would be no obvious reason why recursive AST processing would potentially silently fail to process the entire AST on some system configurations.

@nikicnikicFeb 4, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extensions that overwrite zend_execute_ex should set a higher limit for this ini setting, or disable it entirely. A higher limit doesn't really make sense for a default config (probably a much lower one would also do).

@beberleibeberleiFeb 4, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikic i am thinking in terms of developer experience, this segfault is something users can hardly understand (as you mentioned in your mailing list post). As such i would certainly like to not disable it when my extension is active.

With just 1000, all extensions just disable it magically and users run into these segfaults again, which are more likely to happen when you have an extension installed that uses a zend_execute_ex overwrite.

@nikicnikicFeb 6, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@beberlei So first of all, I do hope that your extension (and other APM extensions) will be able to avoid the VM recursion based on the new hooks that Levi & co were working on. But assuming that doesn't pan out (in time), you don't necessarily have to disable the option (that's just the conservative choice, that gives you exactly the behavior you get on PHP 7), you can also bump the limit to a larger value, based on an analysis of stack usage if your extension is enabled. I don't think that it makes sense to increase the default value further (I'd say 1000 is already very conservative), because it's unlikely to be useful without such an extension in play, but increases the chances of missing stack overflows (if you bump to say 10000).

@nikicnikicFeb 6, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TysonAndre I wouldn't want to introduce a special error handling mechanism just for this functionality -- but possibly it would be better to make this a fatal error instead of an exception? I mainly went with the exception because "I can" and it includes a stack trace that is likely helpful when dealing with infinite recursion.

@TysonAndreTysonAndreApr 11, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be better to make this a fatal error instead of an exception

That would be my preference - terminating cleanly instead of throwing an error would avoid a lot of confusing edge cases in internals and for end users. The programs would already be prone to terminating if they recursed that deeply, except for edge cases (e.g. __toString on a deeply nested tree structure).

My other concern was whether this breaks assumptions in opcache or for end users (e.g. certain opcode types will never throw at runtime, e.g. casting to string)

Error handling properly might be an issue

try {
array_map(...);
} catch (Throwable $e) {
echo "Caught $e"; // re-enters when calling __toString() and also throws
}

Also, if you hit the recursion limit, then __destruct() can't get called when an object is freed, which breaks other assumptions you might have about unserialize(), or assumptions users make about RAII or other patterns (e.g. connection or memory leaks)

@beberleibeberleiApr 11, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikic yes, i have tested levis new API extensively already and it will make this problem go away.

ZEND_INI_END()

ZEND_API size_t zend_vspprintf(char **pbuf, size_t max_len, const char *format, va_list ap) /* {{{ */
Expand Down
7 changes: 7 additions & 0 deletions Zend/zend_execute.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -2046,6 +2046,13 @@ static zend_never_inline ZEND_COLD void ZEND_FASTCALL zend_use_new_element_for_s
zend_throw_error(NULL, "[] operator not supported for strings");
}

static zend_never_inline ZEND_COLD void ZEND_FASTCALL zend_vm_reentry_limit_error()
{
zend_throw_error(NULL,
"VM reentry limit of " ZEND_ULONG_FMT " reached. Infinite recursion?",
EG(vm_reentry_limit));
}

static ZEND_COLD void zend_binary_assign_op_dim_slow(zval *container, zval *dim OPLINE_DC EXECUTE_DATA_DC)
{
if (UNEXPECTED(Z_TYPE_P(container) == IS_STRING)) {
Expand Down
1 change: 1 addition & 0 deletions Zend/zend_execute_API.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -175,6 +175,7 @@ void init_executor(void) /* {{{ */

EG(fake_scope) = NULL;
EG(trampoline).common.function_name = NULL;
EG(vm_reentry_count) = 0;

EG(ht_iterators_count) = sizeof(EG(ht_iterators_slots)) / sizeof(HashTableIterator);
EG(ht_iterators_used) = 0;
Expand Down
3 changes: 3 additions & 0 deletions Zend/zend_globals.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -249,6 +249,9 @@ struct _zend_executor_globals {

zend_get_gc_buffer get_gc_buffer;

zend_ulong vm_reentry_count;
zend_ulong vm_reentry_limit;

void *reserved[ZEND_MAX_RESERVED_RESOURCES];
};

Expand Down
9 changes: 9 additions & 0 deletions Zend/zend_vm_execute.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -54488,6 +54488,12 @@ ZEND_API void execute_ex(zend_execute_data *ex)
LOAD_OPLINE();
ZEND_VM_LOOP_INTERRUPT_CHECK();

if (EG(vm_reentry_count)++ > EG(vm_reentry_limit)) {
zend_vm_reentry_limit_error();
LOAD_OPLINE();
/* Fall through to handle exception below. */
}

while (1) {
#if !defined(ZEND_VM_FP_GLOBAL_REG) || !defined(ZEND_VM_IP_GLOBAL_REG)
int ret;
Expand DownExpand Up@@ -58983,6 +58989,7 @@ ZEND_API void execute_ex(zend_execute_data *ex)
#ifdef ZEND_VM_IP_GLOBAL_REG
opline = vm_stack_data.orig_opline;
#endif
EG(vm_reentry_count)--;
return;
HYBRID_DEFAULT:
VM_TRACE(ZEND_NULL)
Expand All@@ -58994,6 +59001,7 @@ ZEND_API void execute_ex(zend_execute_data *ex)
# ifdef ZEND_VM_IP_GLOBAL_REG
opline = vm_stack_data.orig_opline;
# endif
EG(vm_reentry_count)--;
return;
#else
if (EXPECTED(ret > 0)) {
Expand All@@ -59003,6 +59011,7 @@ ZEND_API void execute_ex(zend_execute_data *ex)
# ifdef ZEND_VM_IP_GLOBAL_REG
opline = vm_stack_data.orig_opline;
# endif
EG(vm_reentry_count)--;
return;
}
#endif
Expand Down
6 changes: 6 additions & 0 deletions Zend/zend_vm_execute.skl
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -16,6 +16,12 @@ ZEND_API void {%EXECUTOR_NAME%}_ex(zend_execute_data *ex)
LOAD_OPLINE();
ZEND_VM_LOOP_INTERRUPT_CHECK();

if (EG(vm_reentry_count)++ > EG(vm_reentry_limit)) {
zend_vm_reentry_limit_error();
LOAD_OPLINE();
/* Fall through to handle exception below. */
}

while (1) {
{%ZEND_VM_CONTINUE_LABEL%}
{%ZEND_VM_DISPATCH%} {
Expand Down
7 changes: 5 additions & 2 deletions Zend/zend_vm_gen.php
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -1787,6 +1787,7 @@ function gen_executor_code($f, $spec, $kind, $prolog, &$switch_labels = array())
out($f,"#ifdef ZEND_VM_IP_GLOBAL_REG\n");
out($f,"\t\t\t\topline = vm_stack_data.orig_opline;\n");
out($f,"#endif\n");
out($f,"\t\t\t\tEG(vm_reentry_count)--;\n");
out($f,"\t\t\t\treturn;\n");
out($f,"\t\t\tHYBRID_DEFAULT:\n");
out($f,"\t\t\t\tVM_TRACE(ZEND_NULL)\n");
Expand DownExpand Up@@ -1984,7 +1985,7 @@ function gen_executor($f, $skl, $spec, $kind, $executor_name, $initializer_name)
out($f,"#define HANDLE_EXCEPTION() ZEND_ASSERT(EG(exception)); LOAD_OPLINE(); ZEND_VM_CONTINUE()\n");
out($f,"#define HANDLE_EXCEPTION_LEAVE() ZEND_ASSERT(EG(exception)); LOAD_OPLINE(); ZEND_VM_LEAVE()\n");
out($f,"#define ZEND_VM_CONTINUE() goto zend_vm_continue\n");
out($f,"#define ZEND_VM_RETURN() return\n");
out($f,"#define ZEND_VM_RETURN() EG(vm_reentry_count)--; return\n");
out($f,"#define ZEND_VM_ENTER_EX() ZEND_VM_INTERRUPT_CHECK(); ZEND_VM_CONTINUE()\n");
out($f,"#define ZEND_VM_ENTER() execute_data = EG(current_execute_data); LOAD_OPLINE(); ZEND_VM_ENTER_EX()\n");
out($f,"#define ZEND_VM_LEAVE() ZEND_VM_CONTINUE()\n");
Expand DownExpand Up@@ -2015,7 +2016,7 @@ function gen_executor($f, $skl, $spec, $kind, $executor_name, $initializer_name)
out($f,"#define HANDLE_EXCEPTION_LEAVE() ZEND_ASSERT(EG(exception)); goto ZEND_HANDLE_EXCEPTION_LABEL\n");
}
out($f,"#define ZEND_VM_CONTINUE() goto *(void**)(OPLINE->handler)\n");
out($f,"#define ZEND_VM_RETURN() return\n");
out($f,"#define ZEND_VM_RETURN() EG(vm_reentry_count)--; return\n");
out($f,"#define ZEND_VM_ENTER_EX() ZEND_VM_INTERRUPT_CHECK(); ZEND_VM_CONTINUE()\n");
out($f,"#define ZEND_VM_ENTER() execute_data = EG(current_execute_data); LOAD_OPLINE(); ZEND_VM_ENTER_EX()\n");
out($f,"#define ZEND_VM_LEAVE() ZEND_VM_CONTINUE()\n");
Expand DownExpand Up@@ -2184,6 +2185,7 @@ function gen_executor($f, $skl, $spec, $kind, $executor_name, $initializer_name)
"# ifdef ZEND_VM_IP_GLOBAL_REG\n" .
$m[1]."opline = vm_stack_data.orig_opline;\n" .
"# endif\n" .
$m[1]."EG(vm_reentry_count)--;\n" .
$m[1]."return;\n" .
"#else\n" .
$m[1]."if (EXPECTED(ret > 0)) {\n" .
Expand All@@ -2193,6 +2195,7 @@ function gen_executor($f, $skl, $spec, $kind, $executor_name, $initializer_name)
"# ifdef ZEND_VM_IP_GLOBAL_REG\n" .
$m[1]."\topline = vm_stack_data.orig_opline;\n" .
"# endif\n".
$m[1]."\tEG(vm_reentry_count)--;\n".
$m[1]."\treturn;\n".
$m[1]."}\n".
"#endif\n");
Expand Down
6 changes: 6 additions & 0 deletions php.ini-development
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -386,6 +386,12 @@ zend.exception_ignore_args = Off
; Production Value: 0
zend.exception_string_param_max_len = 15

; Limit for recursion via VM reentry, used to prevent stack overflow.
; This only affects recursion through magic method calls and similar mechanisms.
; Some profiling, debugging or APM extensions might make this limit apply to plain
; recursion as well, in which case you may wish to raise it.
;zend.vm_reentry_limit = 1000

;;;;;;;;;;;;;;;;;
; Miscellaneous ;
;;;;;;;;;;;;;;;;;
Expand Down
6 changes: 6 additions & 0 deletions php.ini-production
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -388,6 +388,12 @@ zend.exception_ignore_args = On
; of sensitive information in stack traces.
zend.exception_string_param_max_len = 0

; Limit for recursion via VM reentry, used to prevent stack overflow.
; This only affects recursion through magic method calls and similar mechanism.
; Some profiling, debugging or APM extensions might make this limit apply to plain
; recursion as well, in which case you may wish to raise it.
;zend.vm_reentry_limit = 1000

@mvorisekmvorisekJun 29, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following example https://3v4l.org/jJD62 results with segfault as well. No magic method used.

Does this PR fixes is as well and why does it happen in the first place? Does it mean array_map use the system stack, but when recursing php functions only, no system stack is used?

PHP does already support fibers. What about creating a Fiber internally on the start and unrolling to it on every like 1000th reentry so the system stack is never exhausted (always kept below 1000)?

This way, we do not need any php.ini and unlimited recursion will be supported for absolutely everything.

@cmb69cmb69Jun 30, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When PHP code is called from C code (internal functions such as array_map() are implemented in C), the VM is re-entered by calling execute_ex(). If that happens way too often (which is almost always a programming error; infinite recursion), you get a stack overflow error.

@mvorisekmvorisekJun 30, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think Fiber can be used to fix that?

To level of my knowledge, it can solve exactly the issue - fiber (created on php start) can restore some shallow C stack once some reentry count is reached - but still access/modify the latest VM state.

@cmb69cmb69Jun 30, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that you can "just" unroll a machine stack.

@mvorisekmvorisekJun 30, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a working demo - https://3v4l.org/0Jjrr

I don't think that you can "just" unroll a machine stack.

this is what Fiber does - replace machine stack, thus if we replace the current machine stack with shallower machine stack (which we can easily construct on the start), we effectively "unroll" the working machine stack

@mvorisekmvorisekJul 1, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the demo and I can run it (with 2G memory_limit) with 1M iterations/C nested calls.

@cmb69 can you please test it on your side as well?

@beberleibeberleiJul 2, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mvorisek you are asking to rewrite a core architecture of zend engine, its not as easy as you make it sound. A call to an internal fn must go through the internal (and not the userland) execution handler and this causes the stack to increase. Same for PHP extensions that use an old mechanism for instrumentation like NewRelic for example, they even require this for userland calls.

@cmb69cmb69Jul 2, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I have to add that I don't see the point in this. If a stack overflow happens due to indirect execution, you likely made a programming error, and ideally want to have that reported, which is exactly what this PR is about.

If there are still concerns regarding extensions overriding zend_execute_ex() this could be catered to, possibly by ignoring the zend.vm_reentry_limit in that case.

The only thing we might need to consider are fibers; it might be reasonable to have an own vm_reentry_count for each fiber.

@mvorisekmvorisekJul 2, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is the machine stack is limited to about 2k VM reentries [1]. One can argue this is "likely made a programming error", but for deeply recursive algorithms with magic methods, any hard limit can be too limiting.

So I propose to use C fiber to overcome the C/host stack limitation by using a machine stack "unroll" as described /w PoC above. This solution does not need any arbitrary limit and if infine recurse will occur, the execution will stop on standard OOM.

I think this solves the real "VM reentry" limitation and it does not introduce any "for some cases recursion limit" [2]. I strongly belive, from an user perspective, any function invocation should behave the same, eg. behaviour/limitation should be the same for php function called directly vs. called from C function.

[1] see PR description, "VM reentry" happen when "When PHP code is called from C code (internal function"
[2] citing from PR description:

The` VM reentry limit is intentionally named somewhat obscurely: It is not a direct limit on recursion. It only counts the number of VM reentries, because that's what can eventually lead to a stack overflow.

@beberleibeberleiJul 2, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose

This sounds like a may months project and will break many extensions.

For now we should use this to avoid the segfaults.


;;;;;;;;;;;;;;;;;
; Miscellaneous ;
;;;;;;;;;;;;;;;;;
Expand Down