Skip to content
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

Fix streams copy length after partial copy_file_range. #10538

Closed
wants to merge 1 commit into from

Conversation

roptat
Copy link

@roptat roptat commented Feb 7, 2023

Hi,

it seems that on some file systems (here, BTRFS), copy_file_range succeeds but does not copy the whole file. Then, we fall back to a second method that tries to copy maxlen bytes from the current location. Since the copy succeeded in the previous attempt, the file pointer was moved, but maxlen is not updated, so more bytes than expected are copied from the file.

This caused a failure for me in several phar tests, as ext/phar/phar.c checks for how much was copied, in addition to the success returned by the function, even in that case. After applying the patch, these tests now all pass.

If the first method is not attempted, fails or copies 0 bytes, haveread stays at 0, so maxlen is not affected. If enough bytes were copied, the function returns immediately. If the first copy is partial, maxlen is updated to reflect how much bytes should still be copied.

@nielsdos
Copy link
Member

nielsdos commented Feb 7, 2023

This fix doesn't work properly in all cases (e.g. maxlen == PHP_STREAM_COPY_ALL). Furthermore there is a bug with the mmap fallback as well (maxlen must not be modified or otherwise this causes issues as well for the file copy loop below the mmap fallback).
I already fixed these issues in PR #10440 which is already approved but not yet merged.

@roptat
Copy link
Author

roptat commented Feb 7, 2023

Oh, I see, thanks for letting me know! I hope your changes get merged soon :)

@roptat roptat closed this Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants