Skip to content

Fix GH-14361: Deep recursion in zend_cfg.c causes segfault instead of error #14432

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.

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
wants to merge 3 commits into from

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Jun 2, 2024

Building the CFG for JIT or optimizations can run into deep recursion, and depending on the stack limit cause a segfault. This patch uses the stack limit check to fail out of those cases. This prevents a segfault.

We use this check elsewhere in the compiler as well. However, in this case we just make optimizations or JIT fail without aborting the application such that code can still execute.

The attached test case will succeed both without and with this patch, it is mainly intended to test if propagating failure doesn't cause crashes. To reproduce the issue, set a low stack limit using ulimit -s and run the original test case from #14361

@dstogov
Copy link
Member

dstogov commented Jun 3, 2024

I think it's better to switch to iterative reachability mark algorithm (e.g. using zend_worklist)

@nielsdos nielsdos force-pushed the fix-14361-2 branch 2 times, most recently from 5579b4c to d2b79af Compare June 3, 2024 21:15
@nielsdos
Copy link
Member Author

nielsdos commented Jun 3, 2024

I think it's better to switch to iterative reachability mark algorithm (e.g. using zend_worklist)

I agree. I changed it.
To make review simpler: first commit changes indentation. Second commit makes it iterative.
Now it still passes the zend_basic_block *b pointer such that the amount of changes are smaller, but I can change that to an index if you prefer that.

Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

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

Thanks. This looks almost fine.
Please make the final touch - remove the redundant tail-call optimization.

I don't care about passing zend_basic_block or its index.
Maybe, it would be better to not pass it at all and push the start BB here.

Comment on lines 99 to 108
if (i == b->successors_count - 1) {
/* Tail call optimization */
if (succ->flags & ZEND_BB_REACHABLE) {
return;
finished = true;
break;
}

b = succ;
break;
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Iterative algorithm doesn't need tail call optimization any more.
This code may be removed as well as while (!finished) { loop.

@nielsdos
Copy link
Member Author

nielsdos commented Jun 4, 2024

Thanks, removed the tail call optimization

Maybe, it would be better to not pass it at all and push the start BB here.

We currently don't always start from the entry block so I didn't change it here.

Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

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

Looks fine.

@nielsdos nielsdos closed this in a3b148e Jun 5, 2024
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.

Deep recursion in zend_cfg.c causes segfault instead of error
2 participants