Skip to content

[Cache] use copy() instead of rename() on Windows #57663

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 6, 2024
Merged

[Cache] use copy() instead of rename() on Windows #57663

merged 1 commit into from
Jul 6, 2024
+7 −2

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Jul 6, 2024

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

On Windows depending on the PHP version rename() can fail if the target file is being executed. Since the source file is not used by another process using copy() instead should be safe to be used.

@carsonbot carsonbot added this to the 5.4 milestone Jul 6, 2024
@xabbuh xabbuh mentioned this pull request Jul 6, 2024
Closed
nicolas-grekas
nicolas-grekas reviewed Jul 6, 2024
View reviewed changes
@xabbuh
use copy() instead of rename() on Windows
6cfef7e 
On Windows depending on the PHP version rename() can fail if the target
file is being executed. Since the source file is not used by another
process using copy() instead should be safe to be used.
@nicolas-grekas
Copy link
Member

Thank you @xabbuh.

@nicolas-grekas nicolas-grekas merged commit b437362 into symfony:5.4 Jul 6, 2024
@xabbuh xabbuh deleted the issue-50326 branch July 6, 2024 15:01
This was referenced Jul 26, 2024
Merged
Merged
Merged
Merged
@gggeek
Copy link

gggeek commented Jul 26, 2024

I am curious:

  • is copy an atomic operation on windows? If not, using it is not a very good idea - cache readers might get a half-written version of the file
  • does copy actually allow to overwrite a file which is being executed?
  • under which scenario would someone be executing a file straight out of the Sf cache? I would think that the cache storage is kind of 'private access'...

@gggeek
Copy link

gggeek commented Jul 26, 2024

ps: further thoughts

  • it seems the OP is conflating issues in different systems which have differenr fixes, eg. composer is running copy vs unlink as external shell processes
  • the proposed fix for php-src seems at 1st sight better than this. It would not work for all cases where the target file is executed, but at least cover the common scenario of the target file being a php script...

@mickverm mickverm mentioned this pull request Jan 22, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Bug Cache Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
4 participants
@xabbuh @nicolas-grekas @gggeek @carsonbot