> + if (clock_gettime(CLOCK_MONOTONIC_COARSE, &now) == -1) {
> + syslog(LOG_ERR, "clock_gettime (%d %s): error reading clock",
> + errno, strerror(errno));
> + return false;
> + }
I'm curious about the choice of struct timespec/clock_gettime() vs
time_t/time() -- considering we're operating at seconds (or maybe
minutes) granularity anyway?
I can kind of see a point about the ability to request a monotonic
clock, such that some sysadmin changing the system time doesn't
prematurely timeout a bunch of idle connections. Though that doesn't
seem likely to be a real problem in practice (a: don't do that, b: the
clients will just reconnect in a bit anyway).
And I'm having mixed feelings about CLOCK_MONOTONIC_COARSE, which Linux
documents as "Linux-specific". I know we have some Solaris users out
there (because they very kindly send us patches every time us
Linux-using developers break it for them!)
Also, I'm curious about the explicit %s=>strerror(errno) in the syslog
lines, instead of just doing something like: syslog(LOG_ERR,
"clock_gettime: %m"). Not sure if this is a case of you not knowing
about %m, or you knowing something I don't about %m...
> + if (deadline_exceeded(&deadline)) {
> + shutdown = shutdown_silent;
> + goto done_shutdown;
> + }
I notice this bit is explicitly skipping past terminating IDLE on the
backend? I guess we specifically didn't receive an idle termination from
the client in this case, so passing one on to the backend would be
misleading, but I wonder.
Anyway, the patch applies cleanly, builds fine for me, and passes our
current tests. I'll try and put together a couple of Cassandane tests
for the new behaviour and see what shakes out of that.
Cheers,
elli
On Wed, Sep 7, 2016, at 06:54 PM, Philipp Gesang wrote:
> Add a new configuration option “imapidletimeout” which, if
> greater than zero, specifies an upper limit in seconds for idle
> connections. The value defaults to zero (not set).
>
> RFC 2177 recommends that a client re-issue the IDLE command at
> least every 29 minutes if it wishes to continue, otherwise the
> server is free to treat the client as disconnected.
>
> The rationale is that sometimes connections aren’t properly
> reset. Currently, a connection is not collected if it was in IDLE
> state at that point. If this happens repeatedly, imapd keeps
> accumulating dead connections which can cause DOS. This patch
> solves the problem by forcing imapd to stop idling after
> exceeding the configured timeout.
> ---
> imap/imapd.c | 65
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> lib/imapoptions | 4 ++++
> 2 files changed, 66 insertions(+), 3 deletions(-)
>
> diff --git a/imap/imapd.c b/imap/imapd.c
> index 8897412..5efe973 100644
> --- a/imap/imapd.c
> +++ b/imap/imapd.c
> @@ -58,6 +58,9 @@
> #include <sys/wait.h>
> #include <netinet/in.h>
> #include <arpa/inet.h>
> +#include <time.h>
> +#include <stdbool.h>
> +#include <errno.h>
>
> #include <sasl/sasl.h>
>
> @@ -3055,6 +3058,25 @@ static void cmd_id(char *tag)
> imapd_id.did_id = 1;
> }
>
> +static bool deadline_exceeded (const struct timespec *d)
> +{
> + struct timespec now;
> +
> + if (d->tv_sec <= 0) {
> + /* No deadline configured */
> + return false;
> + }
> +
> + if (clock_gettime(CLOCK_MONOTONIC_COARSE, &now) == -1) {
> + syslog(LOG_ERR, "clock_gettime (%d %s): error reading clock",
> + errno, strerror(errno));
> + return false;
> + }
> +
> + return now.tv_sec > d->tv_sec ||
> + (now.tv_sec == d->tv_sec && now.tv_nsec > d->tv_nsec);
> +}
> +
> /*
> * Perform an IDLE command
> */
> @@ -3064,6 +3086,22 @@ static void cmd_idle(char *tag)
> int flags;
> static struct buf arg;
> static int idle_period = -1;
> + static time_t idle_timeout = -1;
> + struct timespec deadline = { 0, 0 };
> +
> + if (idle_timeout == -1) {
> + idle_timeout = config_getint(IMAPOPT_IMAPIDLETIMEOUT);
> + }
> +
> + if (idle_timeout > 0) {
> + errno = 0;
> + if (clock_gettime(CLOCK_MONOTONIC_COARSE, &deadline) == -1) {
> + syslog(LOG_ERR, "clock_gettime (%d %s): error reading
> clock",
> + errno, strerror(errno));
> + } else {
> + deadline.tv_sec += idle_timeout;
> + }
> + }
>
> if (!backend_current) { /* Local mailbox */
>
> @@ -3080,6 +3118,11 @@ static void cmd_idle(char *tag)
>
> index_release(imapd_index);
> while ((flags = idle_wait(imapd_in->fd))) {
> + if (deadline_exceeded(&deadline)) {
> + shut_down(0);
> + break;
> + }
> +
> if (flags & IDLE_INPUT) {
> /* Get continuation data */
> c = getword(imapd_in, &arg);
> @@ -3112,7 +3155,8 @@ static void cmd_idle(char *tag)
> idle_stop(index_mboxname(imapd_index));
> }
> else { /* Remote mailbox */
> - int done = 0, shutdown = 0;
> + int done = 0;
> + enum { shutdown_skip, shutdown_bye, shutdown_silent } shutdown =
> shutdown_skip;
> char buf[2048];
>
> /* get polling period */
> @@ -3144,6 +3188,11 @@ static void cmd_idle(char *tag)
>
> /* Pipe updates to client while waiting for end of command */
> while (!done) {
> + if (deadline_exceeded(&deadline)) {
> + shutdown = shutdown_silent;
> + goto done_shutdown;
> + }
> +
> /* Flush any buffered output */
> prot_flush(imapd_out);
>
> @@ -3152,7 +3201,8 @@ static void cmd_idle(char *tag)
> (shutdown_file(buf, sizeof(buf)) ||
> (imapd_userid &&
> userdeny(imapd_userid, config_ident, buf,
> sizeof(buf))))) {
> - shutdown = done = 1;
> + done = 1;
> + shutdown = shutdown_bye;
> goto done;
> }
>
> @@ -3176,12 +3226,21 @@ static void cmd_idle(char *tag)
> pipe_until_tag(backend_current, tag, 0);
> }
>
> - if (shutdown) {
> + done_shutdown:
> + switch (shutdown) {
> + case shutdown_bye:
> + ;
> char *p;
>
> for (p = buf; *p == '['; p++); /* can't have [ be first char
> */
> prot_printf(imapd_out, "* BYE [ALERT] %s\r\n", p);
> + /* fallthrough */
> + case shutdown_silent:
> shut_down(0);
> + break;
> + case shutdown_skip:
> + default:
> + break;
> }
> }
>
> diff --git a/lib/imapoptions b/lib/imapoptions
> index c53a446..bbd8f95 100644
> --- a/lib/imapoptions
> +++ b/lib/imapoptions
> @@ -892,6 +892,10 @@ Blank lines and lines beginning with ``#'' are
> ignored.
> idled is not enabled or cannot be contacted. The minimum value is
> 1. A value of 0 will disable IDLE. */
>
> +{ "imapidletimeout", 0, INT }
> +/* Timeout for idling clients (RFC 2177) in seconds. A value of zero
> (the
> + default) means no timeout is enforced. */
> +
> { "imapidresponse", 1, SWITCH }
> /* If enabled, the server responds to an ID command with a parameter
> list containing: version, vendor, support-url, os, os-version,
