diff options
author | Binbin <binloveplay1314@qq.com> | 2022-02-06 19:13:56 +0800 |
---|---|---|
committer | Oran Agra <oran@redislabs.com> | 2022-04-27 16:31:52 +0300 |
commit | 2c529844fc0429268dadaead650eafae1e2f3ce2 (patch) | |
tree | cfc6b41748995e1ad0b58921bc5b95db20f99e7b | |
parent | 33f7e12b88ca7b9819bac385ca3e903862bf9ca0 (diff) | |
download | redis-2c529844fc0429268dadaead650eafae1e2f3ce2.tar.gz |
Fix PSYNC crash with wrong offset (#10243)
`PSYNC replicationid str_offset` will crash the server.
The reason is in `masterTryPartialResynchronization`,
we will call `getLongLongFromObjectOrReply` check the
offset. With a wrong offset, it will add a reply and
then trigger a full SYNC and the client become a replica.
So crash in `c->bufpos == 0 && listLength(c->reply) == 0`.
In this commit, we check the psync_offset before entering
function `masterTryPartialResynchronization`, and return.
Regardless of that crash, accepting the sync, but also replying
with an error would have corrupt the replication stream.
(cherry picked from commit 344e41c92228f3b8aade6220c48b7d8c6e7817a7)
-rw-r--r-- | src/replication.c | 19 |
1 files changed, 10 insertions, 9 deletions
diff --git a/src/replication.c b/src/replication.c index 932ce1dee..9228a247f 100644 --- a/src/replication.c +++ b/src/replication.c @@ -521,18 +521,12 @@ int replicationSetupSlaveForFullResync(client *slave, long long offset) { * * On success return C_OK, otherwise C_ERR is returned and we proceed * with the usual full resync. */ -int masterTryPartialResynchronization(client *c) { - long long psync_offset, psync_len; +int masterTryPartialResynchronization(client *c, long long psync_offset) { + long long psync_len; char *master_replid = c->argv[1]->ptr; char buf[128]; int buflen; - /* Parse the replication offset asked by the slave. Go to full sync - * on parse error: this should never happen but we try to handle - * it in a robust way compared to aborting. */ - if (getLongLongFromObjectOrReply(c,c->argv[2],&psync_offset,NULL) != - C_OK) goto need_full_resync; - /* Is the replication ID of this master the same advertised by the wannabe * slave via PSYNC? If the replication ID changed this master has a * different replication history, and there is no way to continue. @@ -779,7 +773,14 @@ void syncCommand(client *c) { * So the slave knows the new replid and offset to try a PSYNC later * if the connection with the master is lost. */ if (!strcasecmp(c->argv[0]->ptr,"psync")) { - if (masterTryPartialResynchronization(c) == C_OK) { + long long psync_offset; + if (getLongLongFromObjectOrReply(c, c->argv[2], &psync_offset, NULL) != C_OK) { + serverLog(LL_WARNING, "Replica %s asks for synchronization but with a wrong offset", + replicationGetSlaveName(c)); + return; + } + + if (masterTryPartialResynchronization(c, psync_offset) == C_OK) { server.stat_sync_partial_ok++; return; /* No full resync needed, return. */ } else { |