Fix cluster config file edge case crashes #1597
Conversation
|
[Rebased this against current unstable to properly merge recent cluster.c changes] Reminder: this commit stops Redis Cluster from crashing on startup with an empty nodes file. Behavior without this branch looks like: matt@ununoctium:~/repos/redis/src% touch nodes.conf
matt@ununoctium:~/repos/redis/src% ./redis-server --cluster-enabled yes
[68207] 27 Mar 12:32:55.685 #
=== REDIS BUG REPORT START: Cut & paste starting from here ===
[68207] 27 Mar 12:32:55.685 # === ASSERTION FAILED ===
[68207] 27 Mar 12:32:55.685 # ==> cluster.c:272 'server.cluster->myself != NULL' is not true
[68207] 27 Mar 12:32:55.685 # (forcing SIGSEGV to print the bug report.)
[68207] 27 Mar 12:32:55.685 # Redis 2.9.11 crashed by signal: 11
[68207] 27 Mar 12:32:55.685 # Failed assertion: server.cluster->myself != NULL (cluster.c:272)
[68207] 27 Mar 12:32:55.685 # --- STACK TRACE
0 redis-server 0x000000010a5c73f7 logStackTrace + 199
1 redis-server 0x000000010a5c5dcc _redisAssert + 140
2 libsystem_platform.dylib 0x00007fff900335aa _sigtramp + 26
3 ??? 0x0000000000000000 0x0 + 0
4 redis-server 0x000000010a5cb172 clusterLoadConfig + 2226
5 redis-server 0x000000010a5cbc9c clusterInit + 396
6 redis-server 0x000000010a582333 initServer + 1987
7 redis-server 0x000000010a586d6d main + 1085
8 libdyld.dylib 0x00007fff879615fd start + 1
9 ??? 0x0000000000000003 0x0 + 3
[68207] 27 Mar 12:32:55.685 # --- INFO OUTPUT
Segmentation fault: 11 |
|
Hello Matt, I believe 1f9c500 is great, but the other commit behavior is dangerous, because to treat an empty config file as fine can be very dangerous. The fact that nodes.conf were empty from time to time and user reported it was due to a bug in the config file writing process, so now it can only happen when the user does dumb things like creating nodes.conf by hand. I think we should exit with an error on empty nodes.conf as well. |
This commit adds a size check after initial config line parsing to make sure we have *at least* 8 arguments per line. If someone has garbage in their cluster-config-file, Redis will now exit with an error message instead of segfaulting due to accessing argv[N] when only argv[N-1] exists. The segfault is because Redis expects at least 8 arguments per line (extra arguments get added when slots are assigned), and the config parser statically accesses argv[0] through argv[7] (and optionally argv[8+] when slots are assigned). This commit does introduce a static number in the form of "if argc<8", but the magic number 8 exists in the form of j=8 in the slot assignment parsing loop too.
|
[Rebased this against current unstable again; removed one commit]
Actually, the current behavior now treats empty config files just fine :-D You fixed it as part of e3cf812c and I've removed my implementation from this branch/PR. I think an empty nodes.conf should be treated the same as just creating a new nodes.conf. When first creating a cluster, sometimes it helps to fix permissions by hand (or pre-assign permissions before the server starts). Many automated deployment (puppet, chef, salt, ansible) or packaging systems (rpms, debs, ...) stage files in place, and it helps to allow an empty placeholder file the service can use without being in error. On a mildy unrelated note: |
This commit adds a size check after initial config line parsing to make sure we have *at least* 8 arguments per line. If someone has garbage in their cluster-config-file, Redis will now exit with an error message instead of segfaulting due to accessing argv[N] when only argv[N-1] exists. The segfault is because Redis expects at least 8 arguments per line (extra arguments get added when slots are assigned), and the config parser statically accesses argv[0] through argv[7] (and optionally argv[8+] when slots are assigned). This commit does introduce a static number in the form of "if argc<8", but the magic number 8 exists in the form of j=8 in the slot assignment parsing loop too. Closes redis#1597
This commit adds a size check after initial config line parsing to make sure we have *at least* 8 arguments per line. Also, instead of asserting for cluster->myself, we just test and error out normally (since the error does a hard exit anyway). Closes redis#1597
This commit adds a size check after initial config line parsing to make sure we have *at least* 8 arguments per line. If someone has garbage in their cluster-config-file, Redis will now exit with an error message instead of segfaulting due to accessing argv[N] when only argv[N-1] exists. The segfault is because Redis expects at least 8 arguments per line (extra arguments get added when slots are assigned), and the config parser statically accesses argv[0] through argv[7] (and optionally argv[8+] when slots are assigned). This commit does introduce a static number in the form of "if argc<8", but the magic number 8 exists in the form of j=8 in the slot assignment parsing loop too. Closes redis#1597
This commit adds a size check after initial config line parsing to make sure we have *at least* 8 arguments per line. Also, instead of asserting for cluster->myself, we just test and error out normally (since the error does a hard exit anyway). Closes redis#1597
This commit adds a size check after initial config line parsing to make sure we have *at least* 8 arguments per line. Also, instead of asserting for cluster->myself, we just test and error out normally (since the error does a hard exit anyway). Closes #1597
Fix two cluster config file related segfaults. One assert-generated segfault can be ignored and just worked through. The other now generates a proper error message instead of just crashing.