"client-output-buffer-limit" input strings treated differently between redis.conf and CONFIG SET

[henry-filgueiras]

There's a semantic difference between configuring client-output-buffer-limit through redis.conf and configuring the limits through the "CONFIG SET client-output-buffer-limit" command (which we do at runtime).

This is the path which goes through the config file ("good case"):
https://github.com/antirez/redis/blob/unstable/src/config.c#L662-L663

You'll see that it does hard = memtoll(...), and memtoll does fancy stuff to interpret the string suffix (mb/gb/etc) and return an appropriately multiplied value into a raw byte count.

Now if you look at the CONFIG SET path, you'll see it's very similar:
https://github.com/antirez/redis/blob/unstable/src/config.c#L968-L969

Except it calls hard = strtoll(...), which does not interpret the "mb"/other suffixes after each string, and interprets the string before a non-digit character as a raw byte value.

So basically, if you pass in a string that works in redis.conf to CONFIG SET, which uses human-readable suffixes, instead of raw byte values, it will succeed (aka no sign of a failed parsing), but the values will not be interpreted correctly -- you will only find out when clients start getting disconnected at a much lower burst value.

This is the redis.conf input string:
client-output-buffer-limit pubsub 32mb 8mb 60

Here's an example session... The server here is v4.0.2 (from the public docker "redis:4.0" container), but it's the same repro on 3.2.9/3.2.11 (not sure how far back it goes).

The lines in a vanilla example redis.conf are:
client-output-buffer-limit normal 0 0 0
client-output-buffer-limit slave 256mb 64mb 60
client-output-buffer-limit pubsub 32mb 8mb 60

Starting up an instance with these in redis.conf yields the following CONFIG GET result:
127.0.0.1:6379> CONFIG GET client-output*

1. "client-output-buffer-limit"
2. "normal 0 0 0 slave 268435456 67108864 60 pubsub 33554432 8388608 60"

Then, if you do this (in an ideal world, this should have equivalent expectations to the redis.conf version):
127.0.0.1:6379> CONFIG SET client-output-buffer-limit "normal 0 0 0 slave 256mb 64mb 60 pubsub 32mb 8mb 60"
OK

CONFIG GET changes:
127.0.0.1:6379> CONFIG GET client-output*

1. "client-output-buffer-limit"
2. "normal 0 0 0 slave 256 64 60 pubsub 32 8 60"

I'm guessing similar CONFIG SET vs redis.conf interpretation mismatches exist for other commands (since these are two mostly independent code paths), but I haven't checked.

The fix in the case of client-output-buffer-limit is to change the {hard,soft} = strtoll(...) in the CONFIG SET path to {hard,soft} = memtoll(...) with the appropriate parameters.

If client-output-buffer-limit through CONFIG SET is expected to have different string interpretation semantics, that should be written down somewhere (I haven't seen it), and should actively fail with some kind of parsing error if someone tries to do "20mb" in their string.

For now we will likely work around this by doing the conversion ourselves when issuing CONFIG SET (versus blindly re-using what should have worked in redis.conf path), but there is a good opportunity to add consistency between redis.conf and CONFIG SET in Redis here.

[itamarhaber]

Related: https://groups.google.com/d/msg/redis-db/m4Z4-03mW2g/q-yDgaWJAAAJ