Do command existence and arity checks when loading AOF to avoid crash #1614
Conversation
Currently, loading commands such as `cluster` or `cluster slots xxx`
from AOF will cause the server to crash.
1. `cluster` is a container command, and executing proc will cause a
crash because we do not check subcommand and arity.
2. `cluster slots xxx`, arity check fail, reply with an error from the
AOF client and trigger a panic.
Of course, there are many other ways for a problematic AOF to cause the
panic, but it is still necessary do some basic checks before executing.
In this way, in these basic cases, we can print useful error messages
instead of crashing directly.
Signed-off-by: Binbin <[email protected]>
|
Found this when looking #1611 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #1614 +/- ##
============================================
+ Coverage 70.84% 71.00% +0.16%
============================================
Files 121 121
Lines 65169 65170 +1
============================================
+ Hits 46167 46277 +110
+ Misses 19002 18893 -109
|
There was a problem hiding this comment.
How will there be commands like CLUSTER SLOTS XXXX in the AOF file? Is it to be more resilient to future changes, like we already replicate CLUSTER SETSLOT now and maybe we will replicate CLUSTER FAILOVER in #1091?
I'm thinking for a container command like CLUSTER without subcommand, and for syntax error in CLUSTER SLOTS, it's safe to just ignore the command. It is safe because it's not a write command and is has no admin side effects. WDYT?
CLUSTER SLOTS will not be in the AOF, this is something like a damaged AOF. Using other commands may also cause the crashes, not just the cluster command, this is an example.
we shouldn't ignore it in silence, i think we should report this error. |
Signed-off-by: Binbin <[email protected]>
Do command existence and arity checks when loading AOF to avoid crash
Currently, loading commands such as
clusterorcluster slots xxxfrom AOF will cause the server to crash.
clusteris a container command, and executing proc will cause acrash because we do not check subcommand and arity.
cluster slots xxx, arity check fail, reply with an error from theAOF client and trigger a panic.
Of course, there are many other ways for a problematic AOF to cause the
panic, but it is still necessary do some basic checks before executing.
In this way, in these basic cases, we can print useful error messages
instead of crashing directly.