Make sure replicas don't write their own replies to the replication link#10020
Merged
oranagra merged 3 commits intoredis:unstablefrom Jan 2, 2022
Merged
Conversation
madolson
reviewed
Dec 28, 2021
ShooterIT
approved these changes
Dec 29, 2021
Member
|
worth to mention that #8868 was a response for a complaint about DoS, user sending SYNC command then then being able to trigger an assertion. so we need to make sure the new assertion is unreachable via user workloads. |
oranagra
approved these changes
Dec 29, 2021
Contributor
|
The other concern is that a replica can disable replies with 'client reply off', which would bypass this check. If we're concerned about malicious users, I don't think this is sufficient. |
Member
|
we already made sure they can't write to the keyspace. |
enjoy-binbin
added a commit
to enjoy-binbin/redis
that referenced
this pull request
Jan 9, 2022
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 redis#10020 and the crash was reported in redis#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.:
oranagra
pushed a commit
that referenced
this pull request
Jan 10, 2022
…ink (#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.:
enjoy-binbin
added a commit
to enjoy-binbin/redis
that referenced
this pull request
Feb 11, 2022
Added regression tests for redis#10020 / redis#10081 / redis#10243. The above PRs fixed some crashes due to an asserting, see function `clientHasPendingReplies` (introduced in redis#9166). This commit added some tests to cover the above scenario. These tests will all fail in redis#9166, althought fixed not, there is value in adding these tests to cover and verify the changes. And it also can cover redis#8868 (verify the logs). Other changes: reduces the wait time in `waitForBgsave` and `waitForBgrewriteaof` from 1s to 50ms, which should reduce the time for some tests.
oranagra
pushed a commit
that referenced
this pull request
Feb 13, 2022
Added regression tests for #10020 / #10081 / #10243. The above PRs fixed some crashes due to an asserting, see function `clientHasPendingReplies` (introduced in #9166). This commit added some tests to cover the above scenario. These tests will all fail in #9166, althought fixed not, there is value in adding these tests to cover and verify the changes. And it also can cover #8868 (verify the logs). Other changes: 1. Reduces the wait time in `waitForBgsave` and `waitForBgrewriteaof` from 1s to 50ms, which should reduce the time for some tests. 2. Improve the test infra to print context when `assert_match` fails. 3. Improve the test infra to print `$error` when `assert_error` fails. ``` Expected an error matching 'ERR*' but got 'OK' (context: type eval line 4 cmd {assert_error "ERR*" {r set a b}} proc ::test) ```
enjoy-binbin
pushed a commit
to enjoy-binbin/redis
that referenced
this pull request
Jul 31, 2023
…truct (redis#10697) Move the client flags to a more cache friendly position within the client struct we regain the lost 2% of CPU cycles since v6.2 ( from 630532.57 to 647449.80 ops/sec ). These are due to higher rate of calls to getClientType due to changes in redis#9166 and redis#10020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Since #9166 we have an assertion here to make sure replica clients don't write anything to their buffer.
But in reality a replica may attempt write data to it's buffer simply by sending a command on the replication link. This command in most cases will be rejected since #8868 but it'll still generate an error.
Actually the only valid command to send on a replication link is 'REPCONF ACK` which generates no response.
We want to keep the design so that replicas can send commands but we need to avoid any situation where we start putting data in their response buffers, especially since they aren't used anymore. This PR makes sure to disconnect a rogue client which generated a write on the replication link that cause something to be written to the response buffer.
To recreate the bug this fixes simply connect via telnet to a redis server and write
sync\r\nwait for the the payload to be written and then write any command (valid or invalid), such asping\r\non the telnet connection. It'll crash the server.