Trouble with 7bae7c8e and radsec clients causing mutex crash
Hi. I have a bug that is manifesting on 32-bit intel but not on x86_64. Configurations are similar and I have fairly high confidence that the changes don't matter; making the configurations identical is difficult. Notes about the configuration: * dynamic is true in proxy.conf * sites-available/tls is the same as the stock configuration except that I've added a default radsec client: client default { ipaddr = 0.0.0.0 netmask = 0 proto = tls secret = radsec } When I try and connect using radsec I get: Listening on auth+acct proto tcp address * port 2083 (TLS) as server default Listening on auth address 127.0.0.1 port 18120 as server inner-tunnel Listening on auth address * port 1812 as server default Listening on acct address * port 1813 as server default Listening on auth address :: port 1812 as server default Listening on acct address :: port 1813 as server default Opening new proxy socket 'proxy address * port 0' Listening on proxy address * port 56457 Ready to process requests ... new connection request on TCP socket freeradius: tpp.c:63: __pthread_tpp_change_priority: Assertion `new_prio == -1 || (new_prio >= __sched_fifo_min_prio && new_prio <= __sched_fifo_max_prio)' failed. Aborted (gdb) bt #0 0xb7fe1424 in __kernel_vsyscall () #1 0xb7b54941 in *__GI_raise (sig=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64 #2 0xb7b57d72 in *__GI_abort () at abort.c:92 #3 0xb7b4db58 in *__GI___assert_fail (assertion=0xb7d0e64c "new_prio == -1 || (new_prio >= __sched_fifo_min_prio && new_prio <= __sched_fifo_max_prio)", file=0xb7d0e628 "tpp.c", line=63, function=0xb7d0e62e "__pthread_tpp_change_priority") at assert.c:81 #4 0xb7d0c724 in __pthread_tpp_change_priority (previous_prio=-1, new_prio=5881) at tpp.c:61 #5 0xb7d04c9d in __pthread_mutex_lock_full (mutex=0xb7c8a63c) at pthread_mutex_lock.c:415 #6 0xb7f9f8fb in rbtree_find (tree=0xb7c8a628, data=0xbffff454) at src/lib/rbtree.c:513 #7 0xb7f9f983 in rbtree_finddata (tree=0xb7c8a628, data=data@entry=0xbffff454) at src/lib/rbtree.c:539 #8 0x0805636c in client_find (clients=clients@entry=0x82f53e8, ipaddr=ipaddr@entry=0xbffff884, proto=6) at src/main/client.c:399 #9 0x0805efd4 in client_listener_find (listener=listener@entry=0x82f0ee0, ipaddr=ipaddr@entry=0xbffff884, src_port=48007) at src/main/listen.c:134 #10 0x0805fe51 in dual_tcp_accept (listener=0x82f0ee0) at src/main/listen.c:574 #11 0x08072775 in event_socket_handler (xel=0x80b2798, fd=16, ctx=0x82f0ee0) at src/main/process.c:4076 #12 0xb7fa82a9 in fr_event_loop (el=0x80b2798) at src/lib/event.c:495 #13 0x08077fd3 in radius_event_process () at src/main/process.c:4977 #14 0x08053d9f in main (argc=4, argv=0xbffffcf4) at src/main/radiusd.c:582 (gdb) f 6 #6 0xb7f9f8fb in rbtree_find (tree=0xb7c8a628, data=0xbffff454) at src/lib/rbtree.c:513 513 src/lib/rbtree.c: No such file or directory. (gdb) p tree $1 = (rbtree_t *) 0xb7c8a628 (gdb) p *tree $2 = {root = 0xb7c8a620, num_elements = -1211587040, compare = 0xb7c8a628 <main_arena+616>, free = 0xb7c8a628 <main_arena+616>, replace = 48, lock = 166, mutex = { __data = {__lock = -1211587024, __count = 3083380280, __owner = -1211587016, __kind = -1211587008, __nusers = 3083380288, {__spins = -1211587000, __list = { __next = 0xb7c8a648}}}, __size = "0\246ȷ8\246ȷ8\246ȷ@\246ȷ@\246ȷH\246ȷ", __align = -1211587024}} Needless to say, some random offset inside main_arena looks entirely out of place as a compare function. And the lock valueand num_elements don't look so good either. 399 src/main/client.c: No such file or directory. (gdb) p clients $3 = (const RADCLIENT_LIST *) 0x82f53e8 (gdb) p *clients $4 = {trees = {0x82f8800, 0x0 <repeats 31 times>, 0x82f56a8, 0x0 <repeats 96 times>}, min_prefix = 0} (gdb) p *$6 $7 = {root = 0x82f8860, num_elements = 1, compare = 0x8056050 <client_ipaddr_cmp>, free = 0, replace = false, lock = false, mutex = {__data = {__lock = 0, __count = 0, __owner = 0, __kind = 0, __nusers = 0, {__spins = 0, __list = {__next = 0x0}}}, __size = '\000' <repeats 23 times>, __align = 0}} I'd appreciate any help trying to track this down.
OK, so I've tracked down what's going on but I don't know what a clean fix is. So, we have a number of things going on. First, client_ipaddr_cmp is failing to decide that the incoming client IP address is equal to the wildcard client address with prefix 0. So, the loop at line 390 in client.c continues: for (i = max_prefix; i >= (int32_t) clients->min_prefix; i--) { Unfortunately the variables are declared unsigned and so we look at clients->trees[(unsigned) -1] It's entirely unsurprising that crashes. I've attached a patch to use a signed loop counter. I understand that if min_prefix == 0, then that comparison should succeed, and the loop should always find a client, provided of course that the IP protocol matches that of the client. However, I think it's easier to argue correctness of the code if it's clear that the loop terminates. But the main problem is why is that comparison failing? We find ourselves inside fr_ipaddr_cmp $11 = {af = 2, ipaddr = {ip4addr = {s_addr = 0}, ip6addr = {__in6_u = {__u6_addr8 = '\000' <repeats 15 times>, __u6_addr16 = {0, 0, 0, 0, 0, 0, 0, 0}, __u6_addr32 = {0, 0, 0, 0}}}}, prefix = 0 '\000', scope = 0} OK, that looks good. However: $12 = {af = 2, ipaddr = {ip4addr = {s_addr = 1510605066}, ip6addr = {__in6_u = {__u6_addr8 = "\n\001\nZ\344\370\377\277\246\255\376\267\243\342\004\b", __u6_addr16 = {266, 23050, 63716, 49151, 44454, 47102, 58019, 2052}, __u6_addr32 = {1510605066, 3221223652, 3086921126, 134537891}}}}, prefix = 0 '\000', scope = 0} Hmm, well, it's obvious why the comparison is failing. So, how is that ip address getting corrupted? It seems that fr_inaddr_mask doesn't work for prefix 0. I'm confused, when I evaluate the expression by hand in the debugger, I get the expected result, but when I watch what happens in the registers, it all goes wrong. 0xb7f94702 <fr_inaddr_mask+34>: mov $0x20,%ecx 0xb7f94707 <fr_inaddr_mask+39>: sub %edx,%ecx 0xb7f94709 <fr_inaddr_mask+41>: mov $0xffffffff,%edx 0xb7f9470e <fr_inaddr_mask+46>: shl %cl,%edx => 0xb7f94710 <fr_inaddr_mask+48>: bswap %edx 0xb7f94712 <fr_inaddr_mask+50>: and (%esi),%edx (gdb) p $cl $34 = 32 (gdb) p $edx $35 = -1 I'm not an expert enough in the C standard to figure out why I get a different answer when I run (gdb) p ~((0x00000001UL << (32 - 0)) - 1) $36 = 0 But well, it's quite clear that the processor is doing not what we want. I guess we can special case prefix == 0. do we need to do something similar for the v6 code?
From 5ae583960bbb270efd6c049842bfd2d69cb3ee8c Mon Sep 17 00:00:00 2001 From: Sam Hartman <hartmans@debian.org> Date: Thu, 10 Jul 2014 07:41:09 -0400 Subject: [PATCH] find_client: min prefix of 0 needs to work
Use signed loop counter to permit 0-1 to be <= min_prefix --- src/main/client.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/client.c b/src/main/client.c index 4888642..d8759fd 100644 --- a/src/main/client.c +++ b/src/main/client.c @@ -367,7 +367,7 @@ RADCLIENT *client_findbynumber(UNUSED const RADCLIENT_LIST *clients, UNUSED int */ RADCLIENT *client_find(RADCLIENT_LIST const *clients, fr_ipaddr_t const *ipaddr, int proto) { - uint32_t i, max_prefix; + int32_t i, max_prefix; RADCLIENT myclient; if (!clients) clients = root_clients; @@ -387,7 +387,7 @@ RADCLIENT *client_find(RADCLIENT_LIST const *clients, fr_ipaddr_t const *ipaddr, return NULL; } - for (i = max_prefix; i >= clients->min_prefix; i--) { + for (i = max_prefix; i >= (int32_t) clients->min_prefix; i--) { void *data; myclient.ipaddr = *ipaddr; -- 2.0.0.rc2
I guess a bit less opacity is in order:
0xb7f94702 <fr_inaddr_mask+34>: mov $0x20,%ecx 0xb7f94707 <fr_inaddr_mask+39>: sub %edx,%ecx 0xb7f94709 <fr_inaddr_mask+41>: mov $0xffffffff,%edx 0xb7f9470e <fr_inaddr_mask+46>: shl %cl,%edx => 0xb7f94710 <fr_inaddr_mask+48>: bswap %edx 0xb7f94712 <fr_inaddr_mask+50>: and (%esi),%edx
That's disassembley of the interesting part of fr_inaddr_mask. $cl contains 32-prefix, and $edx contains the mask we're going to and against the address.
I'm not an expert enough in the C standard to figure out why I get a different answer when I run (gdb) p ~((0x00000001UL << (32 - 0)) - 1) $36 = 0
that's running the same expression from the C that generates the assembley above, with 0 subsubstituted for prefix. That will have us anding 0 with the address which is what we want.
"Alan" == Alan DeKok <aland@deployingradius.com> writes:
Alan> Sam Hartman wrote: >> Try these two patches; they work for me. Alan> The second one makes sense. I'm not sure if the first one Alan> does. Alan> Could the comparison be changed to != instead of >= ? that Alan> should fix any issues, I think. Consider a case where I've got trees with prefix length 32 and 0. So, we start out at 32. We check the exit condition (even for the first iteration) We don't exit. So we look at trees[32] We subtract 1 from I. We keep going. Let's look at i = 1. we check the exit condition; 1 != 0 so we continue. We check trees[1] We subtract 1. We check the exit condition 0 == 0 so we exit and never consider trees[0]
participants (3)
-
Alan DeKok -
Phil Mayers -
Sam Hartman