Repository navigation
Fix GH-10370: File corruption in _php_stream_copy_to_stream_ex when using copy_file_range - #10440
Merged
Merged
Conversation
…n using copy_file_range copy_file_range can return early without copying all the data. This is legal behaviour and worked properly, unless the mmap fallback was used. The mmap fallback would read too much data into the destination, corrupting the destination file. Furthermore, if the mmap fallback would fail and have to fallback to the regular file copying mechanism, a similar issue would occur because both maxlen and haveread are modified. Furthermore, there was a mmap-resource in one of the failure paths of the mmap fallback code. This patch fixes these issues. This also adds regression tests using the new copy_file_range early-return simulation added in the previous commit.
ndossche
force-pushed
the
fix-10370-final
branch
from
January 24, 2023 21:38
b9ec0ac to
45fbb89
Compare
arnaud-lb
approved these changes
Jan 28, 2023
arnaud-lb
left a comment
Member
There was a problem hiding this comment.
This looks good to me!
Could you add test variants that call stream_copy_to_stream explicitly? So that these tests don't get noop'ed in case the implementation of PharData or file_put_contents change in the future.
Member
Author
|
Thanks for you review. |
This was referenced Feb 7, 2023
Member
|
Thank you! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes GH-10370
copy_file_range can return early without copying all the data. This is
legal behaviour and worked properly, unless the mmap fallback was used.
The mmap fallback would read too much data into the destination,
corrupting the destination file. Furthermore, if the mmap fallback would
fail and have to fallback to the regular file copying mechanism, a
similar issue would occur because both maxlen and haveread are modified.
Furthermore, there was a mmap-resource leak in one of the failure paths of
the mmap fallback code.
This patch fixes these issues. This also adds regression tests using the
new copy_file_range early-return simulation added in the previous
commit.
That being said, we should also look into repeating copy_file_range if the early-return happens because the operation was interrupted. But that should be a separate (feature) PR.