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

ext/soap: Don't call readfile() userland function #17432

Merged
merged 6 commits into from
Jan 15, 2025

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Jan 10, 2025

We can perform the operation directly, moreover there is no risk of a user disabling the readfile function and defining their own messing up what we are doing.

@Girgias Girgias marked this pull request as ready for review January 10, 2025 14:19
@Girgias Girgias requested a review from nielsdos as a code owner January 10, 2025 14:19
ext/soap/soap.c Outdated Show resolved Hide resolved
ext/soap/soap.c Show resolved Hide resolved
We can perform the operation directly, moreover there is no risk of a user disabling the readfile function and defining their own messing up what we are doing.
@Girgias Girgias force-pushed the soap-readline-direct branch from 75a8bfe to 6eb1155 Compare January 11, 2025 14:38
@Girgias Girgias requested a review from nielsdos January 11, 2025 14:38
ext/soap/soap.c Outdated
ZVAL_STRING(&param, service->sdl->source);
ZVAL_STRING(&readfile, "readfile");
if (call_user_function(EG(function_table), NULL, &readfile, &readfile_ret, 1, &param ) == FAILURE) {
php_stream *stream = php_stream_open_wrapper_ex(service->sdl->source, "rb", REPORT_ERRORS, NULL, /* context */ NULL);
Copy link
Member

Choose a reason for hiding this comment

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

Please use the default stream context

Copy link
Member

Choose a reason for hiding this comment

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

Pleas also fix WIN64 tests failures.

@Girgias
Copy link
Member Author

Girgias commented Jan 13, 2025

@cmb69 I don't really understand why it seems like the SOAP extension is not being loaded in the new soap test

@cmb69
Copy link
Member

cmb69 commented Jan 13, 2025

I think the problem is that --GET-- spawns php-cgi, but -dextension=soap is only passed to the php(-cli) process. See #16084 (likely the same problem). If I'm right, that would also happen on other OSs if ext/soap is built as shared library.

@Girgias
Copy link
Member Author

Girgias commented Jan 13, 2025

I think the problem is that --GET-- spawns php-cgi, but -dextension=soap is only passed to the php(-cli) process. See #16084 (likely the same problem). If I'm right, that would also happen on other OSs if ext/soap is built as shared library.

Right, this is slightly annoying :/ I guess the easy fix is to just skip those tests on Windows, but we should open an issue to track this IMHO.

@cmb69
Copy link
Member

cmb69 commented Jan 14, 2025

If I'm right, that would also happen on other OSs if ext/soap is built as shared library.

Nope, has nothing to do with shared libs – Windows specific issue.

The problem is that SoapServer expects "wsdl" as query string to deliver the WSDL. However, if a query string doesn't contain an equals sign, command line options are ignored on Windows. So either hack-around by making SoapServer more deliberate:

 ext/soap/soap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ext/soap/soap.c b/ext/soap/soap.c
index 48a7fc8885..d7dfc4ecd5 100644
--- a/ext/soap/soap.c
+++ b/ext/soap/soap.c
@@ -1297,7 +1297,8 @@ PHP_METHOD(SoapServer, handle)
 	if (SG(request_info).request_method &&
 	    strcmp(SG(request_info).request_method, "GET") == 0 &&
 	    SG(request_info).query_string &&
-	    stricmp(SG(request_info).query_string, "wsdl") == 0) {
+	    (stricmp(SG(request_info).query_string, "wsdl") == 0 ||
+	     stricmp(SG(request_info).query_string, "wsdl=") == 0)) {
 
 		if (service->sdl) {
 /*

or apply a proper fix for the tests, namely to spawn a php-cgi process with the command line options, and then send a CGI request and verify the response. Certainly possible, but I'm not sure it's worth the effort.

@Girgias
Copy link
Member Author

Girgias commented Jan 14, 2025

I've made a new issue to track the WSDL query issue on Windows, and have the new tests just skip on Windows for now.

@Girgias Girgias requested a review from nielsdos January 14, 2025 12:04
@cmb69
Copy link
Member

cmb69 commented Jan 14, 2025

I've made a new issue to track the WSDL query issue on Windows, and have the new tests just skip on Windows for now.

Makes sense to me, especially since we now have a proper explanation why the test is skipped on Windows.

@Girgias Girgias merged commit b7169a2 into php:master Jan 15, 2025
1 check passed
@Girgias Girgias deleted the soap-readline-direct branch January 15, 2025 14:37
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.

4 participants