-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Conversation
610ccff
to
e91d15d
Compare
src/Symfony/Component/Mime/Tests/AbstractMimeTypeGuesserTestCase.php
Outdated
Show resolved
Hide resolved
e91d15d
to
e6a1087
Compare
e6a1087
to
0c17a4e
Compare
Thank you @alexandre-daubois. |
Fantastic this is merged ! Thanks you guys |
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.
|
@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 |
@acoulton yep, custom error handler. Not quite sure why that makes a difference though. Is Symfony framework just ignoring it? |
@oojacoboo Your custom error handler does not seem to honour if an error is silenced. |
@xabbuh thanks. We actually had error suppression handled. PHP8 changed the return on https://www.php.net/manual/en/language.operators.errorcontrol.php Was able to get it resolved. Thanks for the tip. |
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.
…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
Related #57077 for 7.0.7 |
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.
[](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` | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](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 (@​acoulton) - bug [symfony/symfony#54759](https://togithub.com/symfony/symfony/issues/54759) \[Filesystem] better distinguish URL schemes and Windows drive letters ([@​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` (@​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>
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.
fileperms()
can fail and returnfalse
, I think we should strengthen the checks on its return value when using it to avoid undesirable behavior.