Skip to content

Commit

Permalink
Fix GH-10562: Memory leak and invalid state with consecutive ftp_nb_fget
Browse files Browse the repository at this point in the history
When the user does not fully consume the data stream, but instead opens
a new one, a memory leak occurs. Moreover, the state is invalid: when
more commands arrive they'll be handled out-of-sync because the state of
the client does not match what the server is doing.
This leads to all sorts of weirdness, for example:
  Warning: ftp_nb_fget(): OK.

Fix it by gracefully closing the old data stream when a new data stream
is started.

Closes GH-11606.
  • Loading branch information
nielsdos committed Jul 7, 2023
1 parent 47d4788 commit c962a96
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 0 deletions.
2 changes: 2 additions & 0 deletions NEWS
Expand Up @@ -21,6 +21,8 @@ PHP NEWS

- FTP:
. Fix context option check for "overwrite". (JonasQuinten)
. Fixed bug GH-10562 (Memory leak and invalid state with consecutive
ftp_nb_fget). (nielsdos)

- GD:
. Fix most of the external libgd test failures. (Michael Orlitzky)
Expand Down
9 changes: 9 additions & 0 deletions ext/ftp/ftp.c
Expand Up @@ -2063,6 +2063,15 @@ ftp_nb_get(ftpbuf_t *ftp, php_stream *outstream, const char *path, const size_t
return PHP_FTP_FAILED;
}

if (ftp->data != NULL) {
/* If there is a transfer in action, abort it.
* If we don't, we get an invalid state and memory leaks when the new connection gets opened. */
data_close(ftp, ftp->data);
if (!ftp_getresp(ftp) || (ftp->resp != 226 && ftp->resp != 250)) {
goto bail;
}
}

if (!ftp_type(ftp, type)) {
goto bail;
}
Expand Down
40 changes: 40 additions & 0 deletions ext/ftp/tests/gh10562.phpt
@@ -0,0 +1,40 @@
--TEST--
GH-10562 (Memory leak with consecutive ftp_nb_fget)
--EXTENSIONS--
ftp
pcntl
--FILE--
<?php
require 'server.inc';

$ftp = ftp_connect('127.0.0.1', $port);
if (!$ftp) die("Couldn't connect to the server");

var_dump(ftp_login($ftp, 'anonymous', 'IEUser@'));

$local_file = __DIR__ . DIRECTORY_SEPARATOR . "gh10562.txt";
$fout = fopen($local_file, "w");

// This requests more data, but we don't do the loop to fetch it.
$ret = ftp_nb_fget($ftp, $fout, "fget", FTP_BINARY, 0);
var_dump($ret == FTP_MOREDATA);

// This aborts the previous request, fetches the whole "a story" file.
$ret = ftp_nb_fget($ftp, $fout, "a story", FTP_BINARY, 0);
while ($ret == FTP_MOREDATA) {
$ret = ftp_nb_continue($ftp);
}

fclose($fout);

echo file_get_contents($local_file), "\n";
?>
--CLEAN--
<?php
@unlink(__DIR__ . DIRECTORY_SEPARATOR . "gh10562.txt");
?>
--EXPECT--
bool(true)
bool(true)
BINARYFooBar
For sale: baby shoes, never worn.

0 comments on commit c962a96

Please sign in to comment.