Skip to content

Commit 45fbb89

Browse files
committed
Fix GH-10370: File corruption in _php_stream_copy_to_stream_ex when 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.
1 parent e1ff730 commit 45fbb89

File tree

4 files changed

+78
-4
lines changed

4 files changed

+78
-4
lines changed

ext/zend_test/tests/gh10370.tar

20 KB
Binary file not shown.

ext/zend_test/tests/gh10370_1.phpt

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
--TEST--
2+
GH-10370: File corruption in _php_stream_copy_to_stream_ex when using copy_file_range - partial copy
3+
--EXTENSIONS--
4+
zend_test
5+
phar
6+
--SKIPIF--
7+
<?php
8+
if (PHP_OS != 'Linux') {
9+
die('skip For Linux only');
10+
}
11+
?>
12+
--INI--
13+
zend_test.limit_copy_file_range=3584
14+
--FILE--
15+
<?php
16+
/* Note: the value 3584 is chosen so that the mmap in _php_stream_copy_to_stream_ex() will mmap
17+
* at an offset of a multiple of 4096, which is the standard page size in most Linux systems. */
18+
$archive = new PharData(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370.tar');
19+
var_dump($archive->extractTo(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370', ['testfile']));
20+
var_dump(sha1_file(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370' . DIRECTORY_SEPARATOR . 'testfile'));
21+
?>
22+
--EXPECT--
23+
bool(true)
24+
string(40) "34e163be8e43c5631d8b92e9c43ab0bf0fa62b9c"
25+
--CLEAN--
26+
<?php
27+
@unlink(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370' . DIRECTORY_SEPARATOR . 'testfile');
28+
@rmdir(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370');
29+
?>

ext/zend_test/tests/gh10370_2.phpt

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
--TEST--
2+
GH-10370: File corruption in _php_stream_copy_to_stream_ex when using copy_file_range - unlimited copy
3+
--EXTENSIONS--
4+
zend_test
5+
--SKIPIF--
6+
<?php
7+
if (PHP_OS != 'Linux') {
8+
die('skip For Linux only');
9+
}
10+
?>
11+
--INI--
12+
zend_test.limit_copy_file_range=4096
13+
--FILE--
14+
<?php
15+
/* Note: the value 4096 is chosen so that the mmap in _php_stream_copy_to_stream_ex() will mmap
16+
* at an offset of a multiple of 4096, which is the standard page size in most Linux systems. */
17+
$input_file = fopen(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370.tar', 'r');
18+
file_put_contents(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370_out.tar', $input_file);
19+
fclose($input_file);
20+
21+
var_dump(sha1_file(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370.tar'));
22+
var_dump(sha1_file(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370_out.tar'));
23+
?>
24+
--EXPECT--
25+
string(40) "04929d1dee68551c74f5c9809691040d16ba854b"
26+
string(40) "04929d1dee68551c74f5c9809691040d16ba854b"
27+
--CLEAN--
28+
<?php
29+
@unlink(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370_out.tar');
30+
?>

main/streams/streams.c

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1634,8 +1634,21 @@ PHPAPI zend_result _php_stream_copy_to_stream_ex(php_stream *src, php_stream *de
16341634
char *p;
16351635

16361636
do {
1637-
size_t chunk_size = (maxlen == 0 || maxlen > PHP_STREAM_MMAP_MAX) ? PHP_STREAM_MMAP_MAX : maxlen;
1638-
size_t mapped;
1637+
/* We must not modify maxlen here, because otherwise the file copy fallback below can fail */
1638+
size_t chunk_size, must_read, mapped;
1639+
if (maxlen == 0) {
1640+
/* Unlimited read */
1641+
must_read = chunk_size = PHP_STREAM_MMAP_MAX;
1642+
} else {
1643+
must_read = maxlen - haveread;
1644+
if (must_read >= PHP_STREAM_MMAP_MAX) {
1645+
chunk_size = PHP_STREAM_MMAP_MAX;
1646+
} else {
1647+
/* In case the length we still have to read from the file could be smaller than the file size,
1648+
* chunk_size must not get bigger the size we're trying to read. */
1649+
chunk_size = must_read;
1650+
}
1651+
}
16391652

16401653
p = php_stream_mmap_range(src, php_stream_tell(src), chunk_size, PHP_STREAM_MAP_MODE_SHARED_READONLY, &mapped);
16411654

@@ -1650,6 +1663,7 @@ PHPAPI zend_result _php_stream_copy_to_stream_ex(php_stream *src, php_stream *de
16501663
didwrite = php_stream_write(dest, p, mapped);
16511664
if (didwrite < 0) {
16521665
*len = haveread;
1666+
php_stream_mmap_unmap(src);
16531667
return FAILURE;
16541668
}
16551669

@@ -1666,9 +1680,10 @@ PHPAPI zend_result _php_stream_copy_to_stream_ex(php_stream *src, php_stream *de
16661680
if (mapped < chunk_size) {
16671681
return SUCCESS;
16681682
}
1683+
/* If we're not reading as much as possible, so a bounded read */
16691684
if (maxlen != 0) {
1670-
maxlen -= mapped;
1671-
if (maxlen == 0) {
1685+
must_read -= mapped;
1686+
if (must_read == 0) {
16721687
return SUCCESS;
16731688
}
16741689
}

0 commit comments

Comments
 (0)