valkey
valkey copied to clipboard
valkey-io
[BUG] A large negative number(e.g. -LONG_MAX) in HRANDFIELD/ZRANDMEMBER can easily cause Valkey to crash.
Describe the bug
When using the HRANDFIELD or ZRANDMEMBER commands, if an extremely negative value(e.g. -LONG_MAX) is provided, the memory will continue to rise and eventually cause Redis to crash.
To reproduce
HRANDFIELD hash -9223372036854775807
Expected behavior
IMHO, it is more reasonable to set a limit on count according to the memory limit or the actual size of the hash table to prevent Redis from crashing. This can prevent crashes caused by careless and incorrect use. If a user really needs to sample a lot of elements that allow duplicates, he can achieve the same purpose by calling the command multiple times.
this is a known issue, https://github.com/redis/redis/issues/11671. we should be able to have a configuration item to limit these such loops.
@madolson i see you marked this, what do we want to do here? Adding a new configuration item that limit the loop, default value is LONG_MAX? I remember discussing this with Oran, and the old conclusion is that he seemed like wasn't leaning in that direction so we drop this at that time.
Hey @madolson if this issue is still up I would like to contribute in this
Hi @madolson, I saw this comment of yours https://github.com/redis/redis/pull/11676#issuecomment-1370388862 where you state that you would be fine with an approach of limiting the number of results by some factor.
Maybe we could add a configuration option that specifies that factor value, where value 0 corresponds to no limit (current behavior).
If you're fine with this approach, I can implement it.
Pinging @madolson on https://github.com/valkey-io/valkey/issues/844#issuecomment-2355151596
I did mark it apparently. It looked like an issue we could add a configuration for. We could also just disconnect the client and stop looping if such an action would generate a very large amount of memory.
I did mark it apparently. It looked like an issue we could add a configuration for. We could also just disconnect the client and stop looping if such an action would generate a very large amount of memory.
@madolson why is that issue any different than any other case of user attempting to read large amount of data (eg hgetall)? is that is because of a potential missuse of the negative count on small hashes?
IMO we already have existing generic mechanisms to enforce client COB size which can be used to prevent this memory growth. So IMO, we should not change the existing behavior and keep with the Valkey moto to "allow flexibility and shared responsibility of the users". I would, however, suggest that in order to improve, we could bail-out from random processing in case client was CLOSE_ASAP is set.
IMO we already have existing generic mechanisms ...
Agreed. The existing COB mechanism should be sufficient. Also agree that the client should be closed and the command should exit when limits are breached.
Sometimes configs represent a lack of imagination.
- Do we have existing mechanisms which could/should apply?
- Is is possible to automatically manage behavior (without a config)?
- If config is needed, is there a more appropriate general config (rather than an overly specific config)?
Every config places an additional burden on the user. Every config requires documentation, testing, and maintenance. Every config represents a potential forward/backward incompatibility.
why is that issue any different than any other case of user attempting to read large amount of data (eg hgetall)? is that is because of a potential missuse of the negative count on small hashes?
Per our discussion, we wanted to throw a useful error, but I didn't intend to strongly propose using a configuration. So, it seems the preference is to bail out of the processing of the command, similar to what we do for KEYS command: https://github.com/valkey-io/valkey/blob/ecca21331117d38c9eadbbccf5c80c34cb11ec53/src/db.c#L946C1-L947C1.
Sounds good, thanks for the update. It looks like we actually already do this:
t_hash.c: https://github.com/valkey-io/valkey/blob/unstable/src/t_hash.c#L1910 https://github.com/valkey-io/valkey/blob/unstable/src/t_hash.c#L1926
t_set.c: https://github.com/valkey-io/valkey/blob/unstable/src/t_set.c#L1053 https://github.com/valkey-io/valkey/blob/unstable/src/t_set.c#L1066
t_zset.c: https://github.com/valkey-io/valkey/blob/unstable/src/t_zset.c#L4087 https://github.com/valkey-io/valkey/blob/unstable/src/t_zset.c#L4100