Skip to content

Commit 54b0213

Browse files
nielsdosnielsdos
committed
Repeat copy_file_range in case of partial copies
copy_file_range can return early without copying all the data. This is not necessarily an error, the copy could've just been interrupted. In that case it makes sense to keep using the file_copy_range syscall to have the best performance. Since the copy is now repeated, I had to make some changes to our file_copy_range interposer such that we can limit the amount of times it is called. I also added two variants such that both the fallback and repeated copy are tested now.
1 parent dc71fdf commit 54b0213

File tree

9 files changed

+142
-52
lines changed

9 files changed

+142
-52
lines changed

‎ext/zend_test/php_test.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,9 @@ 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;
56+
zend_long limit_copy_file_range_length;
57+
zend_long limit_copy_file_range_times;
58+
zend_long amount_of_times_called_copy_file_range;
5759
zend_test_fiber *active_fiber;
5860
zend_long quantity_value;
5961
zend_string *str_test;

‎ext/zend_test/test.c

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -722,7 +722,8 @@ PHP_INI_BEGIN()
722722
STD_PHP_INI_BOOLEAN("zend_test.register_passes", "0", PHP_INI_SYSTEM, OnUpdateBool, register_passes, zend_zend_test_globals, zend_test_globals)
723723
STD_PHP_INI_BOOLEAN("zend_test.print_stderr_mshutdown", "0", PHP_INI_SYSTEM, OnUpdateBool, print_stderr_mshutdown, zend_zend_test_globals, zend_test_globals)
724724
#ifdef HAVE_COPY_FILE_RANGE
725-
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)
725+
STD_PHP_INI_ENTRY("zend_test.limit_copy_file_range_length", "-1", PHP_INI_ALL, OnUpdateLong, limit_copy_file_range_length, zend_zend_test_globals, zend_test_globals)
726+
STD_PHP_INI_ENTRY("zend_test.limit_copy_file_range_times", "-1", PHP_INI_ALL, OnUpdateLong, limit_copy_file_range_times, zend_zend_test_globals, zend_test_globals)
726727
#endif
727728
STD_PHP_INI_ENTRY("zend_test.quantity_value", "0", PHP_INI_ALL, OnUpdateLong, quantity_value, zend_zend_test_globals, zend_test_globals)
728729
STD_PHP_INI_ENTRY("zend_test.str_test", "", PHP_INI_ALL, OnUpdateStr, str_test, zend_zend_test_globals, zend_test_globals)
@@ -962,9 +963,15 @@ PHP_ZEND_TEST_API void bug_gh9090_void_int_char_var(int i, char *fmt, ...) {
962963
*/
963964
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)
964965
{
966+
if (ZT_G(limit_copy_file_range_times) >= Z_L(0)) {
967+
if (++ZT_G(amount_of_times_called_copy_file_range) > ZT_G(limit_copy_file_range_times)) {
968+
errno = EIO;
969+
return -1;
970+
}
971+
}
965972
ssize_t (*original_copy_file_range)(int, off64_t *, int, off64_t *, size_t, unsigned int) = dlsym(RTLD_NEXT, "copy_file_range");
966-
if (ZT_G(limit_copy_file_range) >= Z_L(0)) {
967-
len = ZT_G(limit_copy_file_range);
973+
if (ZT_G(limit_copy_file_range_length) >= Z_L(0)) {
974+
len = MIN(len, ZT_G(limit_copy_file_range_length));
968975
}
969976
return original_copy_file_range(fd_in, off_in, fd_out, off_out, len, flags);
970977
}

‎ext/zend_test/tests/gh10370_1.phpt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ if (PHP_OS != 'Linux') {
1010
}
1111
?>
1212
--INI--
13-
zend_test.limit_copy_file_range=3584
13+
zend_test.limit_copy_file_range_length=3584
1414
--FILE--
1515
<?php
1616
/* Note: the value 3584 is chosen so that the mmap in _php_stream_copy_to_stream_ex() will mmap

‎ext/zend_test/tests/gh10370_2.phpt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ if (PHP_OS != 'Linux') {
99
}
1010
?>
1111
--INI--
12-
zend_test.limit_copy_file_range=4096
12+
zend_test.limit_copy_file_range_length=4096
1313
--FILE--
1414
<?php
1515
/* Note: the value 4096 is chosen so that the mmap in _php_stream_copy_to_stream_ex() will mmap

‎ext/zend_test/tests/gh10370_3.phpt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ if (PHP_OS != 'Linux') {
99
}
1010
?>
1111
--INI--
12-
zend_test.limit_copy_file_range=3584
12+
zend_test.limit_copy_file_range_length=3584
1313
--FILE--
1414
<?php
1515
/* Note: the value 3584 is chosen so that the mmap in _php_stream_copy_to_stream_ex() will mmap

‎ext/zend_test/tests/gh10370_4.phpt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ if (PHP_OS != 'Linux') {
99
}
1010
?>
1111
--INI--
12-
zend_test.limit_copy_file_range=4096
12+
zend_test.limit_copy_file_range_length=4096
1313
--FILE--
1414
<?php
1515
/* Note: the value 4096 is chosen so that the mmap in _php_stream_copy_to_stream_ex() will mmap

‎ext/zend_test/tests/gh10370_5.phpt

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
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, limit times
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_length=3584
13+
zend_test.limit_copy_file_range_times=2
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+
mkdir(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370_005');
19+
20+
$input = fopen(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370.tar', 'r');
21+
$output = fopen(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370_005' . DIRECTORY_SEPARATOR . 'testfile', 'w');
22+
23+
var_dump(stream_copy_to_stream($input, $output, 10240, 0x200));
24+
25+
fclose($input);
26+
fclose($output);
27+
28+
var_dump(sha1_file(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370_005' . DIRECTORY_SEPARATOR . 'testfile'));
29+
?>
30+
--EXPECT--
31+
int(10240)
32+
string(40) "a723ae4ec7eababff73ca961a771b794be6388d2"
33+
--CLEAN--
34+
<?php
35+
@unlink(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370_005' . DIRECTORY_SEPARATOR . 'testfile');
36+
@rmdir(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370_005');
37+
?>

‎ext/zend_test/tests/gh10370_6.phpt

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
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, limit times
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_length=4096
13+
zend_test.limit_copy_file_range_times=2
14+
--FILE--
15+
<?php
16+
/* Note: the value 4096 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+
19+
$input = fopen(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370.tar', 'r');
20+
$output = fopen(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370_006_out.tar', 'w');
21+
22+
var_dump(stream_copy_to_stream($input, $output));
23+
24+
fclose($input);
25+
fclose($output);
26+
27+
var_dump(sha1_file(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370.tar'));
28+
var_dump(sha1_file(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370_006_out.tar'));
29+
?>
30+
--EXPECT--
31+
int(11776)
32+
string(40) "edcad8cd6c276f5e318c826ad77a5604d6a6e93d"
33+
string(40) "edcad8cd6c276f5e318c826ad77a5604d6a6e93d"
34+
--CLEAN--
35+
<?php
36+
@unlink(__DIR__ . DIRECTORY_SEPARATOR . 'gh10370_006_out.tar');
37+
?>

‎main/streams/streams.c

Lines changed: 51 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -1571,63 +1571,70 @@ PHPAPI zend_result _php_stream_copy_to_stream_ex(php_stream *src, php_stream *de
15711571
/* get dest open flags to check if the stream is open in append mode */
15721572
php_stream_parse_fopen_modes(dest->mode, &dest_open_flags) == SUCCESS &&
15731573
!(dest_open_flags & O_APPEND)) {
1574-
1575-
/* clamp to INT_MAX to avoid EOVERFLOW */
1576-
const size_t cfr_max = MIN(maxlen, (size_t)SSIZE_MAX);
1577-
15781574
/* copy_file_range() is a Linux-specific system call which allows efficient copying
15791575
* between two file descriptors, eliminating the need to transfer data from the kernel
15801576
* to userspace and back. For networking file systems like NFS and Ceph, it even
15811577
* eliminates copying data to the client, and local filesystems like Btrfs and XFS can
15821578
* create shared extents. */
1583-
ssize_t result = copy_file_range(src_fd, NULL, dest_fd, NULL, cfr_max, 0);
1584-
if (result > 0) {
1585-
size_t nbytes = (size_t)result;
1586-
haveread += nbytes;
1579+
ssize_t result;
1580+
do {
1581+
/* clamp to INT_MAX to avoid EOVERFLOW */
1582+
const size_t cfr_max = MIN(maxlen - haveread, (size_t)SSIZE_MAX);
15871583

1588-
src->position += nbytes;
1589-
dest->position += nbytes;
1584+
result = copy_file_range(src_fd, NULL, dest_fd, NULL, cfr_max, 0);
15901585

1591-
if ((maxlen != PHP_STREAM_COPY_ALL && nbytes == maxlen) || php_stream_eof(src)) {
1592-
/* the whole request was satisfied or end-of-file reached - done */
1586+
if (result > 0) {
1587+
size_t nbytes = (size_t)result;
1588+
haveread += nbytes;
1589+
1590+
src->position += nbytes;
1591+
dest->position += nbytes;
1592+
1593+
if ((maxlen != PHP_STREAM_COPY_ALL && haveread == maxlen) || php_stream_eof(src)) {
1594+
/* the whole request was satisfied or end-of-file reached - done */
1595+
*len = haveread;
1596+
return SUCCESS;
1597+
}
1598+
1599+
/* copy_file_range may return early without copying all data.
1600+
* There may be more data to copy; continue copying using file_copy_range, and
1601+
* if a failure would occur, it will fall back to the code below. */
1602+
} else if (result == 0) {
1603+
/* end of file */
15931604
*len = haveread;
15941605
return SUCCESS;
15951606
}
1607+
} while (result >= 0);
15961608

1597-
/* there may be more data; continue copying using the fallback code below */
1598-
} else if (result == 0) {
1599-
/* end of file */
1600-
*len = haveread;
1601-
return SUCCESS;
1602-
} else if (result < 0) {
1603-
switch (errno) {
1604-
case EINVAL:
1605-
/* some formal error, e.g. overlapping file ranges */
1606-
break;
1607-
1608-
case EXDEV:
1609-
/* pre Linux 5.3 error */
1610-
break;
1611-
1612-
case ENOSYS:
1613-
/* not implemented by this Linux kernel */
1614-
break;
1615-
1616-
case EIO:
1617-
/* Some filesystems will cause failures if the max length is greater than the file length
1618-
* in certain circumstances and configuration. In those cases the errno is EIO and we will
1619-
* fall back to other methods. We cannot use stat to determine the file length upfront because
1620-
* that is prone to races and outdated caching. */
1621-
break;
1622-
1623-
default:
1624-
/* unexpected I/O error - give up, no fallback */
1625-
*len = haveread;
1626-
return FAILURE;
1627-
}
1609+
ZEND_ASSERT(result < 0);
16281610

1629-
/* fall back to classic copying */
1611+
switch (errno) {
1612+
case EINVAL:
1613+
/* some formal error, e.g. overlapping file ranges */
1614+
break;
1615+
1616+
case EXDEV:
1617+
/* pre Linux 5.3 error */
1618+
break;
1619+
1620+
case ENOSYS:
1621+
/* not implemented by this Linux kernel */
1622+
break;
1623+
1624+
case EIO:
1625+
/* Some filesystems will cause failures if the max length is greater than the file length
1626+
* in certain circumstances and configuration. In those cases the errno is EIO and we will
1627+
* fall back to other methods. We cannot use stat to determine the file length upfront because
1628+
* that is prone to races and outdated caching. */
1629+
break;
1630+
1631+
default:
1632+
/* unexpected I/O error - give up, no fallback */
1633+
*len = haveread;
1634+
return FAILURE;
16301635
}
1636+
1637+
/* fall back to classic copying */
16311638
}
16321639
}
16331640
#endif // HAVE_COPY_FILE_RANGE

0 commit comments

Comments
 (0)