Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Filesystem] Strengthen the check of file permissions indumpFile#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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Mime/Tests/AbstractMimeTypeGuesserTestCase.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
nicolas-grekas commentedApr 4, 2024
Thank you@alexandre-daubois. |
ebuildy commentedApr 4, 2024
Fantastic this is merged ! Thanks you guys |
oojacoboo commentedMay 8, 2024
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. |
acoulton commentedMay 10, 2024
@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 commentedMay 11, 2024
@acoulton yep, custom error handler. Not quite sure why that makes a difference though. Is Symfony framework just ignoring it? |
xabbuh commentedMay 11, 2024
@oojacoboo Your custom error handler does not seem to honour if an error is silenced. |
oojacoboo commentedMay 11, 2024
@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. Thiswas silenced from PHP's default handler with the `@` operator.However, the error is still passed to any custom handler that theapplication has registered, and can therefore cause exceptions orspurious logging depending on the implementation of the handler.The better solution, which is consistent with all other calls tonative functions in this class, would be to use `self::box` tocatch and ignore the potential error so that it never leaksoutside 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 | MITSince#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 commentedMay 23, 2024
Related#57077 for 7.0.7 |
Sincesymfony#54471, dumpFile will trigger a `fileperms(): stat failed`error when writing to a filename that does not yet exist. Thiswas silenced from PHP's default handler with the `@` operator.However, the error is still passed to any custom handler that theapplication has registered, and can therefore cause exceptions orspurious logging depending on the implementation of the handler.The better solution, which is consistent with all other calls tonative functions in this class, would be to use `self::box` tocatch and ignore the potential error so that it never leaksoutside 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)[CompareSource](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)[CompareSource](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 therebase/retry checkbox.🔕 **Ignore**: Close this PR and you won't be reminded about this updateagain.---- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, checkthis box---This PR has been generated by [MendRenovate](https://www.mend.io/free-developer-tools/renovate/). Viewrepository 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>
Sincesymfony#54471, dumpFile will trigger a `fileperms(): stat failed`error when writing to a filename that does not yet exist. Thiswas silenced from PHP's default handler with the `@` operator.However, the error is still passed to any custom handler that theapplication has registered, and can therefore cause exceptions orspurious logging depending on the implementation of the handler.The better solution, which is consistent with all other calls tonative functions in this class, would be to use `self::box` tocatch and ignore the potential error so that it never leaksoutside 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.