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

File corruption in _php_stream_copy_to_stream_ex when using copy_file_range #10370

Closed
johnsudaar opened this issue Jan 18, 2023 · 6 comments
Closed

Comments

@johnsudaar
Copy link

johnsudaar commented Jan 18, 2023

Description

Recently a user notified us of a corrupted file after a composer install on our platform while using PHP 8.2.1.

It seems that under some condition the _php_stream_copy_to_stream_ex can corrupt files.

I am not a PHP expert but here is the information I have found.

If I run the following code:

<?php
$archive = new PharData("./01fd644d9004da280847edbe58bb0346773cffaa.tar");
var_dump($archive->extractTo("./out", ["src/Versions/Version.php"]));

I get this output on some of our servers:

[14:36] Scalingo: /tmp/server-a $ php test.php 
bool(true)
[14:37] Scalingo: /tmp/server-a $ sha512sum out/src/Versions/Version.php 
ef8b2f794a94bbf45e4e4a1ed12a0edf5d403a6216cb23dcddb5fee894b7c6ee7ea73a92d5cc0a567343d9861a018f6a5439033f09e2d4ecbeb0345894b9c611  out/src/Versions/Version.php

But I expected this output instead:

[14:35] Scalingo: /tmp/server-b $ php test.php 
bool(true)
[14:37] Scalingo: /tmp/server-b $ sha512sum out/src/Versions/Version.php
1fe10db185b013570e15147991377c36afe74bae2a2b799589aac0add6e51d33c943cfc80bf57da52c2bfdd393b632ba2658b289fe0df97ae72b93a8659a96fa  out/src/Versions/Version.php

The file outputed on the first server is clearly corrupted when on the second server it's the expected output.

After some investigation I think that we traced back the issue to the copy_file_range call in streams/streams.c.

On system where corruption is occurring, only a part of the requested length is read, where on systems where no corruption is occurring the entire requested length is read.

Here are part of the strace that seems relevant to me.

The strace which results in corrupted file:

openat(AT_FDCWD, "/tmp/server-a/out/src/Versions/Version.php", O_RDWR|O_CREAT|O_TRUNC, 0666) = 6
fstat(6, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0
lseek(6, 0, SEEK_CUR)                   = 0
lseek(5, 73728, SEEK_SET)               = 73728
lseek(5, 73728, SEEK_SET)               = 73728
lseek(6, 0, SEEK_SET)                   = 0
copy_file_range(5, NULL, 6, NULL, 5667, 0) = 4096
fstat(5, {st_mode=S_IFREG|0600, st_size=99840, ...}) = 0
mmap(NULL, 5667, PROT_READ, MAP_SHARED, 5, 0x13000) = 0x7f6d150e6000
lseek(5, 83491, SEEK_SET)               = 83491
write(6, "Step): void\n    {\n        $this-"..., 5667) = 5667
munmap(0x7f6d150e6000, 5667)            = 0
close(6)                                = 0

The strace that results in a valid file:

openat(AT_FDCWD, "/tmp/server-b/out/src/Versions/Version.php", O_RDWR|O_CREAT|O_TRUNC, 0666) = 6
fstat(6, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0
lseek(6, 0, SEEK_CUR)                   = 0
lseek(5, 73728, SEEK_SET)               = 73728
lseek(5, 73728, SEEK_SET)               = 73728
lseek(6, 0, SEEK_SET)                   = 0
copy_file_range(5, NULL, 6, NULL, 5667, 0) = 5667
close(6)                                = 0
chmod("./out/src/Versions/Version.php", 0644) = 0

When it resulted in a valid file, it seems that copy_file_range copied the entire requested length (5667 bytes).

But on the corrupted version, it looks like it copied a part of the file (4096 bytes). Then it switched to the mmap method to copy the rest of the file.

I'm really neither an expert on PHP internals nor on system calls so the following might be completely wrong. But my understanding, seems to be that the mmap call copies too much data:

mmap(NULL, 5667, PROT_READ, MAP_SHARED, 5, 0x13000) = 0x7f6d150e6000

Seems to ask 5667 bytes on the FD 5. But the FD already read 4096 bytes so shouldn't we read 4096 - 5667 = 1571 bytes?
This seems to be confirmed by the seek that comes after.
The base offset seems to be 73728 (cf. the seek before copy_file_range) so the next seek should be 73728 + 5667 = 79395.
But according to the strace, it seeks to 83491 (73728 + 5667 + 4096).

A solution would be to subtract the result bytes from the bytes read in the mmap.
Or to do a loop around copy_file_range that would keep calling that method until it read the entire requested length.

The thing we do not understand is why copy_file_range is returning 4096 on some systems. Because on almost all systems it copies the entire file even if the environment are pretty close (both are on Ubuntu 20.04, Using the same kernel branch, code run in a docker container, ..). That's why it's a bit hard to give some precise instructions on how to reproduce the issue.

Additional information

On both servers PHP version is:

PHP 8.2.1 (cli) (built: Jan 16 2023 18:08:45) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.2.1, Copyright (c) Zend Technologies
    with Zend OPcache v8.2.1, Copyright (c), by Zend Technologies

PHP Version

PHP 8.2.1

Operating System

Ubuntu 20.04

@nielsdos
Copy link
Member

Thanks for the issue report.
Are you able to share the reproducer .tar file you're referencing to in your code example? I'd like to work on this issue :)
Even if I can't trigger the non-full-copy, I can still emulate it by cheating a bit with the syscall parameters so I think there's a good chance I can reproduce this issue if I have an input file.

@nielsdos
Copy link
Member

nielsdos commented Jan 18, 2023

No need for a reproducer anymore, I can reproduce it using an artificial test file and artificial modification to my system.
It only happens when the mmap succeeds, so when your file starts at exactly a multiple of 0x1000 bytes...
The error is related to this line:
https://github.com/php/php-src/blob/PHP-8.2.1/main/streams/streams.c#L1650
This copies too much data because it doesn't account for the offset. If I manually specify the right amount of bytes to copy it works. I'll try to work on a fix.

@nielsdos
Copy link
Member

nielsdos commented Jan 18, 2023

The following patch works for me. I agree that we should also look into repeating copy_file_range though. But the fact that the mmap method after the copy_file_range method didn't work is a bug on its own because fallbacks should work...

diff --git a/main/streams/streams.c b/main/streams/streams.c
index 20029fc73e..28fa7cb651 100644
--- a/main/streams/streams.c
+++ b/main/streams/streams.c
@@ -1634,9 +1634,15 @@ PHPAPI zend_result _php_stream_copy_to_stream_ex(php_stream *src, php_stream *de
 		char *p;
 
 		do {
-			size_t chunk_size = (maxlen == 0 || maxlen > PHP_STREAM_MMAP_MAX) ? PHP_STREAM_MMAP_MAX : maxlen;
-			size_t mapped;
+			size_t must_read = maxlen - haveread;
+			size_t chunk_size = (maxlen == 0 || must_read > PHP_STREAM_MMAP_MAX) ? PHP_STREAM_MMAP_MAX : must_read;
+
+			/* Prevent mapped from becoming larger than the file */
+			if (chunk_size > must_read) {
+				chunk_size = must_read;
+			}
 
+			size_t mapped;
 			p = php_stream_mmap_range(src, php_stream_tell(src), chunk_size, PHP_STREAM_MAP_MODE_SHARED_READONLY, &mapped);
 
 			if (p) {
@@ -1650,6 +1656,7 @@ PHPAPI zend_result _php_stream_copy_to_stream_ex(php_stream *src, php_stream *de
 				didwrite = php_stream_write(dest, p, mapped);
 				if (didwrite < 0) {
 					*len = haveread;
+					php_stream_mmap_unmap(src);
 					return FAILURE;
 				}
 
@@ -1666,9 +1673,9 @@ PHPAPI zend_result _php_stream_copy_to_stream_ex(php_stream *src, php_stream *de
 				if (mapped < chunk_size) {
 					return SUCCESS;
 				}
-				if (maxlen != 0) {
-					maxlen -= mapped;
-					if (maxlen == 0) {
+				if (must_read != 0) {
+					must_read -= mapped;
+					if (must_read == 0) {
 						return SUCCESS;
 					}
 				}

Could you please test the patch too if you're able to :) ? You can apply the patch on top of the PHP-8.2 branch using the git apply command. Thanks!

Note: the patch also contains a fix for a mmap-resource leak. It also prevents modifying maxlen such that the fallback after mmap works too.
Note2: the patch doesn't account for the case where maxlen==0 yet, so it isn't ready as-is but should be enough to test if the approach works to solve your issue. So please only use this patch to test if the approach fixes your issue and not use this patch yet on a production environment.

@chayutBuarit
Copy link

FB_IMG_1659266425780.jpg

@johnsudaar
Copy link
Author

Sorry for the late answer it seems that your answer got lost in my mailbox.

First thanks a lot for taking the time to work on that issue.

Here are the steps that I took in order to test the issue:

  1. Clone the repository
  2. Switch to the PHP-8.2.1 branch
  3. Build PHP
  4. Confirm that the bug is still there (it still was there)
  5. git apply 10370.patch
  6. Build PHP again
  7. Run the same test as step 4

And it seemed to work !

$ ./php/php-src/sapi/cli/php test.php 
bool(true)

$ sha512sum out/src/Versions/Version.php
1fe10db185b013570e15147991377c36afe74bae2a2b799589aac0add6e51d33c943cfc80bf57da52c2bfdd393b632ba2658b289fe0df97ae72b93a8659a96fa  out/src/Versions/Version.php

Which is the correct checksum (I also manually checked the file and it's valid)

It looks like your patch is indeed fixing the issue we're having!

@nielsdos
Copy link
Member

No worries, thank you for testing!
I'll make sure to clean up the patch this week and submit a PR :)

nielsdos added a commit to nielsdos/php-src that referenced this issue Jan 24, 2023
…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.
nielsdos added a commit to nielsdos/php-src that referenced this issue Jan 24, 2023
…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.
nielsdos added a commit to nielsdos/php-src that referenced this issue Jan 28, 2023
arnaud-lb added a commit that referenced this issue Feb 10, 2023
* PHP-8.2:
  Fix concurrent testing
  [ci skip] NEWS
  Fix GH-10370: File corruption in _php_stream_copy_to_stream_ex when using copy_file_range (#10440)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants