There appears to be a bit of a problem with rlm_rediswho and attributes which contain spaces. rlm_redis_query() calls underlying redisCommand(dissocket->conn, query) in libhiredis. However this API separates arguments by spaces, and does not allow any spaces embedded with arguments, not even with quoting. If you try something like this: start-insert = "LPUSH \"%{User-Name}\" \"%{attr},%{attr},...\"" then it actually pushes keys and values with surrounding double quotes. As far as I can see, you are supposed to call redisCommand(context, "SET %s %s", key, value) if key and/or value contain spaces. https://github.com/redis/hiredis https://groups.google.com/forum/?fromgroups=#!topic/redis-db/SeDznDcXQiw Possible solutions I can see: * Add a safe-characters array like sql_xlat, and don't include space (so it expands to =20). Icky. * Parse the string, if it contains quoted words then break into %s substitutions before passing to redisCommand. This would be the most transparent solution I think. * Change rlm_rediswho so that it has template and value, e.g. alive-insert = "LPUSH %s %s" alive-insert-0 = "%{User-Name}" alive-insert-1 = "%l,%{Acct-Session-Id},%{NAS-IP-Address},%{Acct-Session-Time},%{Framed-IP-Address},%{Acct-Input-Gigawords:-0},%{Acct-Output-Gigawords:-0},%{Acct-Input-Octets:-0},%{Acct-Output-Octets:-0}" Icky again. Any one have any better ideas? Regards, Brian.
On 25/10/12 15:10, Brian Candler wrote:
* Parse the string, if it contains quoted words then break into
Why not do it for everything? Why just quoted?
%s substitutions before passing to redisCommand. This would be the most transparent solution I think.
You can probably use the escape context stuff for this. Basically: 1. Define a struct for holding the command and arguments struct redis_command { char *arg[LEN]; int argnum; } 2. Modify the redis_escape function to save to the context: int resdis_escape(REQUEST *in, char *out, size_t outlen, const char *in, void *arg) { struct redis_command *c = arg; c->arg[c->argnum++] = strdup(in); *out++ = '%'; *out++ = 's'; *out++ = '\0'; return 2; } 3. In the command func, just call radius_xlat on the command string with a zero-ed struct: char query[256]; struct redis_command c; c.argnum = 0; radius_xlat(query, sizeof(query), fmt, request, redis_escape, &c); ...the escape func will collect all your arguments and substitute "%s". You then just need to collate the "arg" into a va_thingy struct and call the va_thingy variant of the function.
On Thu, Oct 25, 2012 at 05:41:01PM +0100, Phil Mayers wrote:
You can probably use the escape context stuff for this.
Ah, I had missed redis_escape_function, and I'm just trying to get my head around this. Given a module like redis mydb { ... } and attributes Tmp-String-1 := "foo bar" Tmp-String-2 := "qux baz" then I believe what happens in Tmp-String-3 := "%{mydb:SET %{Tmp-String-1} %{Tmp-String-2}}" is that the expansions of %{Tmp-String-1} and %{Tmp-String-2} are passed via redis_escape_function? That means they would expand to %{mydb:SET foo=20bar qux=20baz} I haven't tested rlm_redis, but I don't see this happening when I use rlm_rediswho. e.g. start-insert = "LPUSH %{User-Name} %{Event-Timestamp},%{Acct-Status-Type},..." just passes through bare spaces if the user-name contains spaces. So that's the first problem to fix. In rlm_rediswho.c I suspect this line... if (!radius_xlat(query, sizeof (query), fmt, request, NULL)) { ...just needs the NULL changing to inst->redis_escape_func.
Basically:
1. Define a struct for holding the command and arguments 2. Modify the redis_escape function to save to the context:
int resdis_escape(REQUEST *in, char *out, size_t outlen, const char *in, void *arg) {
struct redis_command *c = arg; c->arg[c->argnum++] = strdup(in); *out++ = '%'; *out++ = 's'; *out++ = '\0'; return 2; }
I think there may be a problem with that approach: given start-insert = "LPUSH %{User-Name} %{Event-Timestamp},%{Acct-Status-Type},..." I don't want %{Event-Timestamp} and %{Acct-Status-Type} to be treated as separate arguments. ("LPUSH %s %s,%s", "foo", "bar", "baz") # wrong ("LUSH %s %s", "foo", "bar,baz") # right I think what's needed is an inverse of redis_escape when building the command and args, which should be pretty straightforward. I'll have a bash at coding that now. Cheers, Brian.
On 26/10/12 12:18, Brian Candler wrote:
is that the expansions of %{Tmp-String-1} and %{Tmp-String-2} are passed via redis_escape_function? That means they would expand to
Yes
if (!radius_xlat(query, sizeof (query), fmt, request, NULL)) {
...just needs the NULL changing to inst->redis_escape_func.
Probably
I think there may be a problem with that approach: given
start-insert = "LPUSH %{User-Name} %{Event-Timestamp},%{Acct-Status-Type},..."
I don't want %{Event-Timestamp} and %{Acct-Status-Type} to be treated as separate arguments.
Ah. Then yes, you'll need to parse the string before passing to xlat.
Brian Candler wrote:
So that's the first problem to fix. In rlm_rediswho.c I suspect this line...
if (!radius_xlat(query, sizeof (query), fmt, request, NULL)) {
...just needs the NULL changing to inst->redis_escape_func.
Fixed && pushed.
I think what's needed is an inverse of redis_escape when building the command and args, which should be pretty straightforward.
I'm not sure that's a good idea. Alan DeKok.
My $0.02 is to do what src/main/exec.c does. Pass the *raw* query string into rlm_redis_query. It then splits the string into argv, and calls radius_xlat() on each one. It then passes the argv array to redisCommand.
I'll see if I can rework this; I think it will involve changing redis_query to take an argv/argc set of arguments instead of a char* query. FWIW, I also found another problem, which is that the TRIM command was never being called. There's a fix for that on my branch now, but it'll get rebased when I sort the other issue. Regards, Brian.
On Fri, Oct 26, 2012 at 09:25:55PM +0100, Brian Candler wrote:
My $0.02 is to do what src/main/exec.c does. Pass the *raw* query string into rlm_redis_query. It then splits the string into argv, and calls radius_xlat() on each one. It then passes the argv array to redisCommand.
I'll see if I can rework this; I think it will involve changing redis_query to take an argv/argc set of arguments instead of a char* query.
FWIW, I also found another problem, which is that the TRIM command was never being called. There's a fix for that on my branch now, but it'll get rebased when I sort the other issue.
OK this is now available at https://github.com/candlerb/freeradius-server/commits/candlerb/redis_escapin... commit d498c4c: pushes down the xlat functionality into rlm_redis_query, where it splits into arguments, xlats each one separately, and passes to redisCommandArgv. (As a side effect of this, redis_escape_func is no longer used or needed, but I have left the code in there) commit cfc6e78: fixes the trim functionality I have given it a basic functional test, and it all seems OK. Regards, Brian.
On Fri, Oct 26, 2012 at 10:59:17PM +0100, Brian Candler wrote:
https://github.com/candlerb/freeradius-server/commits/candlerb/redis_escapin...
commit d498c4c: pushes down the xlat functionality into rlm_redis_query, where it splits into arguments, xlats each one separately, and passes to redisCommandArgv.
(As a side effect of this, redis_escape_func is no longer used or needed, but I have left the code in there)
commit cfc6e78: fixes the trim functionality
I have given it a basic functional test, and it all seems OK.
Oops, I forgot to bounds-check argc; I'll add that in a bit. However running with the old patch (the one which unescaped =20 to space) overnight with a heavy accounting feed, after a few hours it stopped processing. Final log messages were: Sat Oct 27 04:58:32 2012 : Error: Received conflicting packet from client xxxxxxxx port 1813 - ID: 133 due to unfinished request 14570729. Giving up on old request. Sat Oct 27 04:58:32 2012 : Error: Received conflicting packet from client xxxxxxxx port 1813 - ID: 244 due to unfinished request 14570737. Giving up on old request. Sat Oct 27 04:58:33 2012 : Info: WARNING: Module rlm_rediswho became unblocked for request 14570725 Sat Oct 27 04:58:33 2012 : Info: WARNING: Module rlm_rediswho became unblocked for request 14570738 Sat Oct 27 04:58:33 2012 : Info: WARNING: Module rlm_rediswho became unblocked for request 14570729 Sat Oct 27 04:58:33 2012 : Info: WARNING: Module rlm_rediswho became unblocked for request 14570734 Sat Oct 27 04:58:33 2012 : Info: WARNING: Module rlm_rediswho became unblocked for request 14570728 Sat Oct 27 04:58:33 2012 : Info: WARNING: Module rlm_rediswho became unblocked for request 14570733 Sat Oct 27 04:58:33 2012 : Info: WARNING: Module rlm_rediswho became unblocked for request 14570724 Sat Oct 27 04:58:33 2012 : Info: WARNING: Module rlm_rediswho became unblocked for request 14570732 Sat Oct 27 04:58:36 2012 : Info: WARNING: Child is hung for request 14570724 in component accounting module . Any ideas what the problem might be? This is freeradius-2.2.0 plus the redis patches. Cheers, Brian.
Brian Candler wrote:
However running with the old patch (the one which unescaped =20 to space) overnight with a heavy accounting feed, after a few hours it stopped processing. Final log messages were:
Sat Oct 27 04:58:32 2012 : Error: Received conflicting packet from client xxxxxxxx port 1813 - ID: 133 due to unfinished request 14570729. Giving up on old request. Sat Oct 27 04:58:32 2012 : Error: Received conflicting packet from client xxxxxxxx port 1813 - ID: 244 due to unfinished request 14570737. Giving up on old request. Sat Oct 27 04:58:33 2012 : Info: WARNING: Module rlm_rediswho became unblocked for request 14570725
Well, the rediswho module is taking forever to process a request.
Any ideas what the problem might be? This is freeradius-2.2.0 plus the redis patches.
The log message does say "rlm_rediswho" Alan DeKok.
Any ideas what the problem might be? This is freeradius-2.2.0 plus the redis patches.
The log message does say "rlm_rediswho"
Well, I just wondered under what circumstances the server might stop answering queries. If a particular server thread becomes unresponsive, is there some functionality in the core which kills that thread and starts a new one? Or would the server eventually grind to a halt?
On 2012-10-29, at 10:47 PM, Brian Candler <B.Candler@pobox.com> wrote:
Well, I just wondered under what circumstances the server might stop answering queries
Stop blaming the server. The log message says that the rediswho module is blocking. So.... The rediswho module is the one blocking the server.
If a particular server thread becomes unresponsive, is there some functionality in the core which kills that thread and starts a new one? Or would the server eventually grind to a halt?
You can't really kill threads. Doing that leads to memory leaks, open file descriptors, permanently locked mutexes, etc. Make sure that your databases don't take the server down. Alan DeKok.
On Tue, Oct 30, 2012 at 03:21:11AM +0100, Alan DeKok wrote:
On 2012-10-29, at 10:47 PM, Brian Candler <B.Candler@pobox.com> wrote:
Well, I just wondered under what circumstances the server might stop answering queries
Stop blaming the server.
I am not. I am trying to understand what is going on.
The log message says that the rediswho module is blocking. So.... The rediswho module is the one blocking the server.
If a particular server thread becomes unresponsive, is there some functionality in the core which kills that thread and starts a new one? Or would the server eventually grind to a halt?
You can't really kill threads. Doing that leads to memory leaks, open file descriptors, permanently locked mutexes, etc.
Make sure that your databases don't take the server down.
OK this is clear: if the rediswho module blocks indefinitely, it will take down that thread indefinitely. Once the thread pool is all in this state, the server will stop responding. This is what I saw, so I will dig deeper. The code which picks a connection from a pool appears to be closely borrowed from rlm_sql/sql.c (and therefore is probably OK) Regards, Brian.
Brian Candler wrote:
I am not. I am trying to understand what is going on.
I understand. I'm trying to point out that the log message is pretty obvious.
OK this is clear: if the rediswho module blocks indefinitely, it will take down that thread indefinitely. Once the thread pool is all in this state, the server will stop responding.
Yes. This is what happens with SQL, too. There are many posts about it on the -users list.
The code which picks a connection from a pool appears to be closely borrowed from rlm_sql/sql.c (and therefore is probably OK)
Possibly. That code is pretty horrific, and has been re-written in "master". You can always use "gdb" to attach to a running process. Then, inspect a blocked thread, to see where it's blocked. After that, the solution should be pretty obvious. Alan DeKok.
On 10/29/2012 09:47 PM, Brian Candler wrote:
Any ideas what the problem might be? This is freeradius-2.2.0 plus the redis patches.
The log message does say "rlm_rediswho"
Well, I just wondered under what circumstances the server might stop answering queries.
If all the threads are blocked, primarily.
If a particular server thread becomes unresponsive, is there some functionality in the core which kills that thread and starts a new one? Or would the server eventually grind to a halt?
As Alan says, killing threads is highly problematic because they share your memory space (you risk destroying a thread which has a lock held, or is mutating a shared data structure). It's one of the unfortunate side effects of shared-state concurrency. Maybe one day STM will save us all, but I have my doubts... ;o) Basically anything you do from FreeRADIUS needs to be "virtually non-blocking". There's no absolute answer to this, but I like to use "50 milliseconds" as a good rule of thumb. If it takes less than that, you're relatively safe. If it takes more than that, well - start thinking about your offered load and thread pool size very carefully... If you can catch it in the "locked" state, you might be able to break in with GDB and inspect the state. Most likely there's a deadlock hidden somewhere inside or "underneath" rlm_redis (e.g. in hiredis). I'll be honest - I started to get quite excited about redis, but the state of the client APIs is rapidly putting me off :o(
OK, fixes are here: https://github.com/candlerb/freeradius-server/commits/candlerb/redis_escapin... One commit adds escaping to rediswho_command (rlm_rediswho.c), and the other adds de-escaping to rlm_redis_query (rlm_redis.c). It's tested and it's working. The only question I have is, is it OK to chomp up the string in-place in rlm_redis_query, or should I strdup it first and free it later? Thanks, Brian. P.S. I copied 'mystrtok' from valuepair.c. I checked around and there are a couple of modules which use the standard strtok, which is not re-entrant; is it thread-safe?
Brian Candler wrote:
One commit adds escaping to rediswho_command (rlm_rediswho.c),
Thats already done.
and the other adds de-escaping to rlm_redis_query (rlm_redis.c).
That's probably not the best way to do it.
It's tested and it's working. The only question I have is, is it OK to chomp up the string in-place in rlm_redis_query, or should I strdup it first and free it later?
My $0.02 is to do what src/main/exec.c does. Pass the *raw* query string into rlm_redis_query. It then splits the string into argv, and calls radius_xlat() on each one. It then passes the argv array to redisCommand.
P.S. I copied 'mystrtok' from valuepair.c. I checked around and there are a couple of modules which use the standard strtok, which is not re-entrant; is it thread-safe?
The standard strtok() is not thread-safe. IIRC, the modules only use it when instantiating themsevles, which is OK. Alan DeKok.
participants (3)
-
Alan DeKok -
Brian Candler -
Phil Mayers