Fix a race that may lead to the active (slave) client to be freed.

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.

1 files changed, 7 insertions, 1 deletions

diff --git a/src/redis.c b/src/redis.c
index 34fa71462..7d95d1794 100644
--- a/src/redis.c
+++ b/src/redis.c
@@ -2064,7 +2064,13 @@ int processCommand(redisClient *c) {
      * is returning an error. */
     if (server.maxmemory) {
         int retval = freeMemoryIfNeeded();
-        if ((c->cmd->flags & REDIS_CMD_DENYOOM) && retval == REDIS_ERR) {
+        /* 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 & CMD_DENYOOM) && retval == REDIS_ERR) {
             flagTransaction(c);
             addReply(c, shared.oomerr);
             return REDIS_OK;