Close of build 42.1.
http://bugs.proftpd.org/show_bug.cgi?id=4014
I addition, this patch backports the pr_str_get_nbytes() function.
diff --git a/include/options.h b/include/options.h
index 96fa35d..0b29bf7 100644
--- a/include/options.h
+++ b/include/options.h
@@ -110,6 +110,13 @@
# define PR_TUNABLE_XFER_BUFFER_SIZE PR_TUNABLE_BUFFER_SIZE
#endif
+/* Maximum FTP command size. For details on this size of 512KB, see
+ * the Bug#4014 discussion.
+ */
+#ifndef PR_TUNABLE_CMD_BUFFER_SIZE
+# define PR_TUNABLE_CMD_BUFFER_SIZE (512 * 1024)
+#endif
+
/* Maximum path length. GNU HURD (and some others) do not define
* MAXPATHLEN. POSIX' PATH_MAX is mandated to be at least 256
* (according to some), so 1K, in the absense of MAXPATHLEN, should be
diff --git a/include/str.h b/include/str.h
index b4fed7c..1cf8724 100644
--- a/include/str.h
+++ b/include/str.h
@@ -39,6 +39,7 @@ char *pstrndup(pool *, const char *, size_t);
char *pr_str_strip(pool *, char *);
char *pr_str_strip_end(char *, char *);
+int pr_str_get_nbytes(const char *, const char *, off_t *);
char *pr_str_get_word(char **, int);
#define PR_STR_FL_PRESERVE_COMMENTS 0x0001
diff --git a/modules/mod_core.c b/modules/mod_core.c
index 18a47c2..922f4d1 100644
--- a/modules/mod_core.c
+++ b/modules/mod_core.c
@@ -2240,18 +2240,44 @@ MODRET set_allowforeignaddress(cmd_rec *cmd) {
}
MODRET set_commandbuffersize(cmd_rec *cmd) {
- int size = 0;
+ int res;
+ size_t size = 0;
+ off_t nbytes = 0;
config_rec *c = NULL;
+ const char *units = NULL;
+
+ if (cmd->argc < 2 || cmd->argc > 3) {
+ CONF_ERROR(cmd, "wrong number of parameters")
+ }
- CHECK_ARGS(cmd, 1);
CHECK_CONF(cmd, CONF_ROOT|CONF_VIRTUAL|CONF_GLOBAL);
- /* NOTE: need to add checks for maximum possible sizes, negative sizes. */
- size = atoi(cmd->argv[1]);
+ if (cmd->argc == 3) {
+ units = cmd->argv[2];
+ }
+
+ if (pr_str_get_nbytes(cmd->argv[1], units, &nbytes) < 0) {
+ CONF_ERROR(cmd, pstrcat(cmd->tmp_pool, "unable to parse: ",
+ cmd->argv[1], " ", units, ": ", strerror(errno), NULL));
+ }
+
+ if (nbytes > PR_TUNABLE_CMD_BUFFER_SIZE) {
+ char max[1024];
+
+ snprintf(max, sizeof(max)-1, "%lu", (unsigned long)
+ PR_TUNABLE_CMD_BUFFER_SIZE);
+ max[sizeof(max)-1] = '\0';
+
+ CONF_ERROR(cmd, pstrcat(cmd->tmp_pool, "size ", cmd->argv[1], units,
+ "exceeds max size ", max, NULL));
+ }
+
+ /* Possible truncation here, but only for an absurdly large size. */
+ size = (size_t) nbytes;
c = add_config_param(cmd->argv[0], 1, NULL);
- c->argv[0] = pcalloc(c->pool, sizeof(int));
- *((int *) c->argv[0]) = size;
+ c->argv[0] = pcalloc(c->pool, sizeof(size_t));
+ *((size_t *) c->argv[0]) = size;
return PR_HANDLED(cmd);
}
diff --git a/src/main.c b/src/main.c
index 3e6d637..660e14b 100644
--- a/src/main.c
+++ b/src/main.c
@@ -466,42 +466,21 @@ static int _dispatch(cmd_rec *cmd, int cmd_type, int validate, char *match) {
return success;
}
-/* Returns the appropriate maximum buffer length to use for FTP commands
- * from the client, taking the CommandBufferSize directive into account.
+/* Returns the appropriate maximum buffer size to use for FTP commands
+ * from the client.
*/
-static long get_max_cmd_len(size_t buflen) {
- long res;
- int *bufsz = NULL;
- size_t default_cmd_bufsz;
-
- /* It's possible for the admin to select a PR_TUNABLE_BUFFER_SIZE which
- * is smaller than PR_DEFAULT_CMD_BUFSZ. We need to handle such cases
- * properly.
- */
- default_cmd_bufsz = PR_DEFAULT_CMD_BUFSZ;
- if (default_cmd_bufsz > buflen) {
- default_cmd_bufsz = buflen;
- }
-
+static size_t get_max_cmd_sz(void) {
+ size_t res;
+ size_t *bufsz = NULL;
+
bufsz = get_param_ptr(main_server->conf, "CommandBufferSize", FALSE);
if (bufsz == NULL) {
- res = default_cmd_bufsz;
-
- } else if (*bufsz <= 0) {
- pr_log_pri(PR_LOG_WARNING, "invalid CommandBufferSize size (%d) given, "
- "using default buffer size (%lu) instead", *bufsz,
- (unsigned long) default_cmd_bufsz);
- res = default_cmd_bufsz;
-
- } else if (*bufsz + 1 > buflen) {
- pr_log_pri(PR_LOG_WARNING, "invalid CommandBufferSize size (%d) given, "
- "using default buffer size (%lu) instead", *bufsz,
- (unsigned long) default_cmd_bufsz);
- res = default_cmd_bufsz;
+ res = PR_DEFAULT_CMD_BUFSZ;
} else {
- pr_log_debug(DEBUG1, "setting CommandBufferSize to %d", *bufsz);
- res = (long) *bufsz;
+ pr_log_debug(DEBUG1, "setting CommandBufferSize to %lu",
+ (unsigned long) *bufsz);
+ res = *bufsz;
}
return res;
@@ -509,21 +488,29 @@ static long get_max_cmd_len(size_t buflen) {
int pr_cmd_read(cmd_rec **res) {
static long cmd_bufsz = -1;
- char buf[PR_DEFAULT_CMD_BUFSZ+1] = {'\0'};
+ static char *cmd_buf = NULL;
char *cp;
- size_t buflen;
+ size_t cmd_buflen;
if (res == NULL) {
errno = EINVAL;
return -1;
}
+ if (cmd_bufsz == -1) {
+ cmd_bufsz = get_max_cmd_sz();
+ }
+
+ if (cmd_buf == NULL) {
+ cmd_buf = pcalloc(session.pool, cmd_bufsz + 1);
+ }
+
while (TRUE) {
pr_signals_handle();
- memset(buf, '\0', sizeof(buf));
+ memset(cmd_buf, '\0', cmd_bufsz);
- if (pr_netio_telnet_gets(buf, sizeof(buf)-1, session.c->instrm,
+ if (pr_netio_telnet_gets(cmd_buf, cmd_bufsz, session.c->instrm,
session.c->outstrm) == NULL) {
if (errno == E2BIG) {
@@ -544,9 +531,6 @@ int pr_cmd_read(cmd_rec **res) {
break;
}
- if (cmd_bufsz == -1)
- cmd_bufsz = get_max_cmd_len(sizeof(buf));
-
/* This strlen(3) is guaranteed to terminate; the last byte of buf is
* always NUL, since pr_netio_telnet_gets() is told that the buf size is
* one byte less than it really is.
@@ -554,26 +538,28 @@ int pr_cmd_read(cmd_rec **res) {
* If the strlen(3) says that the length is less than the cmd_bufsz, then
* there is no need to truncate the buffer by inserting a NUL.
*/
- buflen = strlen(buf);
- if (buflen > (cmd_bufsz - 1)) {
+ cmd_buflen = strlen(cmd_buf);
+ if (cmd_buflen > cmd_bufsz) {
pr_log_debug(DEBUG0, "truncating incoming command length (%lu bytes) to "
"CommandBufferSize %lu; use the CommandBufferSize directive to increase "
- "the allowed command length", (unsigned long) buflen,
+ "the allowed command length", (unsigned long) cmd_buflen,
(unsigned long) cmd_bufsz);
- buf[cmd_bufsz - 1] = '\0';
+ cmd_buf[cmd_bufsz-1] = '\0';
}
- if (buflen &&
- (buf[buflen-1] == '\n' || buf[buflen-1] == '\r')) {
- buf[buflen-1] = '\0';
- buflen--;
+ if (cmd_buflen &&
+ (cmd_buf[cmd_buflen-1] == '\n' || cmd_buf[cmd_buflen-1] == '\r')) {
+ cmd_buf[cmd_buflen-1] = '\0';
+ cmd_buflen--;
- if (buflen &&
- (buf[buflen-1] == '\n' || buf[buflen-1] =='\r'))
- buf[buflen-1] = '\0';
+ if (cmd_buflen &&
+ (cmd_buf[cmd_buflen-1] == '\n' || cmd_buf[cmd_buflen-1] =='\r')) {
+ cmd_buf[cmd_buflen-1] = '\0';
+ cmd_buflen--;
+ }
}
- cp = buf;
+ cp = cmd_buf;
if (*cp == '\r')
cp++;
@@ -587,11 +573,11 @@ int pr_cmd_read(cmd_rec **res) {
* command handlers themselves, via cmd->arg. This small hack
* reduces the burden on SITE module developers, however.
*/
- if (strncasecmp(cp, C_SITE, 4) == 0)
+ if (strncasecmp(cp, C_SITE, 4) == 0) {
flags |= PR_STR_FL_PRESERVE_WHITESPACE;
+ }
cmd = make_ftp_cmd(session.pool, cp, flags);
-
if (cmd) {
*res = cmd;
}
diff --git a/src/str.c b/src/str.c
index d243a17..4f327bf 100644
--- a/src/str.c
+++ b/src/str.c
@@ -367,6 +367,81 @@ char *pr_str_strip_end(char *s, char *ch) {
return s;
}
+int pr_str_get_nbytes(const char *str, const char *units, off_t *nbytes) {
+ off_t sz;
+ char *ptr = NULL;
+ float factor = 0.0;
+
+ if (str == NULL) {
+ errno = EINVAL;
+ return -1;
+ }
+
+ /* No negative numbers. */
+ if (*str == '-') {
+ errno = EINVAL;
+ return -1;
+ }
+
+ if (units == NULL ||
+ *units == '\0') {
+ factor = 1.0;
+
+ } else if (strncasecmp(units, "KB", 3) == 0) {
+ factor = 1024.0;
+
+ } else if (strncasecmp(units, "MB", 3) == 0) {
+ factor = 1024.0 * 1024.0;
+
+ } else if (strncasecmp(units, "GB", 3) == 0) {
+ factor = 1024.0 * 1024.0 * 1024.0;
+
+ } else if (strncasecmp(units, "TB", 3) == 0) {
+ factor = 1024.0 * 1024.0 * 1024.0 * 1024.0;
+
+ } else if (strncasecmp(units, "B", 2) == 0) {
+ factor = 1.0;
+
+ } else {
+ errno = EINVAL;
+ return -1;
+ }
+
+ errno = 0;
+
+#ifdef HAVE_STRTOULL
+ sz = strtoull(str, &ptr, 10);
+#else
+ sz = strtoul(str, &ptr, 10);
+#endif /* !HAVE_STRTOULL */
+
+ if (errno == ERANGE) {
+ return -1;
+ }
+
+ if (ptr != NULL && *ptr) {
+ /* Error parsing the given string */
+ errno = EINVAL;
+ return -1;
+ }
+
+ /* Don't bother applying the factor if the result will overflow the result. */
+#ifdef ULLONG_MAX
+ if (sz > (ULLONG_MAX / factor)) {
+#else
+ if (sz > (ULONG_MAX / factor)) {
+#endif /* !ULLONG_MAX */
+ errno = ERANGE;
+ return -1;
+ }
+
+ if (nbytes != NULL) {
+ *nbytes = (off_t) (sz * factor);
+ }
+
+ return 0;
+}
+
char *pr_str_get_word(char **cp, int flags) {
char *res, *dst;
char quote_mode = 0;
diff --git a/tests/t/lib/ProFTPD/Tests/Config/CommandBufferSize.pm b/tests/t/lib/ProFTPD/Tests/Config/CommandBufferSize.pm
index ed4672a..a57c898 100644
--- a/tests/t/lib/ProFTPD/Tests/Config/CommandBufferSize.pm
+++ b/tests/t/lib/ProFTPD/Tests/Config/CommandBufferSize.pm
@@ -94,6 +94,8 @@ sub cmdbuffersz_small {
die("Can't open $test_path: $!");
}
+ my $idle_timeout = 3;
+
my $config = {
PidFile => $pid_file,
ScoreboardFile => $scoreboard_file,
@@ -103,6 +105,7 @@ sub cmdbuffersz_small {
AuthGroupFile => $auth_group_file,
CommandBufferSize => $cmdbufsz,
+ TimeoutIdle => $idle_timeout,
IfModules => {
'mod_delay.c' => {
@@ -128,44 +131,16 @@ sub cmdbuffersz_small {
defined(my $pid = fork()) or die("Can't fork: $!");
if ($pid) {
eval {
- my $client = ProFTPD::TestSuite::FTP->new('127.0.0.1', $port);
+ my $client = ProFTPD::TestSuite::FTP->new('127.0.0.1', $port, 0, 1);
$client->login($user, $passwd);
- my $conn = $client->list_raw($test_file);
- unless ($conn) {
- die("Failed to LIST $test_file: " . $client->response_code() . " " .
- $client->response_msg());
+ # Since our filename is longer than the CommandBufferSize, proftpd
+ # should simply ignore this. It will fail because of the idle timeout
+ # first.
+ eval { $client->stat($test_file) };
+ unless ($@) {
+ die("STAT command succeeded unexpectedly");
}
-
- my $buf;
- $conn->read($buf, 8192, 30);
- eval { $conn->close() };
-
- my $resp_code = $client->response_code();
- my $resp_msg = $client->response_msg();
-
- # CommandBufferSize works by truncating any input longer than the
- # configured length. (It should arguably reject such longer input,
- # but that is a different consideration.)
- #
- # Since our file name is longer than the CommandBufferSize, it means
- # the path will be truncated, and the LIST should return 450.
-
- my $expected;
-
- $expected = 450;
- $self->assert($expected == $resp_code,
- test_msg("Expected $expected, got $resp_code"));
-
- # This length is CommandBufferSize - "LIST"(4) - " "(1) - 1 for
- # the NUL reserved in the code. Thus CommandBufferSize - 6.
- my $truncated_name = ("A" x ($cmdbufsz - 6));
-
- $expected = "$truncated_name: No such file or directory";
- $self->assert($expected eq $resp_msg,
- test_msg("Expected '$expected', got '$resp_msg'"));
-
- $client->quit();
};
if ($@) {
@@ -176,7 +151,7 @@ sub cmdbuffersz_small {
$wfh->flush();
} else {
- eval { server_wait($config_file, $rfh) };
+ eval { server_wait($config_file, $rfh, 15) };
if ($@) {
warn($@);
exit 1;