Skip to content

Fix GH-10801: Named arguments in CTE functions cause a segfault #10811

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 1 commit into from

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Mar 8, 2023

Fixes GH-10801

Named arguments are not supported by the constant evaluation routine, in the sense that they are ignored. This causes two issues:

  • It causes a crash because not all oplines belonging to the call are removed, which results in SEND_VA{L,R} which should've been removed.
  • It causes semantic issues (demonstrated in the test case).

This case never worked anyway, leading to crashes or incorrect behaviour, so just prevent CTE of calls with named parameters for now. We can choose to support it later, but introducing support for this in a stable branch seems too dangerous.

This patch does not change the removal of SEND_* opcodes in remove_call because the crash bug can't be triggered anymore with this patch as there are no named parameters anymore and no variadic CTE functions exist.

Question

I do have the patch already on my machine to support named arguments & variadics in the remove_call function, which solves bullet point 1. If we're interested in supporting named arguments in CTE, I can write the code to actually pass the named arguments during CTE, which would solve bullet point 2. This would make CTE possible with named arguments. Is this something we want?

Fixes phpGH-10801

Named arguments are not supported by the constant evaluation routine, in
the sense that they are ignored. This causes two issues:
  - It causes a crash because not all oplines belonging to the call are
    removed, which results in SEND_VA{L,R} which should've been removed.
  - It causes semantic issues (demonstrated in the test case).

This case never worked anyway, leading to crashes or incorrect behaviour,
so just prevent CTE of calls with named parameters for now.
We can choose to support it later, but introducing support for this in
a stable branch seems too dangerous.

This patch does not change the removal of SEND_* opcodes in remove_call
because the crash bug can't be triggered anymore with this patch as
there are no named parameters anymore and no variadic CTE functions
exist.
@mvorisek
Copy link
Contributor

mvorisek commented Mar 8, 2023

From user perspective, I expect no behavioural/CTE change when I use named parameters.

@nielsdos
Copy link
Member Author

nielsdos commented Mar 8, 2023

There was a behavioural change prior to this PR because the support for named parameters was not there but the optimizer expected it to work.
As I mentioned in the "question" section, I'm willing to add support for named parameters, but only on master as touching the optimizer for stable versions is somewhat dangerous.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM.

Is this something we want?

Unless the implementation is complex consistency would make sense.

@nielsdos nielsdos self-assigned this Mar 10, 2023
@nielsdos nielsdos closed this in 2c53d63 Mar 10, 2023
nielsdos added a commit to nielsdos/php-src that referenced this pull request Mar 11, 2023
In phpGH-10811 I disabled compile-time-eval for function calls with named
arguments because it was completely unsupported and thus would therefore
crash or give semantically wrong results. This patch undoes the
disabling and implements the necessary components to support CTE for
named arguments.

Since named arguments are a bit of a special case for the VM, I also had
to add some special handling for the SCCP pass. I also had to tweak how
the call removal code worked such that the named arguments, which are
not in num_args, are also removed, as well as the CHECK_UNDEF_ARGS
opline.
nielsdos added a commit to nielsdos/php-src that referenced this pull request Mar 11, 2023
In phpGH-10811 I disabled compile-time-eval for function calls with named
arguments because it was completely unsupported and thus would therefore
crash or give semantically wrong results. This patch undoes the
disabling and implements the necessary components to support CTE for
named arguments.

Since named arguments are a bit of a special case for the VM, I also had
to add some special handling for the SCCP pass. I also had to tweak how
the call removal code worked such that the named arguments, which are
not in num_args, are also removed, as well as the CHECK_UNDEF_ARGS
opline.
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.

3 participants