From c138631cd16e5a28cf7f5169bee28ed4c6dae598 Mon Sep 17 00:00:00 2001 From: Matt Stancliff Date: Sat, 22 Mar 2014 18:56:28 -0400 Subject: [PATCH 1/6] Fix maxclients error handling Everywhere in the Redis code base, maxclients is treated as an int with (int)maxclients or `maxclients = atoi(source)`, so let's make maxclients an int. This fixes a bug where someone could specify a negative maxclients on startup and it would work (as well as set maxclients very high) because: unsigned int maxclients; char *update = "-300"; maxclients = atoi(update); if (maxclients < 1) goto fail; But, (maxclients < 1) can only catch the case when maxclients is exactly 0. maxclients happily sets itself to -300, which isn't -300, but rather 4294966996, which isn't < 1, so... everything "worked." maxclients config parsing checks for the case of < 1, but maxclients CONFIG SET parsing was checking for case of < 0 (allowing maxclients to be set to 0). CONFIG SET parsing is now updated to match config parsing of < 1. It's tempting to add a MINIMUM_CLIENTS define, but... I didn't. These changes were inspired by antirez#356, but this doesn't fix that issue. --- src/config.c | 2 +- src/redis.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/config.c b/src/config.c index 0d168c03..15ea1c5c 100644 --- a/src/config.c +++ b/src/config.c @@ -600,7 +600,7 @@ void configSetCommand(redisClient *c) { } else if (!strcasecmp(c->argv[2]->ptr,"maxclients")) { int orig_value = server.maxclients; - if (getLongLongFromObject(o,&ll) == REDIS_ERR || ll < 0) goto badfmt; + if (getLongLongFromObject(o,&ll) == REDIS_ERR || ll < 1) goto badfmt; /* Try to check if the OS is capable of supporting so many FDs. */ server.maxclients = ll; diff --git a/src/redis.h b/src/redis.h index fd5dac19..5d57410e 100644 --- a/src/redis.h +++ b/src/redis.h @@ -786,7 +786,7 @@ struct redisServer { list *clients_waiting_acks; /* Clients waiting in WAIT command. */ int get_ack_from_slaves; /* If true we send REPLCONF GETACK. */ /* Limits */ - unsigned int maxclients; /* Max number of simultaneous clients */ + int maxclients; /* Max number of simultaneous clients */ unsigned long long maxmemory; /* Max number of memory bytes to use */ int maxmemory_policy; /* Policy for key eviction */ int maxmemory_samples; /* Pricision of random sampling */ From 491532a713ae28fdd5e489e8f2d88076da8adf44 Mon Sep 17 00:00:00 2001 From: Matt Stancliff Date: Sat, 22 Mar 2014 19:15:04 -0400 Subject: [PATCH 2/6] Replace magic 32 with REDIS_EVENTLOOP_FDSET_INCR 32 was the additional number of file descriptors Redis would reserve when managing a too-low ulimit. The number 32 was in too many places statically, so now we use a macro instead that looks more appropriate. When Redis sets up the server event loop, it uses: server.maxclients+REDIS_EVENTLOOP_FDSET_INCR So, when reserving file descriptors, it makes sense to reserve at least REDIS_EVENTLOOP_FDSET_INCR FDs instead of only 32. Currently, REDIS_EVENTLOOP_FDSET_INCR is set to 128 in redis.h. Also, I replaced the static 128 in the while f < old loop with REDIS_EVENTLOOP_FDSET_INCR as well, which results in no change since it was already 128. Impact: Users now need at least maxclients+128 as their open file limit instead of maxclients+32 to obtain actual "maxclients" number of clients. Redis will carve the extra REDIS_EVENTLOOP_FDSET_INCR file descriptors it needs out of the "maxclients" range instead of failing to start (unless the local ulimit -n is too low to accomidate the request). --- src/redis.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/redis.c b/src/redis.c index bcdba13d..cefc1fdc 100644 --- a/src/redis.c +++ b/src/redis.c @@ -1490,21 +1490,21 @@ void initServerConfig() { } /* This function will try to raise the max number of open files accordingly to - * the configured max number of clients. It will also account for 32 additional - * file descriptors as we need a few more for persistence, listening - * sockets, log files and so forth. + * the configured max number of clients. It also reserves a number of file + * descriptors (REDIS_EVENTLOOP_FDSET_INCR) for extra operations of + * persistence, listening sockets, log files and so forth. * * If it will not be possible to set the limit accordingly to the configured * max number of clients, the function will do the reverse setting * server.maxclients to the value that we can actually handle. */ void adjustOpenFilesLimit(void) { - rlim_t maxfiles = server.maxclients+32; + rlim_t maxfiles = server.maxclients+REDIS_EVENTLOOP_FDSET_INCR; struct rlimit limit; if (getrlimit(RLIMIT_NOFILE,&limit) == -1) { redisLog(REDIS_WARNING,"Unable to obtain the current NOFILE limit (%s), assuming 1024 and setting the max clients configuration accordingly.", strerror(errno)); - server.maxclients = 1024-32; + server.maxclients = 1024-REDIS_EVENTLOOP_FDSET_INCR; } else { rlim_t oldlimit = limit.rlim_cur; @@ -1518,11 +1518,11 @@ void adjustOpenFilesLimit(void) { limit.rlim_cur = f; limit.rlim_max = f; if (setrlimit(RLIMIT_NOFILE,&limit) != -1) break; - f -= 128; + f -= REDIS_EVENTLOOP_FDSET_INCR; } if (f < oldlimit) f = oldlimit; if (f != maxfiles) { - server.maxclients = f-32; + server.maxclients = f-REDIS_EVENTLOOP_FDSET_INCR; redisLog(REDIS_WARNING,"Unable to set the max number of files limit to %d (%s), setting the max clients configuration to %d.", (int) maxfiles, strerror(errno), (int) server.maxclients); } else { From 4a25983f8fe70e105bfd7cc42fed825001afd1df Mon Sep 17 00:00:00 2001 From: Matt Stancliff Date: Sat, 22 Mar 2014 19:23:01 -0400 Subject: [PATCH 3/6] Improve error handling around setting ulimits The log messages about open file limits have always been slightly opaque and confusing. Here's an attempt to fix their wording, detail, and meaning. Users will have a better understanding of how to fix very common problems with these reworded messages. Also, we handle a new error case when maxclients becomes less than one, essentially rendering the server unusable. We now exit on startup instead of leaving the user with a server unable to handle any connections. This fixes antirez#356 as well. --- src/redis.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/src/redis.c b/src/redis.c index cefc1fdc..43314bd5 100644 --- a/src/redis.c +++ b/src/redis.c @@ -1522,12 +1522,30 @@ void adjustOpenFilesLimit(void) { } if (f < oldlimit) f = oldlimit; if (f != maxfiles) { + int old_maxclients = server.maxclients; server.maxclients = f-REDIS_EVENTLOOP_FDSET_INCR; - redisLog(REDIS_WARNING,"Unable to set the max number of files limit to %d (%s), setting the max clients configuration to %d.", - (int) maxfiles, strerror(errno), (int) server.maxclients); + if (server.maxclients < 1) { + redisLog(REDIS_WARNING,"Your current 'ulimit -n' " + "of %llu is not enough for Redis to start. " + "Please increase your open file limit to at least " + "%llu. Exiting.", oldlimit, maxfiles); + exit(1); + } + redisLog(REDIS_WARNING,"You requested maxclients of %d " + "requiring at least %llu max file descriptors.", + old_maxclients, maxfiles); + redisLog(REDIS_WARNING,"Redis can't set maximum open files " + "to %llu because of OS error: %s.", + maxfiles, strerror(errno)); + redisLog(REDIS_WARNING,"Current maximum open files is %llu. " + "maxclients has been reduced to %d to compensate for " + "low ulimit. " + "If you need higher maxclients increase 'ulimit -n'.", + oldlimit, server.maxclients); } else { - redisLog(REDIS_NOTICE,"Max number of open files set to %d", - (int) maxfiles); + redisLog(REDIS_NOTICE,"Increased maximum number of open files " + "to %llu (it was originally set to %llu).", + maxfiles, oldlimit); } } } From 90b844212d38fec80d0690d30396c462aa5655cd Mon Sep 17 00:00:00 2001 From: Matt Stancliff Date: Sat, 22 Mar 2014 19:29:28 -0400 Subject: [PATCH 4/6] Fix infinite loop on startup if ulimit too low MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fun fact: rlim_t is an unsigned long long on all platforms. Continually subtracting from a rlim_t makes it get smaller and smaller until it wraps, then you're up to 2^64-1. This was causing an infinite loop on Redis startup if your ulimit was extremely (almost comically) low. The case of (f > oldlimit) would never be met in a case like: f = 150 while (f > 20) f -= 128 Since f is unsigned, it can't go negative and would take on values of: Iteration 1: 150 - 128 => 22 Iteration 2: 22 - 128 => 18446744073709551510 Iterations 3-∞: ... To catch the wraparound, we use the previous value of f stored in limit.rlimit_cur. If we subtract from f and get a larger number than the value it had previously, we print an error and exit since we don't have enough file descriptors to help the user at this point. Thanks to @bs3g for the inspiration to fix this problem. Patches existed from @bs3g at antirez#1227, but I needed to repair a few other parts of Redis simultaneously, so I didn't get a chance to use them. --- src/redis.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/redis.c b/src/redis.c index 43314bd5..69551375 100644 --- a/src/redis.c +++ b/src/redis.c @@ -1519,6 +1519,17 @@ void adjustOpenFilesLimit(void) { limit.rlim_max = f; if (setrlimit(RLIMIT_NOFILE,&limit) != -1) break; f -= REDIS_EVENTLOOP_FDSET_INCR; + if (f > limit.rlim_cur) { + /* Instead of getting smaller, f just got bigger. + * That means it wrapped around its unsigned floor + * and is now closer to 2^64. We can't help anymore. */ + redisLog(REDIS_WARNING,"Failed to set max file limit. " + "You requested maxclients of %d " + "but your 'ulimit -n' is set to %llu. " + "Please increase your 'ulimit -n' to at least %llu.", + server.maxclients, oldlimit, maxfiles); + exit(1); + } } if (f < oldlimit) f = oldlimit; if (f != maxfiles) { From 3b54ee6ea414e0b52a4936f341be5c8c865959c6 Mon Sep 17 00:00:00 2001 From: Matt Stancliff Date: Mon, 24 Mar 2014 13:15:35 -0400 Subject: [PATCH 5/6] Add REDIS_MIN_RESERVED_FDS define for open fds Also update the original REDIS_EVENTLOOP_FDSET_INCR to include REDIS_MIN_RESERVED_FDS. REDIS_EVENTLOOP_FDSET_INCR exists to make sure more than (maxclients+RESERVED) entries are allocated, but we can only guarantee that if we include the current value of REDIS_MIN_RESERVED_FDS as a minimum for the INCR size. --- src/redis.c | 10 +++++----- src/redis.h | 5 +++-- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/redis.c b/src/redis.c index 69551375..853ef5c8 100644 --- a/src/redis.c +++ b/src/redis.c @@ -1491,20 +1491,20 @@ void initServerConfig() { /* This function will try to raise the max number of open files accordingly to * the configured max number of clients. It also reserves a number of file - * descriptors (REDIS_EVENTLOOP_FDSET_INCR) for extra operations of + * descriptors (REDIS_MIN_RESERVED_FDS) for extra operations of * persistence, listening sockets, log files and so forth. * * If it will not be possible to set the limit accordingly to the configured * max number of clients, the function will do the reverse setting * server.maxclients to the value that we can actually handle. */ void adjustOpenFilesLimit(void) { - rlim_t maxfiles = server.maxclients+REDIS_EVENTLOOP_FDSET_INCR; + rlim_t maxfiles = server.maxclients+REDIS_MIN_RESERVED_FDS; struct rlimit limit; if (getrlimit(RLIMIT_NOFILE,&limit) == -1) { redisLog(REDIS_WARNING,"Unable to obtain the current NOFILE limit (%s), assuming 1024 and setting the max clients configuration accordingly.", strerror(errno)); - server.maxclients = 1024-REDIS_EVENTLOOP_FDSET_INCR; + server.maxclients = 1024-REDIS_MIN_RESERVED_FDS; } else { rlim_t oldlimit = limit.rlim_cur; @@ -1518,7 +1518,7 @@ void adjustOpenFilesLimit(void) { limit.rlim_cur = f; limit.rlim_max = f; if (setrlimit(RLIMIT_NOFILE,&limit) != -1) break; - f -= REDIS_EVENTLOOP_FDSET_INCR; + f -= REDIS_MIN_RESERVED_FDS; if (f > limit.rlim_cur) { /* Instead of getting smaller, f just got bigger. * That means it wrapped around its unsigned floor @@ -1534,7 +1534,7 @@ void adjustOpenFilesLimit(void) { if (f < oldlimit) f = oldlimit; if (f != maxfiles) { int old_maxclients = server.maxclients; - server.maxclients = f-REDIS_EVENTLOOP_FDSET_INCR; + server.maxclients = f-REDIS_MIN_RESERVED_FDS; if (server.maxclients < 1) { redisLog(REDIS_WARNING,"Your current 'ulimit -n' " "of %llu is not enough for Redis to start. " diff --git a/src/redis.h b/src/redis.h index 5d57410e..14a656b3 100644 --- a/src/redis.h +++ b/src/redis.h @@ -123,6 +123,7 @@ #define REDIS_IP_STR_LEN INET6_ADDRSTRLEN #define REDIS_PEER_ID_LEN (REDIS_IP_STR_LEN+32) /* Must be enough for ip:port */ #define REDIS_BINDADDR_MAX 16 +#define REDIS_MIN_RESERVED_FDS 32 #define ACTIVE_EXPIRE_CYCLE_LOOKUPS_PER_LOOP 20 /* Loopkups per loop. */ #define ACTIVE_EXPIRE_CYCLE_FAST_DURATION 1000 /* Microseconds */ @@ -139,9 +140,9 @@ #define REDIS_LONGSTR_SIZE 21 /* Bytes needed for long -> str */ #define REDIS_AOF_AUTOSYNC_BYTES (1024*1024*32) /* fdatasync every 32MB */ /* When configuring the Redis eventloop, we setup it so that the total number - * of file descriptors we can handle are server.maxclients + FDSET_INCR + * of file descriptors we can handle are server.maxclients + RESERVED_FDS + FDSET_INCR * that is our safety margin. */ -#define REDIS_EVENTLOOP_FDSET_INCR 128 +#define REDIS_EVENTLOOP_FDSET_INCR (REDIS_MIN_RESERVED_FDS+96) /* Hash table parameters */ #define REDIS_HT_MINFILL 10 /* Minimal hash table fill 10% */ From 386a46946babca7b9962a94837ef96bb40b448c7 Mon Sep 17 00:00:00 2001 From: Matt Stancliff Date: Mon, 24 Mar 2014 13:21:15 -0400 Subject: [PATCH 6/6] Fix potentially incorrect errno usage errno may be reset by the previous call to redisLog, so capture the original value for proper error reporting. --- src/redis.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/redis.c b/src/redis.c index 853ef5c8..dc3b30fc 100644 --- a/src/redis.c +++ b/src/redis.c @@ -1534,6 +1534,7 @@ void adjustOpenFilesLimit(void) { if (f < oldlimit) f = oldlimit; if (f != maxfiles) { int old_maxclients = server.maxclients; + int original_errno = errno; server.maxclients = f-REDIS_MIN_RESERVED_FDS; if (server.maxclients < 1) { redisLog(REDIS_WARNING,"Your current 'ulimit -n' " @@ -1547,7 +1548,7 @@ void adjustOpenFilesLimit(void) { old_maxclients, maxfiles); redisLog(REDIS_WARNING,"Redis can't set maximum open files " "to %llu because of OS error: %s.", - maxfiles, strerror(errno)); + maxfiles, strerror(original_errno)); redisLog(REDIS_WARNING,"Current maximum open files is %llu. " "maxclients has been reduced to %d to compensate for " "low ulimit. "