• antirez's avatar
    Fix a race that may lead to the active (slave) client to be freed. · 7a7e46b2
    antirez authored
    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.
    7a7e46b2
server.c 157 KB