Unverified Commit a1ae260e authored by Binbin's avatar Binbin Committed by GitHub
Browse files

Make sure replicas don't write their own replies to the replication link (#10081)

The following steps will crash redis-server:
```
[root]# cat crash
PSYNC replicationid -1
SLOWLOG GET
GET key
[root]# nc 127.0.0.1 6379 < crash
```

This one following #10020 and the crash was reported in #10076.

Other changes about the output info:
1. Cmd with a full name by using `getFullCommandName`, now it will print the right
   subcommand name like `slowlog|get`.
2. Print the full client info by using `catClientInfoString`, the info is also valuable.:
parent d7479107
...@@ -362,9 +362,10 @@ void _addReplyToBufferOrList(client *c, const char *s, size_t len) { ...@@ -362,9 +362,10 @@ void _addReplyToBufferOrList(client *c, const char *s, size_t len) {
* Note this is the simplest way to check a command added a response. Replication links are used to write data but * Note this is the simplest way to check a command added a response. Replication links are used to write data but
* not for responses, so we should normally never get here on a replica client. */ * not for responses, so we should normally never get here on a replica client. */
if (getClientType(c) == CLIENT_TYPE_SLAVE) { if (getClientType(c) == CLIENT_TYPE_SLAVE) {
serverLog(LL_WARNING, "Replica generated a reply to command %s, disconnecting it", sds cmdname = c->lastcmd ? getFullCommandName(c->lastcmd) : NULL;
c->lastcmd ? c->lastcmd->name : "<unknown>"); logInvalidUseAndFreeClientAsync(c, "Replica generated a reply to command %s",
freeClientAsync(c); cmdname ? cmdname : "<unknown>");
sdsfree(cmdname);
return; return;
} }
...@@ -603,6 +604,19 @@ void *addReplyDeferredLen(client *c) { ...@@ -603,6 +604,19 @@ void *addReplyDeferredLen(client *c) {
* ready to be sent, since we are sure that before returning to the * ready to be sent, since we are sure that before returning to the
* event loop setDeferredAggregateLen() will be called. */ * event loop setDeferredAggregateLen() will be called. */
if (prepareClientToWrite(c) != C_OK) return NULL; if (prepareClientToWrite(c) != C_OK) return NULL;
/* Replicas should normally not cause any writes to the reply buffer. In case a rogue replica sent a command on the
* replication link that caused a reply to be generated we'll simply disconnect it.
* Note this is the simplest way to check a command added a response. Replication links are used to write data but
* not for responses, so we should normally never get here on a replica client. */
if (getClientType(c) == CLIENT_TYPE_SLAVE) {
sds cmdname = c->lastcmd ? getFullCommandName(c->lastcmd) : NULL;
logInvalidUseAndFreeClientAsync(c, "Replica generated a reply to command %s",
cmdname ? cmdname : "<unknown>");
sdsfree(cmdname);
return NULL;
}
trimReplyUnusedTailSpace(c); trimReplyUnusedTailSpace(c);
listAddNodeTail(c->reply,NULL); /* NULL is our placeholder. */ listAddNodeTail(c->reply,NULL); /* NULL is our placeholder. */
return listLast(c->reply); return listLast(c->reply);
...@@ -1528,6 +1542,22 @@ void freeClientAsync(client *c) { ...@@ -1528,6 +1542,22 @@ void freeClientAsync(client *c) {
pthread_mutex_unlock(&async_free_queue_mutex); pthread_mutex_unlock(&async_free_queue_mutex);
} }
/* Log errors for invalid use and free the client in async way.
* We will add additional information about the client to the message. */
void logInvalidUseAndFreeClientAsync(client *c, const char *fmt, ...) {
va_list ap;
va_start(ap, fmt);
sds info = sdscatvprintf(sdsempty(), fmt, ap);
va_end(ap);
sds client = catClientInfoString(sdsempty(), c);
serverLog(LL_WARNING, "%s, disconnecting it: %s", info, client);
sdsfree(info);
sdsfree(client);
freeClientAsync(c);
}
/* Perform processing of the client before moving on to processing the next client /* Perform processing of the client before moving on to processing the next client
* this is useful for performing operations that affect the global state but can't * this is useful for performing operations that affect the global state but can't
* wait until we're done with all clients. In other words can't wait until beforeSleep() * wait until we're done with all clients. In other words can't wait until beforeSleep()
......
...@@ -2354,6 +2354,7 @@ void redisSetCpuAffinity(const char *cpulist); ...@@ -2354,6 +2354,7 @@ void redisSetCpuAffinity(const char *cpulist);
client *createClient(connection *conn); client *createClient(connection *conn);
void freeClient(client *c); void freeClient(client *c);
void freeClientAsync(client *c); void freeClientAsync(client *c);
void logInvalidUseAndFreeClientAsync(client *c, const char *fmt, ...);
int beforeNextClient(client *c); int beforeNextClient(client *c);
void resetClient(client *c); void resetClient(client *c);
void freeClientOriginalArgv(client *c); void freeClientOriginalArgv(client *c);
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment