Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions ext/ftp/tests/server.inc
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,10 @@ if ($pid) {
fclose($fs);


}elseif (preg_match('/^PASV/', $buf, $matches) && isset($pasv_reply_at_buffer_end)) {
// The address ends at the 510th byte and the port is cut off.
fputs($s, "227 " . str_repeat("x", 496) . "127,0,0,1,4,210)\r\n");

}elseif (preg_match('/^PASV/', $buf, $matches)) {
$pasv=true;
$host = "127.0.0.1";
Expand Down Expand Up @@ -537,6 +541,13 @@ if ($pid) {
fclose($fs);
}elseif (preg_match('/^SIZE \/bug73457/', $buf)) {
fputs($s, "213 10\r\n");
}elseif (preg_match('/^SIZE \/pasvbufferend/', $buf)) {
fputs($s, "213 10\r\n");
}elseif (preg_match('/^SIZE \/mdtmnodigits/', $buf)) {
// A longer line with digits, read into the same buffer before MDTM.
fputs($s, "213 10 bytes (19991005213102)\r\n");
}elseif (preg_match('/^MDTM \/mdtmnodigits/', $buf)) {
fputs($s, "213 unknown\r\n");
}elseif (preg_match("/^SITE/", $buf)) {
fputs($s, "500 Syntax error, command unrecognized.\r\n");
}else {
Expand Down
28 changes: 20 additions & 8 deletions ext/standard/ftp_fopen_wrapper.c
Original file line number Diff line number Diff line change
Expand Up @@ -74,13 +74,23 @@ typedef struct _php_ftp_dirstream_data {
} php_ftp_dirstream_data;

/* {{{ get_ftp_result */
/* Reads reply lines into buffer until a complete status line ("ddd ") and
* returns its code, or 0 if the stream ends before one.
* On return the buffer always holds a NUL-terminated string, which the
* callers rely on when they parse or print the reply text: it is emptied
* first in case nothing can be read, and php_stream_gets() terminates every
* line it reads. */
static inline int get_ftp_result(php_stream *stream, char *buffer, size_t buffer_size)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good if there's a doc block comment stating that the buffer is always NUL terminated, as that's what the calling code depends on for safety.
The reason is that either the read fails and then the 1st line puts a NUL; or it doesn't fail and php_stream_gets will put a NUL.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: added a comment above get_ftp_result() stating the buffer is always NUL-terminated on return and why (folded into the same commit). Thanks for the review!

{
buffer[0] = '\0'; /* in case read fails to read anything */
while (php_stream_gets(stream, buffer, buffer_size-1) &&
!(isdigit((unsigned char)buffer[0]) && isdigit((unsigned char)buffer[1]) &&
isdigit((unsigned char)buffer[2]) && buffer[3] == ' '));
return strtol(buffer, NULL, 10);
while (php_stream_gets(stream, buffer, buffer_size-1)) {
if (isdigit((unsigned char)buffer[0]) && isdigit((unsigned char)buffer[1]) &&
isdigit((unsigned char)buffer[2]) && buffer[3] == ' ') {
return strtol(buffer, NULL, 10);
}
}
/* no complete status line, callers parse the reply from buffer + 4 */
return 0;
}
/* }}} */

Expand Down Expand Up @@ -352,8 +362,10 @@ static unsigned short php_fopen_do_pasv(php_stream *stream, char *ip, size_t ip_
tpath++;
}
tpath[-1] = '\0';
memcpy(ip, hoststart, ip_size);
ip[ip_size-1] = '\0';
if ((size_t)(tpath - hoststart) > ip_size) {
return 0;
}
memcpy(ip, hoststart, tpath - hoststart);
hoststart = ip;

/* pull out the MSB of the port */
Expand Down Expand Up @@ -825,11 +837,11 @@ static int php_stream_ftp_url_stat(php_stream_wrapper *wrapper, const char *url,
struct tm tm, tmbuf, *gmt;
time_t stamp;

while ((size_t)(p - tmp_line) < sizeof(tmp_line) && !isdigit((unsigned char)*p)) {
while (*p && !isdigit((unsigned char)*p)) {
p++;
}

if ((size_t)(p - tmp_line) > sizeof(tmp_line)) {
if (!*p) {
goto mdtm_error;
}

Expand Down
20 changes: 20 additions & 0 deletions ext/standard/tests/streams/ftp_fopen_do_pasv_reply_bounds.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
--TEST--
ftp:// wrapper rejects a PASV reply whose address ends at the reply buffer limit
--SKIPIF--
<?php
if (array_search('ftp',stream_get_wrappers()) === FALSE) die("skip ftp wrapper not available.");
if (!function_exists('pcntl_fork')) die("skip pcntl_fork() not available.");
?>
--FILE--
<?php

$pasv_reply_at_buffer_end = true;
require __DIR__ . "/../../../ftp/tests/server.inc";

$path = "ftp://localhost:" . $port . "/pasvbufferend";

var_dump(fopen($path, "r"));
?>
--EXPECTF--
Warning: fopen(%s): Failed to open stream: FTP server reports %a in %s on line %d
bool(false)
20 changes: 20 additions & 0 deletions ext/standard/tests/streams/ftp_url_stat_mdtm_no_digits.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
--TEST--
stat() on ftp:// ignores an MDTM reply without digits
--SKIPIF--
<?php
if (array_search('ftp',stream_get_wrappers()) === FALSE) die("skip ftp wrapper not available.");
if (!function_exists('pcntl_fork')) die("skip pcntl_fork() not available.");
?>
--FILE--
<?php

require __DIR__ . "/../../../ftp/tests/server.inc";

$path = "ftp://localhost:" . $port . "/mdtmnodigits";

$stat = stat($path);
var_dump($stat['size'], $stat['mtime']);
?>
--EXPECT--
int(10)
int(-1)
Loading