[sr-dev] [kamailio/kamailio] ndb_redis: cluster support broken (#2461)

[Kritarth]

@linuxmaniac Encountered a crash regarding this today.
Relevant backtrace frame:
```
#0  0x00007fdc33f5e906 in redisc_exec (srv=0x7ffcb7b121b0, res=0x7ffcb7b121d0, cmd=0x7ffcb7b121c0) at redis_client.c:1073
        rsrv = 0x7fdc63b2d878
        rpl = 0x7fdc63b2d1d0
        c = 0 '\000'
        ap = <error reading variable ap (Attempt to dereference a generic pointer.)>
        ap2 = <error reading variable ap2 (Attempt to dereference a generic pointer.)>
        ap3 = <error reading variable ap3 (Attempt to dereference a generic pointer.)>
        ap4 = <error reading variable ap4 (Attempt to dereference a generic pointer.)>
        ret = -1
        __func__ = "redisc_exec"
```
Let's look at the code for this, with the line number mentioned on the left:
```
1052                 rpl->rplRedis = redisvCommand(rsrv->ctxRedis, cmd->s, ap3 );
1053                 if(rpl->rplRedis == NULL)
1054                 {
1055                         /* null reply, reconnect and try again */
1056                         if(rsrv->ctxRedis->err)
1057                         {
1058                                 LM_ERR("Redis error: %s\n", rsrv->ctxRedis->errstr);
1059                         }
1060                         if(redisc_reconnect_server(rsrv)==0)
1061                         {
1062                                 rpl->rplRedis = redisvCommand(rsrv->ctxRedis, cmd->s, ap4);
1063                         } else {
1064                                 LM_ERR("unable to reconnect to redis server: %.*s\n",
1065                                                 srv->len, srv->s);
1066                                 STR_ZTOV(cmd->s[cmd->len], c);
1067                                 goto error_exec;
1068                         }
1069                 }
1070         }
1071
1072         LM_DBG("rpl->rplRedis->type:%d\n", rpl->rplRedis->type);
1073         if(rpl->rplRedis->type == REDIS_REPLY_ERROR) {
1074                 LM_ERR("Redis error:%.*s\n",
1075                         (int)rpl->rplRedis->len, rpl->rplRedis->str);
1076                 goto error_exec;
1077         }
```
This crashes if the code at :1062 fails, since after that we don't perform a null check on `rpl->rplRedis` and try to get its type at :1072 (or :1073 if debug logging is disabled). Earlier this was not a problem since we never tried to fetch the reply type in this function after the missing null check.

Added a simple null check to fix this.
```
diff --git a/src/modules/ndb_redis/redis_client.c b/src/modules/ndb_redis/redis_client.c
index 42880310f2..cd3f14c7fb 100644
--- a/src/modules/ndb_redis/redis_client.c
+++ b/src/modules/ndb_redis/redis_client.c
@@ -1059,6 +1059,11 @@ int redisc_exec(str *srv, str *res, str *cmd, ...)
                        if(redisc_reconnect_server(rsrv)==0)
                        {
                                rpl->rplRedis = redisvCommand(rsrv->ctxRedis, cmd->s, ap4);
+                               if (rpl->rplRedis == NULL)
+                               {
+                                       redis_count_err_and_disable(rsrv);
+                                       goto error_exec;
+                               }
                        } else {
                                LM_ERR("unable to reconnect to redis server: %.*s\n",
                                                srv->len, srv->s);
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/issues/2461#issuecomment-788821147
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.kamailio.org/pipermail/sr-dev/attachments/20210302/4de51225/attachment.htm>