Skip to content

Conversation

@oranagra
Copy link
Member

This solves an issue reported in #8712 in which a replica would bypass
the client write pause check and cause an assertion due to executing a
write command during failover.

The fact is that we don't expect replicas to execute any command other
than maybe REPLCONF and PING, etc. but matching against the ADMIN
command flag is insufficient, so instead i just block keyspace access
for now.

@oranagra oranagra requested review from madolson and yossigo April 26, 2021 14:21
@oranagra oranagra linked an issue Apr 26, 2021 that may be closed by this pull request
This solves an issue reported in #8712 in which a replica would bypass
the client write pause check and cause an assertion due to executing a
write command during failover.

The fact is that we don't expect replicas to execute any command other
than maybe REPLCONF and PING, etc. but matching against the ADMIN
command flag is insufficient, so instead i just block keyspace access
for now.
Copy link
Contributor

@madolson madolson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@oranagra oranagra merged commit 46f4ebb into redis:unstable Apr 27, 2021
@oranagra oranagra mentioned this pull request May 3, 2021
oranagra added a commit that referenced this pull request May 3, 2021
…8868)

This solves an issue reported in #8712 in which a replica would bypass
the client write pause check and cause an assertion due to executing a
write command during failover.

The fact is that we don't expect replicas to execute any command other
than maybe REPLCONF and PING, etc. but matching against the ADMIN
command flag is insufficient, so instead i just block keyspace access
for now.

(cherry picked from commit 46f4ebb)
madolson pushed a commit to madolson/redis that referenced this pull request May 12, 2021
…edis#8868)

This solves an issue reported in redis#8712 in which a replica would bypass
the client write pause check and cause an assertion due to executing a
write command during failover.

The fact is that we don't expect replicas to execute any command other
than maybe REPLCONF and PING, etc. but matching against the ADMIN
command flag is insufficient, so instead i just block keyspace access
for now.
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Sep 8, 2021
…edis#8868)

This solves an issue reported in redis#8712 in which a replica would bypass
the client write pause check and cause an assertion due to executing a
write command during failover.

The fact is that we don't expect replicas to execute any command other
than maybe REPLCONF and PING, etc. but matching against the ADMIN
command flag is insufficient, so instead i just block keyspace access
for now.
oranagra pushed a commit that referenced this pull request Jan 2, 2022
…ink (#10020)

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\n wait for the the payload to
be written and then write any command (valid or invalid), such as ping\r\n on the telnet connection. It'll crash the server.
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)
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

[CRASH]The psync command will crash redis

2 participants