Skip to content
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

fix: add guards to oapi cufa opcode check #708

Merged
merged 3 commits into from
Aug 9, 2023
Merged

Conversation

ZNeumann
Copy link
Contributor

@ZNeumann ZNeumann commented Aug 2, 2023

When instrumenting wordpress/drupal hooks, we want to instrument the hook function calls, which are dispatched by php's call_user_function_array. This php function (cufa for short) is flattened/inlined, so to determine if we are in a function called by a cufa call, we need to check the opcodes of the caller to the current function. If the caller is a php internal function, checking these opcodes was not safe and lead to invalid reads. As such, before reading the opcodes, we first check if the caller is a user function. We only want to instrument cufa calls from user functions anyway.

@zsistla zsistla added this to the OAPI Instrumentation milestone Aug 3, 2023
agent/php_execute.c Outdated Show resolved Hide resolved
agent/php_execute.c Outdated Show resolved Hide resolved
Copy link
Member

@lavarou lavarou left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Co-authored-by: Michal Nowacki <[email protected]>
Copy link
Contributor

@zsistla zsistla left a comment

Choose a reason for hiding this comment

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

Great job getting the issue fixed.

@ZNeumann ZNeumann merged commit dca53bd into oapi Aug 9, 2023
38 of 57 checks passed
@lavarou lavarou deleted the zjn/oapi-cufa-fix branch August 9, 2023 16:14
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.

3 participants