Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ndb_redis: redis_cmd() doesn't return error with a faulty command #2300

Closed
linuxmaniac opened this issue Apr 24, 2020 · 5 comments
Closed

ndb_redis: redis_cmd() doesn't return error with a faulty command #2300

linuxmaniac opened this issue Apr 24, 2020 · 5 comments
Labels

Comments

@linuxmaniac
Copy link
Member

Description

Detected that a command like this was not returning <0

if(redis_cmd("test", "HSET dd field1", "r")) {
  [...]
}

Same command on redis-cli:

# redis-cli
127.0.0.1:6379> HSET dd field1
(error) ERR wrong number of arguments for 'hset' command

Troubleshooting

Reproduction

I've added some debug to the code and I find out that the command is returning:

Apr 23 10:55:02 spce proxy[12880]: DEBUG: ndb_redis [redis_client.c:956]: redisc_exec(): rpl->rplRedis->type:3

That's an INTEGER. From hiredis/read.h:

#define REDIS_REPLY_INTEGER 3
#define REDIS_REPLY_NIL 4
#define REDIS_REPLY_STATUS 5
#define REDIS_REPLY_ERROR 6

I've created a simple test code to check the same command:

#include <stdio.h>
#include <stdarg.h>
#include "hiredis.h"

void check(redisContext *c, const char *cmd, ...) {
        va_list ap;
        va_start(ap,cmd);
        redisReply *r = redisvCommand(c, cmd, ap);
        va_end(ap);

        if(r == NULL) {
                printf("reply NULL\n");
        }
        printf("reply->type:%d\n", r->type);
        freeReplyObject(r);
}

int main(void) {
        redisContext *c = redisConnect("127.0.0.1", 6379);

        if (c == NULL || c->err) {
            if (c) {
                printf("Error: %s\n", c->errstr);
            } else {
                printf("Can't allocate redis context\n");
            }
            return 1;
        }
        check(c, "HSET dd field");
        redisFree(c);
        return 0;
}
root@spce:/usr/local/devel# cc t.c -o t -lhiredis -I/usr/include/hiredis
root@spce:/usr/local/devel# ./t
reply->type:6

No problems there. The reply is ERROR.

Additional Information

  • Kamailio Version - output of kamailio -v
version: kamailio 5.3.3 (x86_64/linux)
flags: USE_TCP, USE_TLS, USE_SCTP, TLS_HOOKS, USE_RAW_SOCKS, DISABLE_NAGLE, USE_MCAST, NO_SIG_DEBUG, DNS_IP_HACK, SHM_MMAP, PKG_MALLOC, Q_MALLOC, F_MALLOC, TLSF_MALLOC, DBG_SR_MEMORY, USE_FUTEX, FAST_LOCK-ADAPTIVE_WAIT, USE_DNS_CACHE, USE_DNS_FAILOVER, USE_NAPTR, USE_DST_BLACKLIST, HAVE_RESOLV_RES, TLS_PTHREAD_MUTEX_SHARED
ADAPTIVE_WAIT_LOOPS 1024, MAX_RECV_BUFFER_SIZE 262144, MAX_URI_SIZE 1024, BUF_SIZE 65535, DEFAULT PKG_SIZE 8MB
poll method support: poll, epoll_lt, epoll_et, sigio_rt, select.
id: unknown
compiled with gcc 8.3.0
@miconda
Copy link
Member

miconda commented Apr 27, 2020

I don't get exactly what you report here: is a problem with the kamailio code or a strange value for type field in the redis reply. Because that redis reply structure is what redisvCommand() returns. Any further details about the issue itself?

@linuxmaniac
Copy link
Member Author

I would say our code is doing something wrong since the same command using redisvCommand() in my simple test code works as expected, returning an error reply type not an integer.

@linuxmaniac
Copy link
Member Author

I will try to explain,

"HSET dd field" is missing the value parameter and it's a faulty command, it must fail.

redis_cmd() doesn't return <0 so there was no error 0_0 ( reply->type 3, INTEGER )
My simple code test fails as expected ( reply->type 6, ERROR )

@miconda
Copy link
Member

miconda commented Apr 28, 2020

First I was not sure that you report the redis_cmd() is not returning false (negative) in such case, or the type field has the wrong value.

Then we use the same hiredis function to get the response:

rpl->rplRedis = redisvCommand(rsrv->ctxRedis, cmd->s, ap );

Strange that gets different results on your system. Have you compiled the ndb_redis module where you did the test example, or is it from debian packages?

@linuxmaniac
Copy link
Member Author

both are using the same library from buster: libhiredis-dev 0.14.0-3

@henningw henningw added the bug label May 6, 2020
linuxmaniac added a commit that referenced this issue May 7, 2020
linuxmaniac added a commit that referenced this issue May 7, 2020
linuxmaniac added a commit that referenced this issue Sep 21, 2020
linuxmaniac added a commit that referenced this issue Mar 2, 2021
fixes #2461 related #2300

(cherry picked from commit 5557b9b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants