Fix a false positive warning of gcc 12+ -Werror=maybe-uninitialized

[eitlane]

Hello,
The warning I have (which is treated as error) looks like this:

gcc-ar rcs libhiredis_ssl.a ssl.o
gcc -std=c99 -c -O3 -fPIC  -I/vg/hiredis/DEPS/root/include -DNDEBUG -O3 -fno-lto -ffat-lto-objects -fuse-linker-plugin -std=gnu17 -fno-working-directory -ggdb3 -I/vg/hiredis/DEPS/root/include -Wall -Wextra -Werror -Wstrict-prototypes -Wwrite-strings -Wno-missing-field-initializers -g -ggdb  -pedantic test.c
test.c: In function 'test_invalid_timeout_errors':
test.c:1358:16: error: 'c' may be used uninitialized [-Werror=maybe-uninitialized]
 1358 |     test_cond(c->err == REDIS_ERR_IO && strcmp(c->errstr, "Invalid timeout specified") == 0);
      |               ~^~~~~
test.c:76:26: note: in definition of macro 'test_cond'
   76 | #define test_cond(_c) if(_c) printf("\033[0;32mPASSED\033[0;0m\n"); else {printf("\033[0;31mFAILED\033[0;0m\n"); fails++;}
      |                          ^~
test.c:1343:19: note: 'c' was declared here
 1343 |     redisContext *c;
      |                   ^
cc1: all warnings being treated as errors
make: *** [Makefile:270: test.o] Error 1

I reproduced it with gcc 12 and 13 (I didn't try on any previous versions)

Thanks,
V

[michael-grunder]

Instead of initializing the context to NULL, what about modifying the assertion:

test_cond(c != NULL && c->err == REDIS_ERR_IO && strcmp(c->errstr, "Invalid timeout specified") == 0);

Otherwise we still could dereference a NULL pointer. This should also be done for the subsequent assertion in the function.

If you feel like making that change, I'll get this merged.

[Romain-Geissler-1A]

Yep Venci (aka @eitlane) , indeed the above proposal is better, so please proceed.

Note that you don't need to apply this patch in our own Amadeus package of hiredis, since now the other pull request about -Werror is merged, so I will apply my patch instead of this one in our package.

[eitlane]

@michael-grunder I tried that but it didn't remove the warning. I can see that something similar was done on line 217 in the same file

static redisContext *do_connect(struct config config) {
    redisContext *c = NULL;

    if (config.type == CONN_TCP) {
        c = redisConnect(config.tcp.host, config.tcp.port);
    } else if (config.type == CONN_SSL) {
        c = redisConnect(config.ssl.host, config.ssl.port);
    } else if (config.type == CONN_UNIX) {
        c = redisConnectUnix(config.unix_sock.path);
    } else if (config.type == CONN_FD) {
        /* Create a dummy connection just to get an fd to inherit */
        redisContext *dummy_ctx = redisConnectUnix(config.unix_sock.path);
        if (dummy_ctx) {
            int fd = disconnect(dummy_ctx, 1);
            printf("Connecting to inherited fd %d\n", fd);
            c = redisConnectFd(fd);
        }
    } else {
        assert(NULL);
    }

When I checked when this was made I found this PR (Fix compiler warnings) back in 2011.
Indeed when I removed the NULL to test I have this:

cc -std=c99 -c -O3 -fPIC  -DNDEBUG -O3 -ffat-lto-objects -fuse-linker-plugin -std=gnu17 -fno-working-directory -Wall -Wextra -Werror -Wstrict-prototypes -Wwri
te-strings -Wno-missing-field-initializers -g -ggdb  -pedantic test.c
test.c: In function ‘do_connect.isra’:
test.c:238:8: error: ‘c’ may be used uninitialized [-Werror=maybe-uninitialized]
  238 |     if (c == NULL) {
      |        ^
test.c:218:19: note: ‘c’ was declared here
  218 |     redisContext *c;
      |                   ^
cc1: all warnings being treated as errors
make: *** [Makefile:270: test.o] Error 1

Thanks,
V

[michael-grunder]

Feel free to keep your change that initializes the context to NULL.

That said, the assertions should also be modified to ensure it is not NULL since redisConnectWithTimeout may return NULL as well.

[eitlane]

That's reasonable. I did the update