Fix crashes in the Redis xbit storage path

[atobar-quadrant]

Why this PR exists

Sagan can crash when xbit storage is set to redis and the Redis connection has problems. The crash forces deployments off Redis storage and onto mmap, which costs real functionality: with mmap the original event that set an xbit is never saved, so correlated alerts (for example a "login after brute force" rule using xbits: isset) carry their own event as the correlation data instead of the event that set the bit, and a Sagan restart erases all xbits, so correlations that span a restart never fire.

This PR fixes the crash, plus three related memory bugs in the same file. All changes are in src/redis.c. A second commit adds one #include line to src/config-yaml.c to fix a build error that only appears with --disable-lognorm.

The explanations below assume no C background, so reviewers who mostly work in other languages can follow what changed and why.

Bug 1: the crash (reading an answer that never arrived)

When Sagan talks to Redis it uses a library called hiredis. You ask Redis a question with redisCommand() and get back an answer object called reply. If the connection dies while the question is in flight, hiredis has no answer to give you, so it returns NULL, which is C for "nothing".

The old login code assumed an answer always comes back and immediately read a field inside it:

reply = redisCommand(config->c_reader_redis, "AUTH %s", config->redis_password);

if (!strcmp(reply->str, "OK"))     /* reads reply->str without checking reply first */

Reading a field of "nothing" makes the operating system kill the process on the spot. That is the segfault. The AUTH (login) step runs every time Sagan connects or reconnects to Redis, so a connectivity flap lands on exactly this line. This is why the crash shows up together with Redis connection trouble and not at any other time.

The fix checks for the missing answer, drops the dead connection, waits 2 seconds, and retries the whole connect-and-login sequence:

reply = redisCommand(config->c_reader_redis, "AUTH %s", config->redis_password);

if ( reply == NULL )
    {
        redisFree(config->c_reader_redis);
        config->c_reader_redis = NULL;
        Sagan_Log(WARN, "[%s, line %d] Redis 'reader' disconnected during AUTH! Sleeping for 2 seconds!", __FILE__, __LINE__);
        sleep(2);
        continue;    /* back to the top of the connect/login loop */
    }

The 2 second pause matters. Without it, Sagan would retry tens of thousands of times per second during an outage, flooding the log and hammering the Redis host. With it, an outage costs one log line every 2 seconds and the engine reconnects by itself when Redis comes back.

A genuinely wrong password still shuts Sagan down with the same "Authentication failure" error as before. The retry only covers the case where Redis went away mid-login. The same fix is applied to both connection paths (the "reader" and the "writer").

Bug 2: commands and data were used as templates

Some C functions take a template string where % marks a placeholder to fill in, the way AUTH %s above fills in the password. Redis_Reader() handed the Redis command, and later the data coming back from Redis, to these functions as the template itself:

reply = redisCommand(config->c_reader_redis, redis_command);    /* command used as a template */
...
snprintf(str, size, reply->str);                                /* stored data used as a template */

If the stored data contains a % character, the function treats it as a placeholder instruction and goes looking for a value that was never provided. It reads whatever garbage happens to be in memory, which corrupts the result or crashes the process. The values stored in these keys include log payloads, which are outside input, so this is a security problem as well as a stability one.

The fix sends commands as a list of words through redisCommandArgv(), which has no template step at all, and copies replies through an explicit "%s" template so the data is always treated as plain text:

reply = redisCommandArgv(config->c_reader_redis, argc, argv, argvlen);
...
snprintf(str, size, "%s", reply->str);

One warning for anyone comparing this against other hiredis code: the tempting one-line fix, redisCommand(c, "%s", redis_command), does not work. hiredis then sends the whole string ("GET sagan:...") to Redis as a single word, Redis rejects it as an unknown command, and every xbit lookup silently returns empty. I hit this during testing. redisCommandArgv() is the correct API.

Bug 3: writing one byte past the end of a buffer

Sagan reserves a fixed amount of memory for the reply it copies out of Redis. The writer side stores values up to twice that size (message_buffer_size * 2 on write, message_buffer_size on read). After copying, the old code wrote a string terminator at the position matching the reply's full length:

snprintf(str, size, reply->str);
str[reply->len] = '\0';     /* reply->len can be larger than the buffer named 'str' */

For a large correlation payload that terminator lands outside the reserved memory, on top of whatever is stored next. Sometimes nothing visible happens. Sometimes the process dies later in an unrelated place, which is the kind of intermittent crash that is nearly impossible to trace from a core dump. snprintf() already limits the copy to the buffer size and terminates it, so the extra write is simply removed.

Bug 4: sloppy cleanup in the reconnect loops

Two smaller problems in the retry loops that only matter during connection trouble. The reader loop released a failed connection back to the system and then read its status through the loop condition, touching memory it had already given back. The writer loop never released failed connection attempts at all, so every retry during an outage leaked a little memory. Both paths now release the connection and clear the pointer before retrying.

How this was tested

Two setups, both scripted and repeatable.

First, crash reproduction with fault injection. A small Python script pretends to be Redis: it accepts the TCP connection and hangs up immediately, which is the same shape as a real connectivity flap between connect and login. Pointed at it with a password configured:

• the unmodified build dies with SIGSEGV on the first connection
• the patched build logs Redis 'reader' disconnected during AUTH! Sleeping for 2 seconds! and keeps retrying at 2 second intervals (8 attempts in a 15 second run), then connects normally once a real Redis appears on that port

Second, end to end against a real Redis 7 with requirepass set. Using one rule with xbits: set and one with xbits: isset:

• the first test event fired the set rule and stored the event in Redis under sagan:<cluster>:<xbit_name>:<ip> with the expected expiry
• the follow-up event fired the isset rule, read that stored event back, and attached it as the correlation data

The debug log from that run, trimmed:

[D] ... [redis.c, line 504] Redis 'string' Reply: "{ "sensor": "redis-crash-repro", ... "signature": "REPRO brute force set", "sid": 9000001, ... "storage": "redis", ... }"
[D] ... [xbit-redis.c, line 361] Rule matches all xbit conditions. Returning true.

That reply is the original event coming back as the correlation for the second alert, which is the behavior xbit-storage: redis exists to provide. The test harness (fake Redis script, minimal sagan.yaml, test rules and events) is small and I am happy to attach it if useful.

One build note

A Sagan binary built without the hiredis development library accepts xbit-storage: redis in sagan.yaml and then silently uses mmap anyway, because the fatal "redis requested but not available" check is itself compiled out in that case. When validating a build of this branch, confirm hiredis is actually linked (ldd sagan | grep hiredis) before trusting that Redis storage is in effect.