Skip to content

Commit b4db690

Browse files
ndosschendossche
authored
Fix GH-10370: File corruption in _php_stream_copy_to_stream_ex when using copy_file_range (#10440)
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 81aedad commit b4db690

File tree

8 files changed

+170
-4
lines changed

8 files changed

+170
-4
lines changed

‎ext/zend_test/php_test.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ ZEND_BEGIN_MODULE_GLOBALS(zend_test)
5353
int replace_zend_execute_ex;
5454
int register_passes;
5555
bool print_stderr_mshutdown;
56+
zend_long limit_copy_file_range;
5657
zend_test_fiber *active_fiber;
5758
zend_long quantity_value;
5859
zend_string *str_test;

‎ext/zend_test/test.c‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -650,6 +650,9 @@ PHP_INI_BEGIN()
650650
STD_PHP_INI_BOOLEAN("zend_test.replace_zend_execute_ex", "0", PHP_INI_SYSTEM, OnUpdateBool, replace_zend_execute_ex, zend_zend_test_globals, zend_test_globals)
651651
STD_PHP_INI_BOOLEAN("zend_test.register_passes", "0", PHP_INI_SYSTEM, OnUpdateBool, register_passes, zend_zend_test_globals, zend_test_globals)
652652
STD_PHP_INI_BOOLEAN("zend_test.print_stderr_mshutdown", "0", PHP_INI_SYSTEM, OnUpdateBool, print_stderr_mshutdown, zend_zend_test_globals, zend_test_globals)
653+
#ifdef HAVE_COPY_FILE_RANGE
654+
STD_PHP_INI_ENTRY("zend_test.limit_copy_file_range", "-1", PHP_INI_ALL, OnUpdateLong, limit_copy_file_range, zend_zend_test_globals, zend_test_globals)
655+
#endif
653656
STD_PHP_INI_ENTRY("zend_test.quantity_value", "0", PHP_INI_ALL, OnUpdateLong, quantity_value, zend_zend_test_globals, zend_test_globals)
654657
STD_PHP_INI_ENTRY("zend_test.str_test", "", PHP_INI_ALL, OnUpdateStr, str_test, zend_zend_test_globals, zend_test_globals)
655658
STD_PHP_INI_ENTRY("zend_test.not_empty_str_test", "val", PHP_INI_ALL, OnUpdateStrNotEmpty, not_empty_str_test, zend_zend_test_globals, zend_test_globals)
@@ -930,3 +933,17 @@ PHP_ZEND_TEST_API void bug_gh9090_void_int_char_var(int i, char *fmt, ...) {
930933

931934
va_end(args);
932935
}
936+
937+
#ifdef HAVE_COPY_FILE_RANGE
938+
/**
939+
* This function allows us to simulate early return of copy_file_range by setting the limit_copy_file_range ini setting.
940+
*/
941+
PHP_ZEND_TEST_API ssize_t copy_file_range(int fd_in, off64_t *off_in, int fd_out, off64_t *off_out, size_t len, unsigned int flags)
942+
{
943+
ssize_t (*original_copy_file_range)(int, off64_t *, int, off64_t *, size_t, unsigned int) = dlsym(RTLD_NEXT, "copy_file_range");
944+
if (ZT_G(limit_copy_file_range) >= Z_L(0)) {
945+
len = ZT_G(limit_copy_file_range);
946+
}
947+
return original_copy_file_range(fd_in, off_in, fd_out, off_out, len, flags);
948+
}
949+
#endif

‎ext/zend_test/tests/gh10370.tar‎

11.5 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) "a723ae4ec7eababff73ca961a771b794be6388d2"
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) "edcad8cd6c276f5e318c826ad77a5604d6a6e93d"
26+
string(40) "edcad8cd6c276f5e318c826ad77a5604d6a6e93d"
27+
--CLEAN--
28+
<?php
29+
@unlink(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370_out.tar');
30+
?>

‎ext/zend_test/tests/gh10370_3.phpt‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
--TEST--
2+
GH-10370: File corruption in _php_stream_copy_to_stream_ex when using copy_file_range - partial copy using stream_copy_to_stream
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=3584
13+
--FILE--
14+
<?php
15+
/* Note: the value 3584 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+
mkdir(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370');
18+
19+
$input = fopen(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370.tar', 'r');
20+
$output = fopen(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370' . DIRECTORY_SEPARATOR . 'testfile', 'w');
21+
22+
var_dump(stream_copy_to_stream($input, $output, 10240, 0x200));
23+
24+
fclose($input);
25+
fclose($output);
26+
27+
var_dump(sha1_file(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370' . DIRECTORY_SEPARATOR . 'testfile'));
28+
?>
29+
--EXPECT--
30+
int(10240)
31+
string(40) "a723ae4ec7eababff73ca961a771b794be6388d2"
32+
--CLEAN--
33+
<?php
34+
@unlink(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370' . DIRECTORY_SEPARATOR . 'testfile');
35+
@rmdir(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370');
36+
?>

‎ext/zend_test/tests/gh10370_4.phpt‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
--TEST--
2+
GH-10370: File corruption in _php_stream_copy_to_stream_ex when using copy_file_range - unlimited copy using stream_copy_to_stream
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+
18+
mkdir(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370');
19+
20+
$input = fopen(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370.tar', 'r');
21+
$output = fopen(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370_out.tar', 'w');
22+
23+
var_dump(stream_copy_to_stream($input, $output));
24+
25+
fclose($input);
26+
fclose($output);
27+
28+
var_dump(sha1_file(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370.tar'));
29+
var_dump(sha1_file(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370_out.tar'));
30+
?>
31+
--EXPECT--
32+
int(11776)
33+
string(40) "edcad8cd6c276f5e318c826ad77a5604d6a6e93d"
34+
string(40) "edcad8cd6c276f5e318c826ad77a5604d6a6e93d"
35+
--CLEAN--
36+
<?php
37+
@unlink(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370_out.tar');
38+
?>

‎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)