Skip to content

[Filesystem] Strengthen the check of file permissions in dumpFile #54471

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

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

alexandre-daubois
Copy link
Member

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #54444
License MIT

fileperms() can fail and return false, I think we should strengthen the checks on its return value when using it to avoid undesirable behavior.

@nicolas-grekas
Copy link
Member

Thank you @alexandre-daubois.

@nicolas-grekas nicolas-grekas merged commit e483eee into symfony:5.4 Apr 4, 2024
9 of 12 checks passed
@ebuildy
Copy link
Contributor

ebuildy commented Apr 4, 2024

Fantastic this is merged ! Thanks you guys

This was referenced Apr 29, 2024
@oojacoboo
Copy link

So, somehow this change has broken our container caching layer. Looked into it a bit and I'm not even seeing any permissions issues with the actual files being dumped/written. They have user write permissions (644) and the proper user ownership.

fileperms(): stat failed for /tmp/cache/apiDevProjectContainer.php in /srv/www/vendor/symfony/filesystem/Filesystem.php:689 ErrorException called

@acoulton
Copy link
Contributor

@oojacoboo we're getting that too - I assume you have a custom error handler registered and it's intercepting the (expected) error generated when writing a non-existent file. I've proposed a fix in #54878

@oojacoboo
Copy link

@acoulton yep, custom error handler. Not quite sure why that makes a difference though. Is Symfony framework just ignoring it?

@xabbuh
Copy link
Member

xabbuh commented May 11, 2024

@oojacoboo Your custom error handler does not seem to honour if an error is silenced.

@oojacoboo
Copy link

@xabbuh thanks. We actually had error suppression handled. PHP8 changed the return on error_reporting, and it now returns the value of a bitwise operation.

https://www.php.net/manual/en/language.operators.errorcontrol.php

Was able to get it resolved. Thanks for the tip.

nicolas-grekas pushed a commit that referenced this pull request May 16, 2024
Since #54471, dumpFile will trigger a `fileperms(): stat failed`
error when writing to a filename that does not yet exist. This
was silenced from PHP's default handler with the `@` operator.

However, the error is still passed to any custom handler that the
application has registered, and can therefore cause exceptions or
spurious logging depending on the implementation of the handler.

The better solution, which is consistent with all other calls to
native functions in this class, would be to use `self::box` to
catch and ignore the potential error so that it never leaks
outside this class.
nicolas-grekas added a commit that referenced this pull request May 16, 2024
…om handler (acoulton)

This PR was merged into the 5.4 branch.

Discussion
----------

[Filesystem] Fix dumpFile `stat failed` error hitting custom handler

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | See below
| License       | MIT

Since #54471, dumpFile will trigger a `fileperms(): stat failed` error when writing to a filename that does not yet exist. This was silenced from PHP's default handler with the `@` operator.

However, the error is still passed to any custom handler that the application has registered, and can therefore cause exceptions or spurious logging depending on the implementation of the handler.

The better solution, which is consistent with all other calls to native functions in this class, would be to use `self::box` to catch and ignore the potential error so that it never leaks outside this class.

Commits
-------

247182a [Filesystem] Fix dumpFile `stat failed` error hitting custom handler
@shakaran
Copy link
Contributor

Related #57077 for 7.0.7

MindfulPol pushed a commit to MindfulPol/symfony that referenced this pull request Jun 1, 2024
Since symfony#54471, dumpFile will trigger a `fileperms(): stat failed`
error when writing to a filename that does not yet exist. This
was silenced from PHP's default handler with the `@` operator.

However, the error is still passed to any custom handler that the
application has registered, and can therefore cause exceptions or
spurious logging depending on the implementation of the handler.

The better solution, which is consistent with all other calls to
native functions in this class, would be to use `self::box` to
catch and ignore the potential error so that it never leaks
outside this class.
github-merge-queue bot pushed a commit to Lendable/composer-license-checker that referenced this pull request Jun 3, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [symfony/filesystem](https://symfony.com)
([source](https://togithub.com/symfony/filesystem)) | `6.4.6` -> `6.4.8`
|
[![age](https://developer.mend.io/api/mc/badges/age/packagist/symfony%2ffilesystem/6.4.8?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/packagist/symfony%2ffilesystem/6.4.8?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/packagist/symfony%2ffilesystem/6.4.6/6.4.8?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/packagist/symfony%2ffilesystem/6.4.6/6.4.8?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>symfony/filesystem (symfony/filesystem)</summary>

###
[`v6.4.8`](https://togithub.com/symfony/filesystem/releases/tag/v6.4.8)

[Compare
Source](https://togithub.com/symfony/filesystem/compare/v6.4.7...v6.4.8)

**Changelog**
(symfony/filesystem@v6.4.7...v6.4.8)

- bug
[symfony/symfony#54878](https://togithub.com/symfony/symfony/issues/54878)
\[Filesystem] Fix dumpFile `stat failed` error hitting custom handler
(@&#8203;acoulton)
- bug
[symfony/symfony#54759](https://togithub.com/symfony/symfony/issues/54759)
\[Filesystem] better distinguish URL schemes and Windows drive letters
([@&#8203;xabbuh](https://togithub.com/xabbuh))

###
[`v6.4.7`](https://togithub.com/symfony/filesystem/releases/tag/v6.4.7)

[Compare
Source](https://togithub.com/symfony/filesystem/compare/v6.4.6...v6.4.7)

**Changelog**
(symfony/filesystem@v6.4.6...v6.4.7)

- bug
[symfony/symfony#54471](https://togithub.com/symfony/symfony/issues/54471)
\[Filesystem] Strengthen the check of file permissions in `dumpFile`
(@&#8203;alexandre-daubois)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/Lendable/composer-license-checker).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zNzcuOCIsInVwZGF0ZWRJblZlciI6IjM3LjM3Ny44IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
symfonyaml pushed a commit to symfonyaml/symfony that referenced this pull request Oct 21, 2024
Since symfony#54471, dumpFile will trigger a `fileperms(): stat failed`
error when writing to a filename that does not yet exist. This
was silenced from PHP's default handler with the `@` operator.

However, the error is still passed to any custom handler that the
application has registered, and can therefore cause exceptions or
spurious logging depending on the implementation of the handler.

The better solution, which is consistent with all other calls to
native functions in this class, would be to use `self::box` to
catch and ignore the potential error so that it never leaks
outside this class.
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.

9 participants