Zend Abstract Interface: Method call seam (PHP 5) #1186

SammyK · 2021-03-22T22:16:00Z

Description

The Zend Abstract Interface (ZAI) is an API for calling into PHP from C. The seams are decoupled from the main PHP tracer and tested using the ZAI SAPI.

This PR includes the first ZAI seam on PHP 5 for safely calling methods (sandboxed):

zai_class_lookup()
zai_call_method_without_args()
zai_call_static_method_without_args()

This seam will be used to finish the implementation of autoflushing in #1189.

Readiness checklist

(only for Members) Changelog has been added to the release document.
Tests added for this feature/bug.

Reviewer checklist

Appropriate labels assigned.
Milestone is set.
Changelog has been added to the release document. For community contributors the reviewer is in charge of this task.

zend_abstract_interface/functions/tests/functions.cc

zend_abstract_interface/zai_sapi/zai_sapi.c

labbati

Love to see this coming 😄

config.m4

dockerfiles/ci/alpine/php-8.0/Dockerfile

morrisonlevi

I am unable to build the project for PHP 5.6-debug, though I don't think the debug part is affecting it. CMake is unable to locate libphp5.so. This makes sense to me, because that's an artifact of the embed SAPI, which I didn't build:

/opt/php/5.6-debug/bin/php-config --php-sapis

cli fpm phpdbg cgi

However, I thought using zai meant we no longer needed the embed SAPI?

SammyK · 2021-05-04T13:29:35Z

However, I thought using zai meant we no longer needed the embed SAPI?

The embed SAPI is only required to run the tests with the ZAI SAPI. Although we don't use the embed SAPI directly, ZAI SAPI requires access to the PHP shared object which, as you mentioned, is only built when the embed SAPI is enabled. So the embed SAPI is required to run the tests.

Ideally we should be able to build PHP with a config flag like --build-shared that would build the shared object without needing to enable the embed SAPI. That's something I've got on my TODO list to investigate for a 8.1 change.

zend_abstract_interface/methods/php5/methods.c

morrisonlevi · 2021-05-06T20:34:49Z

zend_abstract_interface/methods/php5/methods.c

+    zai_sandbox sandbox;
+    zai_sandbox_open(&sandbox);
+
+    zend_try {


Couldn't this be a problem if we catch bailouts in tracing closure callbacks? I assume bailouts happen because they can't recover, so does this need to "re-throw" the bailout? I'm thinking about ext/soap or something. I don't know, we don't intend to call any such functions, do we? Does exit use bailout on these versions?

Hum. I think you're right that we cannot safely catch arbitrary zend_bailouts and go on pretending like nothing happened. In the ext/soap case, zend_bailouts are caught and converted into exceptions, but as you pointed out that's not the case for exit. (In PHP 8 exit is a clean shutdown thanks to the uncatchable Chuck Norris exception, UnwindExit. 😄) But we can expect an arbitrary unclean shutdown here when exit is used or some fatal error. If we ignored it, this would lead to a lot of ZMM leaks and likely a number of real memory leaks.

So although it is technically a "hole in the sandbox" to remove the zend_try here, it's actually safer to not catch them. If we wanted a true sandbox, we could run these as "sub requests" like ext/sandbox does. But I'm not sure the overhead of something like this would be worth it - especially since we're reducing our C-to-PHP seams.

Very nice catch on this (pun intended). I'll remove this and update the tests.

Do we need to remove it, or do we need to catch it, undo the sandbox and then re-throw?

Yes, 👆 that. 👍

This has been addressed in ff1823c.

morrisonlevi · 2021-05-07T14:48:12Z

zend_abstract_interface/methods/methods.h

+ * Methods cannot be called outside of a request context so this MUST be called
+ * from within a request context (after RINIT and before RSHUTDOWN). A crash
+ * will occur if this is called outside of a request context.
+ */
+bool zai_call_method_without_args_ex(zval *object, const char *method, size_t method_len, zval **retval TSRMLS_DC);


Can we properly detect request context and return false if we're not inside one? Then we can run tests since they won't crash. If this is too expensive then we can consider moving it to debug-only or something.

Good idea. I've aded this check in e346b91. I believe this only applies to static method calls since a crash will occur when trying to instantiate an object outside of a request context. So I updated the formally crashy test to a static method call test.

morrisonlevi

Thanks for addressing my concerns. I'm glad that you documented many of the sharp edges so we know why the code is written that way; this was something sorely needed.

SammyK · 2021-05-10T17:55:35Z

Thanks for such a high quality code review @morrisonlevi. 💯

SammyK added 🏆 enhancement A new feature or improvement c-extension Apply this label to issues and prs related to the C-extension labels Mar 22, 2021

SammyK force-pushed the sammyk/zend-abstract-interface branch 5 times, most recently from 6528aa6 to 13e9614 Compare March 23, 2021 21:58

morrisonlevi reviewed Mar 23, 2021

View reviewed changes

zend_abstract_interface/functions/tests/functions.cc Outdated Show resolved Hide resolved

morrisonlevi reviewed Mar 23, 2021

View reviewed changes

zend_abstract_interface/zai_sapi/zai_sapi.c Outdated Show resolved Hide resolved

labbati reviewed Mar 24, 2021

View reviewed changes

config.m4 Outdated Show resolved Hide resolved

dockerfiles/ci/alpine/php-8.0/Dockerfile Outdated Show resolved Hide resolved

Base automatically changed from sammyk/php8/fix-curl-symbol to master March 26, 2021 21:04

SammyK changed the title ~~Zend Abstract Interface~~ Zend Abstract Interface + ZAI SAPI Mar 26, 2021

SammyK force-pushed the sammyk/zend-abstract-interface branch 4 times, most recently from 932fe70 to 49b0ec6 Compare April 12, 2021 22:03

SammyK changed the title ~~Zend Abstract Interface + ZAI SAPI~~ Zend Abstract Interface Apr 14, 2021

SammyK mentioned this pull request Apr 14, 2021

ZAI SAPI #1204

Merged

7 tasks

SammyK force-pushed the sammyk/zend-abstract-interface branch from 49b0ec6 to 766a401 Compare April 16, 2021 19:27

SammyK changed the base branch from master to sammyk/zai-sapi-test-helpers April 16, 2021 19:27

SammyK changed the title ~~Zend Abstract Interface~~ Zend Abstract Interface: Method call seam (PHP 5) Apr 16, 2021

SammyK added this to the 0.58.0 milestone Apr 16, 2021

SammyK force-pushed the sammyk/zend-abstract-interface branch from 766a401 to 732eced Compare April 16, 2021 19:35

Base automatically changed from sammyk/zai-sapi-test-helpers to master April 19, 2021 13:31

labbati modified the milestones: 0.58.0, 0.59.0 Apr 21, 2021

SammyK force-pushed the sammyk/zend-abstract-interface branch from 732eced to 954dd6f Compare April 29, 2021 22:54

SammyK changed the base branch from master to sammyk/zai/sandbox April 29, 2021 22:54

SammyK force-pushed the sammyk/zend-abstract-interface branch from 954dd6f to 6293b4e Compare April 30, 2021 14:14

SammyK marked this pull request as ready for review April 30, 2021 14:59

SammyK mentioned this pull request Apr 30, 2021

Auto-flushing (PHP 5) #1189

Merged

5 tasks

morrisonlevi reviewed May 3, 2021

View reviewed changes

labbati modified the milestones: 0.59.0, 0.60.0 May 5, 2021

Base automatically changed from sammyk/zai/sandbox to master May 6, 2021 19:38

SammyK added 3 commits May 6, 2021 15:41

Add ZAI method-call seam (PHP 5)

0fa8a62

Add tests for userland exceptions

727c7de

Fail sandboxed calls with unhandled exceptions

6bbb941

SammyK force-pushed the sammyk/zend-abstract-interface branch from ebf6fca to 6bbb941 Compare May 6, 2021 19:41

morrisonlevi reviewed May 6, 2021

View reviewed changes

SammyK added 4 commits May 7, 2021 09:31

Add class lookup assertions for lowercase and prefix

62c9b73

Add test for disable_classes INI

b7bd824

Test for class lookup outside of request context

3f8a405

Fix lint

c7d2fe8

morrisonlevi reviewed May 7, 2021

View reviewed changes

SammyK added 3 commits May 10, 2021 11:54

Allow zend_bailout to bubble up

ff1823c

Prevent methods from being called outside of a request context

e346b91

Remove comment about crash (no longer relevant)

469b542

morrisonlevi approved these changes May 10, 2021

View reviewed changes

SammyK merged commit f4b5065 into master May 10, 2021

SammyK deleted the sammyk/zend-abstract-interface branch May 10, 2021 17:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Zend Abstract Interface: Method call seam (PHP 5) #1186

Zend Abstract Interface: Method call seam (PHP 5) #1186

SammyK commented Mar 22, 2021 •

edited by morrisonlevi

labbati left a comment

morrisonlevi left a comment

SammyK commented May 4, 2021

morrisonlevi May 6, 2021

SammyK May 7, 2021

morrisonlevi May 7, 2021

SammyK May 7, 2021

SammyK May 10, 2021

morrisonlevi May 7, 2021

SammyK May 10, 2021

morrisonlevi left a comment

SammyK commented May 10, 2021

Zend Abstract Interface: Method call seam (PHP 5) #1186

Zend Abstract Interface: Method call seam (PHP 5) #1186

Conversation

SammyK commented Mar 22, 2021 • edited by morrisonlevi

Description

Readiness checklist

Reviewer checklist

labbati left a comment

Choose a reason for hiding this comment

morrisonlevi left a comment

Choose a reason for hiding this comment

SammyK commented May 4, 2021

morrisonlevi May 6, 2021

Choose a reason for hiding this comment

SammyK May 7, 2021

Choose a reason for hiding this comment

morrisonlevi May 7, 2021

Choose a reason for hiding this comment

SammyK May 7, 2021

Choose a reason for hiding this comment

SammyK May 10, 2021

Choose a reason for hiding this comment

morrisonlevi May 7, 2021

Choose a reason for hiding this comment

SammyK May 10, 2021

Choose a reason for hiding this comment

morrisonlevi left a comment

Choose a reason for hiding this comment

SammyK commented May 10, 2021

SammyK commented Mar 22, 2021 •

edited by morrisonlevi