mirror of
https://codeberg.org/redict/redict.git
synced 2025-01-22 16:18:28 -05:00
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.
This commit is contained in:
parent
218e522c82
commit
bb21537596
@ -2402,6 +2402,12 @@ int processCommand(client *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 C_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 == C_ERR) {
|
||||
flagTransaction(c);
|
||||
addReply(c, shared.oomerr);
|
||||
|
Loading…
Reference in New Issue
Block a user