This version of the patch is now on master. Thanks very much for putting it together :)
I've opened https://github.com/cyrusimap/cyrus-imapd/issues/42 for backporting it to 2.5 -- expecting to do it soon, but making sure it doesn't get forgotten if I get distracted! Cheers, ellie On Thu, Sep 22, 2016, at 02:26 AM, Philipp Gesang wrote: > Use the value of the configuration variable “timeout” as an upper > limit in minutes for idle connections. To allow further > customization, add a new configuration option “imapidletimeout” > which, if greater than zero, will be used instead. 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 | 73 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++--- > lib/imapoptions | 5 ++++ > 2 files changed, 75 insertions(+), 3 deletions(-) > > diff --git a/imap/imapd.c b/imap/imapd.c > index e2bd237..745110c 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; > + } > + > + errno = 0; > + if (clock_gettime(CLOCK_MONOTONIC, &now) == -1) { > + syslog(LOG_ERR, "clock_gettime (%d %m): error reading clock", > 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,26 @@ 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) { > + idle_timeout = config_getint(IMAPOPT_TIMEOUT); > + } > + idle_timeout *= 60; /* unit is minutes */ > + } > + > + if (idle_timeout > 0) { > + errno = 0; > + if (clock_gettime(CLOCK_MONOTONIC, &deadline) == -1) { > + syslog(LOG_ERR, "clock_gettime (%d %m): error reading > clock", > + errno); > + } else { > + deadline.tv_sec += idle_timeout; > + } > + } > > if (!backend_current) { /* Local mailbox */ > > @@ -3080,6 +3122,13 @@ static void cmd_idle(char *tag) > > index_release(imapd_index); > while ((flags = idle_wait(imapd_in->fd))) { > + if (deadline_exceeded(&deadline)) { > + syslog(LOG_DEBUG, "timeout for user '%s' while idling", > + imapd_userid); > + shut_down(0); > + break; > + } > + > if (flags & IDLE_INPUT) { > /* Get continuation data */ > c = getword(imapd_in, &arg); > @@ -3112,7 +3161,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 +3194,14 @@ static void cmd_idle(char *tag) > > /* Pipe updates to client while waiting for end of command */ > while (!done) { > + if (deadline_exceeded(&deadline)) { > + syslog(LOG_DEBUG, > + "timeout for user '%s' while idling on remote > mailbox", > + imapd_userid); > + shutdown = shutdown_silent; > + goto done; > + } > + > /* Flush any buffered output */ > prot_flush(imapd_out); > > @@ -3152,7 +3210,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 +3235,20 @@ static void cmd_idle(char *tag) > pipe_until_tag(backend_current, tag, 0); > } > > - if (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 2b26241..b4e238c 100644 > --- a/lib/imapoptions > +++ b/lib/imapoptions > @@ -2115,6 +2115,11 @@ product version in the capabilities */ > allow a bit of leeway for clients that try to NOOP every 30 > minutes. */ > > +{ "imapidletimeout", 0, INT } > +/* Timeout for idling clients (RFC 2177) in minutes. If set to > + zero (the default) or less, the value of "timeout" will be > + used instead. */ > + > { "tls_ca_file", NULL, STRING, "2.5.0", "tls_client_ca_file" } > /* Deprecated in favor of \fItls_client_ca_file\fR. */ >
