redislabs / redis_extensions Goto Github PK

For example if the arity is wrong or there are missing arguments

not sure is this is specific to my module or what.
Stack trace:

Program received signal SIGSEGV, Segmentation fault.
slowlogCreateEntry (argv=0x7ffff661e4c0, argc=8, duration=262) at slowlog.c:67
67                  sdslen(argv[j]->ptr) > SLOWLOG_ENTRY_MAX_STRING)
(gdb) bt
#0  slowlogCreateEntry (argv=0x7ffff661e4c0, argc=8, duration=262) at slowlog.c:67
#1  0x000000000047061d in slowlogPushEntryIfNeeded (argv=<optimized out>, argc=<optimized out>, duration=duration@entry=262) at slowlog.c:115
#2  0x000000000042951a in call (c=c@entry=0x7ffff6704b40, flags=flags@entry=15) at server.c:2284
#3  0x000000000042c7f7 in processCommand (c=c@entry=0x7ffff6704b40) at server.c:2539
#4  0x00000000004398ff in processInputBuffer (c=0x7ffff6704b40) at networking.c:1269
#5  0x00000000004230a7 in aeProcessEvents (eventLoop=eventLoop@entry=0x7ffff662e0a0, flags=flags@entry=3) at ae.c:412
#6  0x000000000042340b in aeMain (eventLoop=0x7ffff662e0a0) at ae.c:455
#7  0x000000000042010b in main (argc=5, argv=0x7fffffffdf78) at server.c:4143

argv is:
p **argv
$3 = {type = 0, encoding = 8, lru = 2738905, refcount = 2, ptr = 0x7ffff662d8f3}

INFO remains the best single-poll way to get all of redis's information at a single glance. Could we add a section for loaded modules to it? We'll need to poll this information regardless (INFO or MODULES to make use of it, but it'd be handy to have directly in INFO.

An example I'm thinking of is having master/slave swapping happening more directly from a module in redis, e.g. telling the current master to swap roles with the child and other children to re-slave accordingly to re-form the chain (rather than a tree). Currently we have external calls to change this layout:

1 -> 2 -> 3 -> 4
to:
2 -> 1 -> 3 -> 4

This might be something we switch to a PROMOTE <host> <port>, e.g. PROMOTE server2 6379, and the module handles it all the way down with the initial pub/sub. The current APIs don't allow this of course, but we look forward to more management APIs in the future to more easily control chains and clusters with less client interaction. This is mainly due to the fact that redis stops responding to commands during a slave, so there's a lot of trouble doing this reliably from the outside quickly.

Some modules can just use [start, end, skip], some need to manually parse the string and tell you what argument positions the key names are, but some command / modules need a way to just do:
RedisModule_KeyWithName(char* keyname)

i have two examples:

1. a module with a command that in addition to getting its input and output keys, also work with additional keys (like a global index key, or a temporary scratch space key). so it needs to access a key that is not mentioned in its argument list at all.
2. a command with a syntax in which the key name is embedded inside the string and not split into a separate argument (or maybe it concatenates some arguments with a prefix or a suffix).

yesterday i experimented with embedding RRD-tool inside redis, and one of the objectives was to support the RRD command syntax so that existing applications can be ported easily.
here's an example command syntax:
redis-cli rrd.graph DEF:mydata=my.rrd:ds:AVERAGE
"my.rrd" is the key name.
these commands can be quite complex and access many keys.

Hi, guys.
I just found a typo error(very trivial)
But just report it :)

module.c:2287 existance -> existence

Thanks.

I just have tested module branch :)

For example:

int NullCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {

    RedisModule_AutoMemory(ctx);

    RedisModuleKey *key = RedisModule_OpenKey(ctx, argv[0], REDISMODULE_READ);

    RedisModule_ReplyWithNull(ctx);
    return REDISMODULE_OK;
}

Despite the reassuring comment in module.c and because zsetDel is called instead of zremCommand.

I want to deinitialize some state which I initialized in onLoad() and it seems there's no clear way to do it.
A few solutions:

1. Use gcc's __attribute__((destructor)) which means we don't need to change anything but it's gcc specific and there's no reference to the redis ctx.
2. Add an API for registering an un-load callback.
3. Add a mandatory onUnLoad symbol that needs to be implemented.
4. Like 3 but make the symbol optional and make redis smart enough to know not to reference the symbol but rather call it only if it exists.
5. Make onLoad called with an argument letting us know if this is init or de-init time.

Reproduce with:

int Test(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {
   if (argc != 1) return RedisModule_WrongArity(ctx);

   RedisModule_AutoMemory(ctx);

   RedisModuleCallReply *rep = RedisModule_Call(ctx,"COMMAND","cc","INFO","SET");
   RedisModule_ReplyWithCallReply(ctx,RedisModule_CallReplyArrayElement(rep,0));

   return REDISMODULE_OK;
}

Reproduce:

int Test_RedisCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {
    if (argc != 2) return RedisModule_WrongArity(ctx);

    const size_t offset = 10;
    const char *str = "foo";
    size_t slen = strlen(str);
    char buff[4096];
    memset(buff,'\0',offset);
    memcpy(&buff[offset],str,slen);
    buff[offset+slen]='\0';

    RedisModule_AutoMemory(ctx);
    RedisModule_ReplyWithArray(ctx,6);

    RedisModule_Call(ctx,"DEL","s",argv[1]);
    RedisModule_Call(ctx,"SET","sb",argv[1],buff,offset+slen);
    RedisModule_ReplyWithCallReply(ctx,RedisModule_Call(ctx,"GET","s",argv[1]));
    RedisModule_ReplyWithCallReply(ctx,RedisModule_Call(ctx,"DEBUG","cs","sdslen",argv[1]));

    RedisModule_Call(ctx,"DEL","s",argv[1]);
    RedisModule_Call(ctx,"SETRANGE","slc",argv[1],offset,str);
    RedisModule_ReplyWithCallReply(ctx,RedisModule_Call(ctx,"GET","s",argv[1]));
    RedisModule_ReplyWithCallReply(ctx,RedisModule_Call(ctx,"DEBUG","cs","sdslen",argv[1]));

    RedisModule_Call(ctx,"DEL","s",argv[1]);
    RedisModuleKey *key = RedisModule_OpenKey(ctx,argv[1],REDISMODULE_READ|REDISMODULE_WRITE);
    RedisModule_StringTruncate(key,offset+slen);   
    size_t len;
    char *dma = RedisModule_StringDMA(key,&len,REDISMODULE_READ|REDISMODULE_WRITE);
    memcpy(&dma[offset],str,slen);
    RedisModule_CloseKey(key);
    RedisModule_ReplyWithCallReply(ctx,RedisModule_Call(ctx,"GET","s",argv[1]));
    RedisModule_ReplyWithCallReply(ctx,RedisModule_Call(ctx,"DEBUG","cs","sdslen",argv[1]));

    return REDISMODULE_OK;
}

Output:

127.0.0.1:6379> test a
1) "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00foo"
2) key_sds_len:1, key_sds_avail:0, val_sds_len:13, val_sds_avail:0
3) "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00foo"
4) key_sds_len:1, key_sds_avail:0, val_sds_len:13, val_sds_avail:0
5) "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00foo"
6) key_sds_len:1, key_sds_avail:0, val_sds_len:13, val_sds_avail:13

Apologies; I'll be able to answer this myself soon - getting a test rig set up in a VM, but will take a short while. In particular, I'm trying to think in terms of library wrappers, and what a library would need to expose a module, in particular one that is cluster-compatible, which means the library needs to understand the arguments for the purposes of addressing.

This is broadly related to the work in progress on docs.

How about adding this feature for module commands

module list helloworld

127.0.0.1:6379> module list helloworld
 1) "hello.push.call"
 2) "hello.zsumrange"
 3) "hello.push.call2"
 4) "hello.list.splice.auto"
 5) "hello.more.expire"
 6) "hello.simple"
 7) "hello.toggle.case"
 8) "hello.hcopy"
 9) "hello.push.native"
10) "hello.list.sum.len"
11) "hello.rand.array"
12) "hello.lexrange"
13) "hello.repl1"
14) "hello.repl2"
15) "hello.list.splice"

patch.file

diff --git src/module.c src/module.c
index af1e2f0..59fca79 100644
--- src/module.c
+++ src/module.c
@@ -2438,21 +2438,51 @@ void moduleCommand(client *c) {
             }
             addReplyErrorFormat(c,"Error unloading module: %s",errmsg);
         }
-    } else if (!strcasecmp(subcmd,"list") && c->argc == 2) {
-        dictIterator *di = dictGetIterator(modules);
-        dictEntry *de;
-
-        addReplyMultiBulkLen(c,dictSize(modules));
-        while ((de = dictNext(di)) != NULL) {
-            sds name = dictGetKey(de);
-            struct RedisModule *module = dictGetVal(de);
-            addReplyMultiBulkLen(c,4);
-            addReplyBulkCString(c,"name");
-            addReplyBulkCBuffer(c,name,sdslen(name));
-            addReplyBulkCString(c,"ver");
-            addReplyLongLong(c,module->ver);
+    } else if (!strcasecmp(subcmd,"list")) {
+        if (c->argc == 2) {
+            dictIterator *di = dictGetIterator(modules);
+            dictEntry *de;
+
+            addReplyMultiBulkLen(c,dictSize(modules));
+            while ((de = dictNext(di)) != NULL) {
+                sds name = dictGetKey(de);
+                struct RedisModule *module = dictGetVal(de);
+                addReplyMultiBulkLen(c,4);
+                addReplyBulkCString(c,"name");
+                addReplyBulkCBuffer(c,name,sdslen(name));
+                addReplyBulkCString(c,"ver");
+                addReplyLongLong(c,module->ver);
+            }
+            dictReleaseIterator(di);
+        } else if (c->argc == 3) {
+            struct RedisModule *module = dictFetchValue(modules,c->argv[2]->ptr);
+            if (module == NULL) {
+                addReplyErrorFormat(c,"no such module with that name");
+                return;
+            }
+
+            int cmds = 0;
+            dictIterator *di = dictGetSafeIterator(server.commands);
+            dictEntry *de;
+            void *mbcount;
+            mbcount = addDeferredMultiBulkLength(c);
+            while ((de = dictNext(di)) != NULL) {
+                struct redisCommand *cmd = dictGetVal(de);
+                if (cmd->proc == RedisModuleCommandDispatcher) {
+                    RedisModuleCommandProxy *cp =
+                        (void*)(unsigned long)cmd->getkeys_proc;
+                    sds cmdname = cp->rediscmd->name;
+                    if (cp->module == module) {
+                        addReplyBulkCBuffer(c,cmdname,sdslen(cmdname));
+                        cmds++;
+                    }
+                }
+            }
+            setDeferredMultiBulkLength(c,mbcount,cmds);
+            dictReleaseIterator(di);
+        } else {
+            addReply(c,shared.syntaxerr);
         }
-        dictReleaseIterator(di);
     } else {
         addReply(c,shared.syntaxerr);
     }

Will there be a control for module loading in redis.conf? I don't mean to load a module (currently in the docs), I mean to load any modules. I think loading should require a flag in the config (that defaults to off). To load any modules, you'd have to enable module loading.

Reasoning: the current approach to loading off a file system allows for a few security holes. Since the error may differ on whether a file is even found or not, it provides a mechanism to crawl the file system and seeing what redis has access to. Since many people do not setup their permissions correctly, this can quickly be a problem.

A second case is it is generally a way to load unnoticed software akin to malware that the redis admin is unaware of. For example one could load a module where they're crawling sensitive data but with a new command that's not flagged in current audit logs.

I'll open another issue in a moment for module visibility in general.