Describe the bug
Using hrandfield in the following way hangs the server. Other clients are then not able to connect to the server and get any response, potentially leading to DoS.
To reproduce
Run the following commands in the cli with the server running:
hmset myhash a 1
hrandfield myhash -9223372036854770000
Expected behavior
Returns an error code or a result, instead of hanging.
Additional information
Tested on:
Ubuntu 22.04.1 LTS
Redis ver: built from commit af0a4fe20771603f0eab75a1f60748d124cf33c3
Comment From: sundb
@yype Thanks, Perhaps we just need an issue to discuss these similar crashes(#11670, #11669, #11668). Do you wanna make a PR to fix them?
Comment From: yype
@yype Thanks, Perhaps we just need an issue to discuss these similar crashes(#11670, #11669, #11668). Do you wanna make a PR to fix them?
@sundb Thanks for the reply. I agree that we can discuss these crashes together if they share the same root cause, but I may not have time to make a PR for them. At a glance, some of them are triggered by an abort and others are triggered by segfaults, and for this issue we are looking at right now, it's also related to a 'large' number, which I thought could all be (potentially) prevented by putting precondition checks for these commands (to restrict the maxsize of memory allocation.)
Comment From: oranagra
The issue here is essentially the same as with KEYS *, asking redis to do too much work in one go, it'll loop forever and keep eating memory uncontrolled. can possibly be mitigated by Client Eviction, although not completely solved (the loop will keep running).
Note that causing redis to hang is not really a challenge (Lua scripts can easily do that too)
Comment From: yype
The issue here is essentially the same as with KEYS *, asking redis to do too much work in one go, it'll loop forever and keep eating memory uncontrolled. can possibly be mitigated by Client Eviction, although not completely solved (the loop will keep running).
Note that causing redis to hang is not really a challenge (Lua scripts can easily do that too)
Yeah, I'm not sure if Client Eviction could mitigate this problem since the loop keeps running, and force-killing the client issuing the command won't unhang the server. We might need other heuristics to abort such operations.
Comment From: oranagra
i attempted to mitigate that in the above refereed PR, not sure i'd like to proceed in that direction, and in any case it'll not kick in by default since it depends on a non default config.
Comment From: oranagra
update with decisions here: https://github.com/redis/redis/pull/11676#issuecomment-1378341710 actions: 1. merge https://github.com/redis/redis/pull/11676 as a bug fix (detach it from this issue) 2. consider improving the solution for issue and other loops for redis 7.2 and 8.0
Comment From: oranagra
The plan is to add a config with some limit to loops such as this one and others. this config will be set to a default of unlimited in Redis 7.2, and will be changed to a sensible limit in 8.0 (for being a breaking change)
Comment From: yype
The plan is to add a config with some limit to loops such as this one and others. this config will be set to a default of unlimited in Redis 7.2, and will be changed to a sensible limit in 8.0 (for being a breaking change)
Sounds good to me. Thanks for the feedback!