Trouble with 7bae7c8e and radsec clients causing mutex crash

[Sam Hartman]

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 at 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