Skip to content

Commit 9b86533

Browse files
committed
Fix GH-19065: Long match statement can segfault compiler during recursive SSA renaming
On some systems, like Alpine, the thread stack size is small by default. The last step of SSA construction involves variable renaming that is recursive, and also makes copies of their version of the renamed variables on the stack. This combination causes a stack overflow during compilation on Alpine. Triggerable for example with very long match statements. A stop-gap solution would be to use heap allocated arrays for the renamed variable list, but that would only delay the error as increasing the number of match arms increases the depth of the dominator tree, and will eventually run into the same issue. This patch transforms the algorithm into an iterative one. There are two states stored in a worklist stack: positive numbers indicate that the block still needs to undergo variable renaming. Negative numbers indicate that the block and its dominated children are already renamed. Because 0 is also a valid block number, we bias the block numbers by adding 1. To restore to the right variant when backtracking the "recursive" step, we index into an array pointing to the different variable renaming variants. Closes GH-19083.
1 parent b57578f commit 9b86533

File tree

2 files changed

+82
-20
lines changed

2 files changed

+82
-20
lines changed

NEWS

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@ PHP NEWS
2020
. Fixed bug GH-18736 (Circumvented type check with return by ref + finally).
2121
(ilutov)
2222
. Fixed zend call stack size for macOs/arm64. (David Carlier)
23+
. Fixed bug GH-19065 (Long match statement can segfault compiler during
24+
recursive SSA renaming). (nielsdos, Arnaud)
2325

2426
- Calendar:
2527
. Fixed bug GH-19371 (integer overflow in calendar.c). (nielsdos)

Zend/Optimizer/zend_ssa.c

Lines changed: 80 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
#include "zend_ssa.h"
2323
#include "zend_dump.h"
2424
#include "zend_inference.h"
25+
#include "zend_worklist.h"
2526
#include "Optimizer/zend_optimizer_internal.h"
2627

2728
static bool dominates(const zend_basic_block *blocks, int a, int b) {
@@ -787,23 +788,14 @@ ZEND_API int zend_ssa_rename_op(const zend_op_array *op_array, const zend_op *op
787788
}
788789
/* }}} */
789790

790-
static zend_result zend_ssa_rename(const zend_op_array *op_array, uint32_t build_flags, zend_ssa *ssa, int *var, int n) /* {{{ */
791+
static void zend_ssa_rename_in_block(const zend_op_array *op_array, uint32_t build_flags, zend_ssa *ssa, int *var, int n) /* {{{ */
791792
{
792793
zend_basic_block *blocks = ssa->cfg.blocks;
793794
zend_ssa_block *ssa_blocks = ssa->blocks;
794795
zend_ssa_op *ssa_ops = ssa->ops;
795796
int ssa_vars_count = ssa->vars_count;
796797
int i, j;
797798
zend_op *opline, *end;
798-
int *tmp = NULL;
799-
ALLOCA_FLAG(use_heap = 0);
800-
801-
// FIXME: Can we optimize this copying out in some cases?
802-
if (blocks[n].next_child >= 0) {
803-
tmp = do_alloca(sizeof(int) * (op_array->last_var + op_array->T), use_heap);
804-
memcpy(tmp, var, sizeof(int) * (op_array->last_var + op_array->T));
805-
var = tmp;
806-
}
807799

808800
if (ssa_blocks[n].phis) {
809801
zend_ssa_phi *phi = ssa_blocks[n].phis;
@@ -887,22 +879,90 @@ static zend_result zend_ssa_rename(const zend_op_array *op_array, uint32_t build
887879
}
888880

889881
ssa->vars_count = ssa_vars_count;
882+
}
883+
/* }}} */
890884

891-
j = blocks[n].children;
892-
while (j >= 0) {
893-
// FIXME: Tail call optimization?
894-
if (zend_ssa_rename(op_array, build_flags, ssa, var, j) == FAILURE)
895-
return FAILURE;
896-
j = blocks[j].next_child;
897-
}
885+
static zend_result zend_ssa_rename(const zend_op_array *op_array, uint32_t build_flags, zend_ssa *ssa, int *var, int n)
886+
{
887+
/* The worklist contains block numbers, encoded as positive or negative value.
888+
* Positive values indicate that the variable rename still needs to happen for the block.
889+
* Negative values indicate the variable rename was done and all children were handled too.
890+
* In that case, we will clean up.
891+
* Because block 0 is valid, we bias the block numbers by adding 1 such that we can distinguish
892+
* positive and negative values in all cases. */
893+
zend_worklist_stack work;
894+
ALLOCA_FLAG(work_use_heap);
895+
ZEND_WORKLIST_STACK_ALLOCA(&work, ssa->cfg.blocks_count, work_use_heap);
896+
zend_worklist_stack_push(&work, n + 1);
897+
898+
/* This is used to backtrack the right version of the renamed variables to use. */
899+
ALLOCA_FLAG(save_vars_use_heap);
900+
unsigned int save_vars_top = 0;
901+
int **save_vars = do_alloca(sizeof(int *) * (ssa->cfg.blocks_count + 1), save_vars_use_heap);
902+
save_vars[0] = var;
903+
904+
while (work.len) {
905+
n = zend_worklist_stack_pop(&work);
906+
907+
/* Enter state: perform SSA variable rename */
908+
if (n > 0) {
909+
n--;
910+
911+
// FIXME: Can we optimize this copying out in some cases?
912+
int *new_var;
913+
if (ssa->cfg.blocks[n].next_child >= 0) {
914+
new_var = emalloc(sizeof(int) * (op_array->last_var + op_array->T));
915+
memcpy(new_var, save_vars[save_vars_top], sizeof(int) * (op_array->last_var + op_array->T));
916+
save_vars[++save_vars_top] = new_var;
917+
} else {
918+
new_var = save_vars[save_vars_top];
919+
}
898920

899-
if (tmp) {
900-
free_alloca(tmp, use_heap);
921+
zend_ssa_rename_in_block(op_array, build_flags, ssa, new_var, n);
922+
923+
int j = ssa->cfg.blocks[n].children;
924+
if (j >= 0) {
925+
/* Push backtrack state */
926+
zend_worklist_stack_push(&work, -(n + 1));
927+
928+
/* Push children in enter state */
929+
unsigned int child_count = 0;
930+
int len_prior = work.len;
931+
do {
932+
zend_worklist_stack_push(&work, j + 1);
933+
j = ssa->cfg.blocks[j].next_child;
934+
child_count++;
935+
} while (j >= 0);
936+
937+
/* Reverse block order to maintain SSA variable number order given in previous PHP versions,
938+
* but the data structure doesn't allow reverse dominator tree traversal. */
939+
for (unsigned int i = 0; i < child_count / 2; i++) {
940+
int tmp = work.buf[len_prior + i];
941+
work.buf[len_prior + i] = work.buf[work.len - 1 - i];
942+
work.buf[work.len - 1 - i] = tmp;
943+
}
944+
} else {
945+
/* Leafs jump directly to backtracking */
946+
goto backtrack;
947+
}
948+
}
949+
/* Leave state: backtrack */
950+
else {
951+
n = -n;
952+
n--;
953+
backtrack:;
954+
if (ssa->cfg.blocks[n].next_child >= 0) {
955+
efree(save_vars[save_vars_top]);
956+
save_vars_top--;
957+
}
958+
}
901959
}
902960

961+
free_alloca(save_vars, save_vars_use_heap);
962+
ZEND_WORKLIST_STACK_FREE_ALLOCA(&work, work_use_heap);
963+
903964
return SUCCESS;
904965
}
905-
/* }}} */
906966

907967
ZEND_API zend_result zend_build_ssa(zend_arena **arena, const zend_script *script, const zend_op_array *op_array, uint32_t build_flags, zend_ssa *ssa) /* {{{ */
908968
{

0 commit comments

Comments
 (0)