Skip to content

Reset user func trampoline values FFI may overwrite #10916

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 2 commits into from

Conversation

iluuu1994
Copy link
Member

@iluuu1994 iluuu1994 commented Mar 23, 2023

This was revealed by #10880 by changing padding and now overwriting last_var which broke stack size calculation.

The values are overwritten by FFI here:

php-src/ext/ffi/ffi.c

Lines 2168 to 2172 in c58c266

func->internal_function.handler = ZEND_FN(ffi_trampoline);
func->internal_function.module = NULL;
func->internal_function.reserved[0] = type;
func->internal_function.reserved[1] = *(void**)cdata->ptr;

A more future-proof solution might be to memset the entire function but I wanted to avoid adding that overhead without feedback first.

@iluuu1994 iluuu1994 requested review from dstogov and nielsdos March 23, 2023 19:37
@iluuu1994 iluuu1994 force-pushed the ffi-trampoline-reset branch from 017062f to 05ef57f Compare March 27, 2023 14:13
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.
Actually, last_var must be set to zero at first place.

Previously we didn't set it because of calloc() or because it was never set by anything else in reused EG(trampoline), but when we added ext/FFI, it started to work out-of the box only because of "lucky" structure layout and we forgot to add that last_var initialization.

@iluuu1994
Copy link
Member Author

Actually, last_var must be set to zero at first place.

Sorry, just so there's no misunderstanding: Do you mean it needs to be moved up further or are you reiterating why this needs to be added? Theoretically we could move this into the if (EXPECTED(EG(trampoline).common.function_name == NULL)) { branch.

@dstogov
Copy link
Member

dstogov commented Mar 27, 2023

no, the C code is exactly right. The new comments may be confusing for people who are not familiar with the problem.

It's better to add comments that EG(trampoline) may be reused in different places (e.g. FFI) and all sensitive fields must be reinitialized.

@iluuu1994
Copy link
Member Author

EG(trampoline) is reused from other places, like FFI (e.g. zend_ffi_cdata_get_closure()) where it is used as an internal function. It may overwrite fields that don't belong to common, thus overwriting zend_op_array specific data, most significantly last_var. We need to reset this value so that it doesn't contain garbage when the engine allocates space for the next stack frame. This didn't cause any issues until now due to "lucky" structure layout.

@dstogov Is that better?

@iluuu1994 iluuu1994 closed this in 4e0bd03 Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants