Skip to content

Commit 9e58a1d

Browse files
lacatoirelacatoire
authored
ext/ftp: throw an Error when ftp_nb_fget()/ftp_nb_fput() hit a busy connection (#23540)
* ext/ftp: throw an Error when the connection is already transferring ftp_nb_fget() and ftp_nb_fput() answered an already busy connection with a warning and false, against a declared int return type. Reaching that guard means a transfer was started from inside another transfer, which is a programming mistake, so throw an Error instead, as ftp_close() already does on the same in_use flag. The declarations stay int. The check stays ahead of the direction and closestream writes, so the running transfer is left untouched. The test reaches the guard through a stream wrapper that calls back into the extension mid transfer. * [skip ci] Note the ftp_nb_fget()/ftp_nb_fput() Error in UPGRADING * Address review: extend the Error to ftp_nb_get() and ftp_nb_put() The in_use guard is a programming error in all four non-blocking transfer functions, so all four now throw the same Error instead of two of them emitting a warning and returning false. ftp_nb_get() and ftp_nb_put() keep their int|false declaration, which is still returned when the local file cannot be opened. ftp_nb_put() closes the local stream before throwing, and the guards stay ahead of the direction/closestream writes, so a rejected re-entrant call leaves the running transfer untouched. ftp_nb_get_during_transfer.phpt and ftp_nb_get_during_nb_transfer.phpt asserted the old warning; they now record the Error and still assert that the outer transfer completes. * Catch Throwable in the re-entrant transfer test The test asserts the class it prints, so it must not presume Error in the catch: a change of thrown class has to fail the test rather than escape it.
1 parent 8b0088a commit 9e58a1d

5 files changed

Lines changed: 101 additions & 10 deletions

File tree

‎UPGRADING‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,12 @@ PHP 8.6 UPGRADE NOTES
4848
the integer index is greater than INT_MAX instead of overflowing to a
4949
smaller index.
5050

51+
- FTP:
52+
. ftp_nb_fget(), ftp_nb_fput(), ftp_nb_get() and ftp_nb_put() now throw an
53+
Error when the connection is already transferring, instead of emitting a
54+
warning and returning false. ftp_close() already throws on the same
55+
condition.
56+
5157
- GD:
5258
. imagesetstyle(), imagefilter() and imagecrop() filter the types / values of
5359
their array arguments and raise a TypeError / ValueError accordingly.

‎ext/ftp/php_ftp.c‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -655,8 +655,8 @@ PHP_FUNCTION(ftp_nb_fget)
655655

656656
/* configuration */
657657
if (ftp->in_use) {
658-
php_error_docref(NULL, E_WARNING, "FTP\\Connection is already in use");
659-
RETURN_FALSE;
658+
zend_throw_error(NULL, "Cannot start a transfer while another transfer is in progress");
659+
RETURN_THROWS();
660660
}
661661

662662
ftp->direction = 0; /* recv */
@@ -770,8 +770,8 @@ PHP_FUNCTION(ftp_nb_get)
770770
}
771771
GET_FTPBUF(ftp, z_ftp);
772772
if (ftp->in_use) {
773-
php_error_docref(NULL, E_WARNING, "FTP\\Connection is already in use");
774-
RETURN_FALSE;
773+
zend_throw_error(NULL, "Cannot start a transfer while another transfer is in progress");
774+
RETURN_THROWS();
775775
}
776776
XTYPE(xtype, mode);
777777

@@ -960,8 +960,8 @@ PHP_FUNCTION(ftp_nb_fput)
960960

961961
/* configuration */
962962
if (ftp->in_use) {
963-
php_error_docref(NULL, E_WARNING, "FTP\\Connection is already in use");
964-
RETURN_FALSE;
963+
zend_throw_error(NULL, "Cannot start a transfer while another transfer is in progress");
964+
RETURN_THROWS();
965965
}
966966

967967
ftp->direction = true; /* send */
@@ -1106,8 +1106,8 @@ PHP_FUNCTION(ftp_nb_put)
11061106

11071107
if (ftp->in_use) {
11081108
php_stream_close(instream);
1109-
php_error_docref(NULL, E_WARNING, "FTP\\Connection is already in use");
1110-
RETURN_FALSE;
1109+
zend_throw_error(NULL, "Cannot start a transfer while another transfer is in progress");
1110+
RETURN_THROWS();
11111111
}
11121112

11131113
/* configuration */

‎ext/ftp/tests/ftp_nb_get_during_nb_transfer.phpt‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,17 @@ require 'server.inc';
1010
class NbGetDuringNbGet {
1111
public $context;
1212
public static $ftp;
13+
public static $error;
1314
public function stream_open($path, $mode, $options, &$opened_path) {
1415
return true;
1516
}
1617
public function stream_write($data) {
17-
@ftp_nb_get(self::$ftp, 'php://memory', 'a story.txt', FTP_BINARY);
18+
try {
19+
ftp_nb_get(self::$ftp, 'php://memory', 'a story.txt', FTP_BINARY);
20+
} catch (Throwable $e) {
21+
/* recorded rather than echoed: stream_write() may run more than once */
22+
self::$error = $e::class . ': ' . $e->getMessage();
23+
}
1824
return strlen($data);
1925
}
2026
public function stream_close() {}
@@ -35,10 +41,13 @@ while ($r == FTP_MOREDATA) {
3541
}
3642
var_dump($r === FTP_FINISHED);
3743

44+
var_dump(NbGetDuringNbGet::$error);
45+
3846
ftp_close($ftp);
3947
echo "closed\n";
4048
?>
4149
--EXPECT--
4250
bool(true)
4351
bool(true)
52+
string(68) "Error: Cannot start a transfer while another transfer is in progress"
4453
closed

‎ext/ftp/tests/ftp_nb_get_during_transfer.phpt‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,17 @@ require 'server.inc';
1010
class NbGetDuringGet {
1111
public $context;
1212
public static $ftp;
13+
public static $error;
1314
public function stream_open($path, $mode, $options, &$opened_path) {
1415
return true;
1516
}
1617
public function stream_write($data) {
17-
@ftp_nb_get(self::$ftp, 'php://memory', 'a story.txt', FTP_BINARY);
18+
try {
19+
ftp_nb_get(self::$ftp, 'php://memory', 'a story.txt', FTP_BINARY);
20+
} catch (Throwable $e) {
21+
/* recorded rather than echoed: stream_write() may run more than once */
22+
self::$error = $e::class . ': ' . $e->getMessage();
23+
}
1824
return strlen($data);
1925
}
2026
public function stream_close() {}
@@ -31,10 +37,13 @@ NbGetDuringGet::$ftp = $ftp;
3137

3238
var_dump(@ftp_get($ftp, 'reentrantget://sink', 'a story.txt', FTP_BINARY));
3339

40+
var_dump(NbGetDuringGet::$error);
41+
3442
ftp_close($ftp);
3543
echo "closed\n";
3644
?>
3745
--EXPECT--
3846
bool(true)
3947
bool(true)
48+
string(68) "Error: Cannot start a transfer while another transfer is in progress"
4049
closed

‎ext/ftp/tests/ftp_nb_transfer_during_transfer.phpt‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
--TEST--
2+
ftp_nb_fget(), ftp_nb_fput(), ftp_nb_get() and ftp_nb_put() throw when a transfer is already in progress
3+
--EXTENSIONS--
4+
ftp
5+
pcntl
6+
--FILE--
7+
<?php
8+
require 'server.inc';
9+
10+
class TransferDuringNbWrite {
11+
public $context;
12+
public static $ftp;
13+
public static $call;
14+
public function stream_open($path, $mode, $options, &$opened_path) {
15+
return true;
16+
}
17+
public function stream_write($data) {
18+
try {
19+
(self::$call)(self::$ftp);
20+
} catch (Throwable $e) {
21+
echo $e::class, ': ', $e->getMessage(), "\n";
22+
}
23+
return strlen($data);
24+
}
25+
public function stream_close() {}
26+
public function stream_eof() {
27+
return true;
28+
}
29+
}
30+
31+
stream_wrapper_register('reentrantnb', TransferDuringNbWrite::class);
32+
33+
$ftp = ftp_connect('127.0.0.1', $port);
34+
var_dump(ftp_login($ftp, 'user', 'pass'));
35+
TransferDuringNbWrite::$ftp = $ftp;
36+
37+
$sink = fopen('php://memory', 'w+');
38+
/* ftp_nb_put() opens the local file before it reaches the guard, so it has to exist. */
39+
$local = __DIR__ . '/ftp_nb_transfer_during_transfer.tmp';
40+
file_put_contents($local, 'payload');
41+
42+
$calls = [
43+
static fn ($ftp) => ftp_nb_fget($ftp, $sink, 'a story.txt', FTP_BINARY),
44+
static fn ($ftp) => ftp_nb_fput($ftp, 'a story.txt', $sink, FTP_BINARY),
45+
static fn ($ftp) => ftp_nb_get($ftp, $local, 'a story.txt', FTP_BINARY),
46+
static fn ($ftp) => ftp_nb_put($ftp, 'a story.txt', $local, FTP_BINARY),
47+
];
48+
49+
foreach ($calls as $call) {
50+
TransferDuringNbWrite::$call = $call;
51+
@ftp_nb_get($ftp, 'reentrantnb://sink', 'a story.txt', FTP_BINARY);
52+
}
53+
54+
ftp_close($ftp);
55+
echo "closed\n";
56+
?>
57+
--CLEAN--
58+
<?php
59+
@unlink(__DIR__ . '/ftp_nb_transfer_during_transfer.tmp');
60+
?>
61+
--EXPECT--
62+
bool(true)
63+
Error: Cannot start a transfer while another transfer is in progress
64+
Error: Cannot start a transfer while another transfer is in progress
65+
Error: Cannot start a transfer while another transfer is in progress
66+
Error: Cannot start a transfer while another transfer is in progress
67+
closed

0 commit comments

Comments
 (0)