Redis 2.8.24

In issue #2948 a crash was reported in processCommand(). Later Oran Agra
(@oranagra) traced the bug (in private chat) in the following sequence
of events:

1. Some maxmemory is set.
2. The slave is the currently active client and is executing PING or
   REPLCONF or whatever a slave can send to its master.
3. freeMemoryIfNeeded() is called since maxmemory is set.
4. flushSlavesOutputBuffers() is called by freeMemoryIfNeeded().
5. During slaves buffers flush, a write error could be encoutered in
   writeToClient() or sendReplyToClient() depending on the version of
   Redis. This will trigger freeClient() against the currently active
   client, so a segmentation fault will likely happen in
   processCommand() immediately after the call to freeMemoryIfNeeded().

There are different possible fixes:

1. Add flags to writeToClient() (recent versions code base) so that
   we can ignore the write errors, and use this flag in
   flushSlavesOutputBuffers(). However this is not simple to do in older
   versions of Redis.
2. Use freeClientAsync() during write errors. This works but changes the
   current behavior of releasing clients ASAP when possible. Normally
   we write to clients during the normal event loop processing, in the
   writable client, where there is no active client, so no care must be
   taken.
3. The fix of this commit: to detect that the current client is no
   longer valid. This fix is a bit "ad-hoc", but works across all the
   versions and has the advantage of not changing the remaining
   behavior. Only alters what happens during this race condition,
   hopefully.

00-RELEASENOTES

@@ -14,6 +14,23 @@ HIGH:     There is a critical bug that may affect a subset of users. Upgrade!
 CRITICAL: There is a critical bug affecting MOST USERS. Upgrade ASAP.
 --------------------------------------------------------------------------------
 
+--[ Redis 2.8.24 ] Release date: 18 Dec 2015
+
+Upgrade urgency: MODERATE. We fixed a crash that happens very rarely, so
+                 updating does not hurt, but most users are unlikely to
+                 experience this condition because it requires some odd
+                 timing.
+
+* [FIX] lua_struct.c/getnum security issue fixed. (Luca Bruno discovered it,
+        patched by Sun He and Chris Lamb)
+* [FIX] Fix a race condition in processCommand() because of interactions
+        with freeMemoryIfNeeded(). Details in issue #2948 and especially
+        in the commit message d999f5a. (Race found analytically by
+        Oran Agra, patch by Salvatore Sanfilippo)
+
+* [NEW] Log offending memory access address on SIGSEGV/SIGBUS (Salvatore
+        Sanfilippo)
+
 --[ Redis 2.8.23 ] Release date: 15 Oct 2015
 
 Upgrade urgency: MODERATE, the most important thing is a fix in the replication

deps/lua/src/lua_struct.c

@@ -89,12 +89,14 @@ typedef struct Header {
 } Header;
 
 
-static int getnum (const char **fmt, int df) {
+static int getnum (lua_State *L, const char **fmt, int df) {
   if (!isdigit(**fmt))  /* no number? */
     return df;  /* return default value */
   else {
     int a = 0;
     do {
+      if (a > (INT_MAX / 10) || a * 10 > (INT_MAX - (**fmt - '0')))
+        luaL_error(L, "integral size overflow");
       a = a*10 + *((*fmt)++) - '0';
     } while (isdigit(**fmt));
     return a;
@@ -115,9 +117,9 @@ static size_t optsize (lua_State *L, char opt, const char **fmt) {
     case 'f':  return sizeof(float);
     case 'd':  return sizeof(double);
     case 'x': return 1;
-    case 'c': return getnum(fmt, 1);
+    case 'c': return getnum(L, fmt, 1);
     case 'i': case 'I': {
-      int sz = getnum(fmt, sizeof(int));
+      int sz = getnum(L, fmt, sizeof(int));
       if (sz > MAXINTSIZE)
         luaL_error(L, "integral size %d is larger than limit of %d",
                        sz, MAXINTSIZE);
@@ -150,7 +152,7 @@ static void controloptions (lua_State *L, int opt, const char **fmt,
     case '>': h->endian = BIG; return;
     case '<': h->endian = LITTLE; return;
     case '!': {
-      int a = getnum(fmt, MAXALIGN);
+      int a = getnum(L, fmt, MAXALIGN);
       if (!isp2(a))
         luaL_error(L, "alignment %d is not a power of 2", a);
       h->align = a;

src/debug.c

@@ -813,6 +813,10 @@ void sigsegvHandler(int sig, siginfo_t *info, void *secret) {
     bugReportStart();
     redisLog(REDIS_WARNING,
         "    Redis %s crashed by signal: %d", REDIS_VERSION, sig);
+    if (sig == SIGSEGV) {
+        redisLog(REDIS_WARNING,
+        "    SIGSEGV caused by address: %p", (void*)info->si_addr);
+    }
     redisLog(REDIS_WARNING,
         "    Failed assertion: %s (%s:%d)", server.assert_failed,
                         server.assert_file, server.assert_line);

src/redis.c

@@ -2064,6 +2064,12 @@ int processCommand(redisClient *c) {
      * is returning an error. */
     if (server.maxmemory) {
         int retval = freeMemoryIfNeeded();
+        /* freeMemoryIfNeeded may flush slave output buffers. This may result
+         * into a slave, that may be the active client, to be freed. */
+        if (server.current_client == NULL) return REDIS_ERR;
+
+        /* It was impossible to free enough memory, and the command the client
+         * is trying to execute is denied during OOM conditions? Error. */
         if ((c->cmd->flags & REDIS_CMD_DENYOOM) && retval == REDIS_ERR) {
             flagTransaction(c);
             addReply(c, shared.oomerr);

src/version.h

@@ -1 +1 @@
-#define REDIS_VERSION "2.8.23"
+#define REDIS_VERSION "2.8.24"