Skip to content

[SOAP] Temporary WSDL cache files not being deleted #12838

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
Lyrkan opened this issue Nov 30, 2023 · 10 comments
Closed

[SOAP] Temporary WSDL cache files not being deleted #12838

Lyrkan opened this issue Nov 30, 2023 · 10 comments

Comments

@Lyrkan
Copy link

Lyrkan commented Nov 30, 2023

Description

Hi there,

After upgrading some servers from PHP 8.1.25 to 8.1.26 I started receiving alerts that their disk usage was increasing unusually fast due to thousands of tmp.wsdl.* files being created in the WSDL cache folder.

From what I've seen those temporary files were added in #12469 but I think it may be related to another issue I had before.

PHP scripts from my app are owned by an user called admin and may be executed by two different users :

  • admin (CLI)
  • www-data (PHP-FPM).

In both cases SOAP APIs may be called which will result in the WSDL cache being populated.
From my understanding, the filenames for those files depend on a php_get_current_user() call which doesn't return the process owner but the owner of the script, which means that in both cases (CLI or PHP-FPM) the files will have the same name.

Since - at least in my case - they are created in the /tmp directory with 600 permissions, only the owner of the process that was used to create them can also read them. So, I basically end-up with files called wsdl-admin-* owned by the first user that populated the WSDL cache (ex: admin) and not readable by the other one (ex: www-data). I'm not sure why it worked before 8.1.26, I'm guessing that it probably didn't but failed silently.

What I think happens next is that the new code from #12469 tries to move the tmp.wsdl.* files to wsdl-admin-<hash> but does not have the permission to do so.

So, two questions:

  • is there a reason the cache files are named based on the script file owner and not the PHP process owner?
  • shouldn't the tmp.wsdl.* files be removed if they could not be renamed?

PHP Version

PHP 8.1.26

Operating System

Ubuntu 20.04

@nielsdos
Copy link
Member

Thanks for the report! Your analysis seems right.

From my understanding, the filenames for those files depend on a php_get_current_user() call which doesn't return the process owner but the owner of the script, which means that in both cases (CLI or PHP-FPM) the files will have the same name.

It's actually even worse. In case the SAPI has set up a custom user to run under then it will return that user... So it's quite inconsistent. (except on Windows where it will always return the current user).

is there a reason the cache files are named based on the script file owner and not the PHP process owner?

Seems like an oversight to me. I'm not going to touch this for stable branches, but I'll take a look to fix this on master.

shouldn't the tmp.wsdl.* files be removed if they could not be renamed?

Yeah that should fix your issue.

I'm not sure why it worked before 8.1.26, I'm guessing that it probably didn't but failed silently.

Yeah the caching just didn't work before. This should be fixable by taking the right username, something that I'll look into for master. For stable branches I'll make sure the temp files are removed.

nielsdos added a commit to nielsdos/php-src that referenced this issue Nov 30, 2023
If there are two users that can execute the script that caches a WSDL,
but the script is owned by a single user, then the caching code will
name the cached file with the file owner username and a hash of the uri.
When one of the two tries to rename the file created by the other
process, this does not work because it has no permission to do so.
This then leaves temporary files floating in the temp directory.

To fix the immediate problem, unlink the file after rename has failed.
On the long term, this has to be fixed by taking the username of the
process instead of the username of the file owner.
@Lyrkan
Copy link
Author

Lyrkan commented Dec 1, 2023

Hi @nielsdos,

Thanks for taking care of it.

For now as a workaround I moved cache files to their own directory and handle permissions on them using ACLs, seems to do the trick :)

nielsdos added a commit that referenced this issue Dec 1, 2023
* PHP-8.2:
  Fix GH-12838: [SOAP] Temporary WSDL cache files not being deleted
nielsdos added a commit that referenced this issue Dec 1, 2023
* PHP-8.3:
  Fix GH-12826: Weird pointers issue in nested loops
  Fix GH-12838: [SOAP] Temporary WSDL cache files not being deleted
@nikoelgatito
Copy link

Hi @nielsdos,

We recently updated an application to use php 8.1 and we are experiencing a similar issue.

After reviewing the content of the file below for the latest php 8.1 version, I can see that the commit 4eee81b was never applied.

Affected File:
https://github.com/php/php-src/blob/php-8.1.32/ext/soap/php_sdl.c

Is there a specific reason why this commit never made it to the stable php 8.1 version?

@nielsdos
Copy link
Member

nielsdos commented May 6, 2025

Hi @nielsdos,

We recently updated an application to use php 8.1 and we are experiencing a similar issue.

After reviewing the content of the file below for the latest php 8.1 version, I can see that the commit 4eee81b was never applied.

Affected File: https://github.com/php/php-src/blob/php-8.1.32/ext/soap/php_sdl.c

Is there a specific reason why this commit never made it to the stable php 8.1 version?

Support for 8.1 ended before the fix was made.

@nikoelgatito
Copy link

Hi @nielsdos,
We recently updated an application to use php 8.1 and we are experiencing a similar issue.
After reviewing the content of the file below for the latest php 8.1 version, I can see that the commit 4eee81b was never applied.
Affected File: https://github.com/php/php-src/blob/php-8.1.32/ext/soap/php_sdl.c
Is there a specific reason why this commit never made it to the stable php 8.1 version?

Support for 8.1 ended before the fix was made.

I appreciate your quick response.

Indeed, I can see that the active support period ended 5 days before this issue was reported.

Since this issue may result in a space saturation of the /tmp directory, which would be causing a system crash, could it be considered a security fix?

@nielsdos
Copy link
Member

nielsdos commented May 6, 2025

Since this issue may result in a space saturation of the /tmp directory, which would be causing a system crash, could it be considered a security fix?

I don't think so as it doesn't fit the threat model.
However, we have a policy nowadays of allowing certain backports to security releases in some cases, so we could ask the release managers.

@nikoelgatito
Copy link

Since this issue may result in a space saturation of the /tmp directory, which would be causing a system crash, could it be considered a security fix?

I don't think so as it doesn't fit the threat model. However, we have a policy nowadays of allowing certain backports to security releases in some cases, so we could ask the release managers.

Thank you for sharing this information.

As some applications perform large amounts of SOAP requests in a short period of time, caching is particularly important and it is filling up the /tmp directory quickly.

Considering that this change is exceptionally small and has been in use for quite some time in versions 8.2+, backporting it should not come with any risk.

Therefore, it would be greatly appreciated if you could ask the release managers if they could allow this backport.

@nielsdos
Copy link
Member

nielsdos commented May 6, 2025

Cc @ramsey @patrickallaert if you agree I'll cherry pick the regression fix into 8.1.

@bukka
Copy link
Member

bukka commented May 9, 2025

However, we have a policy nowadays of allowing certain backports to security releases in some cases, so we could ask the release managers.

Technically this is not allowed in year 4 as per https://github.com/php/policies/blob/main/release-process.rst#bug-fix-and-security-releases . So RM cannot currently make such exception.

I think it would make sense to update policy as we also started making some build updates and it would make sense to backport fixes like this. I will look into it as I wanted to add some other updates for some time.

@bukka
Copy link
Member

bukka commented May 9, 2025

Ok this gave me a bit motivation to do that RFC so here it is: https://wiki.php.net/rfc/policy-release-process-update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants