dhcpcd-discuss

On 12/16/2013 10:11 PM, Roy Marples wrote:
> On 14/12/2013 20:37, David McGurty wrote:
> > > Fixed here:
> > > http://roy.marples.name/cgi-bin/gitweb.cgi?p=dhcpcd.git;a=commitdiff;h=c53d79103a91e70f473d17df3caa301be615d25a 
> >
> > When will this fix go into trunk and be released as a new version?
>
> It's already in trunk.
> I should be releasing a new version soon ish, hopefully this week.
>
> > > If you can't apply the patch then just specify the arping ip's on 
> > > different lines like so:
> > > interface bond0
> > > arping 192.168.1.1
> > > arping 192.168.3.1
> > >
> > > And it should work
> >
> > Tried that. It didn't work. Seemed to me that the latter "arping" line
> > overrides the previous one. So in essence it was the same problem -
> > just the other way around. The latter profile would work, but the
> > other wouldn't.
>
> Well, it works fine in my current git head, so assume fixed in 
> dhcpcd-6.2 when it comes out!
>
> Thanks
>
> Roy

Hey Roy!

I'm reviving this thread, because contrary to your statement, the 
problem persists for me.
Only the last "arping" configuration is honoured, all others never match.

I was wondering if you tested it when having multiple arping-profiles 
and the one that should match is not the last one in the list, ie. a 
config similar to this one

arping 192.168.1.1
arping 192.168.2.1

profile aa:aa:aa:aa:aa:aa
static ip_address=192.168.1.254/24
static routers=192.168.1.1

profile bb:bb:bb:bb:bb:bb
static ip_address=192.168.2.254/24
static routers=192.168.2.1

Where the one that should match when running dhcpcd is the first one 
(192.168.1.1 on hw aa:aa:aa:aa:aa:aa)

I am asking because I was poking around in the source code and found the 
following problem:

in dhcp.c:dhcp_start() you (re-)start the arping process like this:

if (state->arping_index < ifo->arping_len) {
    arp_start(ifp);
    return;
}

And afterwards you check if the DHCPCD_STATIC flag has been set on the 
iface options:

if (ifo->options & DHCPCD_STATIC) {
    dhcp_static(ifp);
    return;
}

The problem with this when there are multiple arping profiles to chose 
from, only when the last one in the array matches does 
(state->arping_index < ifo->arping_len)   become false, and hence 
arp_start is not called.
So when an arping profile matches that is not the last in the array, the 
iface never gets configured statically.
Because arping_index is reset to 0 on every call to arp_start(), the 
first profile in the array is checked over and over again until the 
timeout is reached.

I think the order of those two statements should be reversed. The check 
for DHCPCD_STATIC has to be performed before doing arp_start()...

With the order reversed, the arping profile selection works as expected, 
however I discovered another problem that was triggered when there was 
no matching profile. The program would core-dump after trying all profiles.

I traced this back to a NULL pointer dereference in 
if_options.c:finish_config() which gets called at the very end of 
read_config(). When there is no matching profile defined in the config, 
ifo is NULL.


I have attached a patch fixing those issues. Obviously, I'm not sure if 
what i have done won't break something on other setups, since I don't 
have a deep understanding of how the code works...

With this patch, arping profiles work as expected on my setup, falling 
through to dynamic address assignment if no matching profile could be found.


Thanks for your help!

Dave




diff -Naurp dhcpcd-6.2.1/arp.c dhcpcd-6.2.1.fixed/arp.c
--- dhcpcd-6.2.1/arp.c	2014-01-15 21:24:38.000000000 +0100
+++ dhcpcd-6.2.1.fixed/arp.c	2014-02-21 23:55:26.038765158 +0100
@@ -329,6 +329,5 @@ arp_start(struct interface *ifp)
 	struct dhcp_state *state = D_STATE(ifp);
 
 	state->probes = 0;
-	state->arping_index = 0;
 	arp_probe(ifp);
 }
diff -Naurp dhcpcd-6.2.1/dhcp.c dhcpcd-6.2.1.fixed/dhcp.c
--- dhcpcd-6.2.1/dhcp.c	2014-01-15 21:24:38.000000000 +0100
+++ dhcpcd-6.2.1.fixed/dhcp.c	2014-02-21 23:54:37.442096580 +0100
@@ -2572,17 +2572,17 @@ dhcp_start(struct interface *ifp)
 	state->start_uptime = uptime();
 	free(state->offer);
 	state->offer = NULL;
-
-	if (state->arping_index < ifo->arping_len) {
-		arp_start(ifp);
-		return;
-	}
-
+	
 	if (ifo->options & DHCPCD_STATIC) {
 		dhcp_static(ifp);
 		return;
 	}
-
+ 
+	if (state->arping_index < ifo->arping_len) {
+		arp_start(ifp);
+		return;
+	}
+ 
 	if (dhcp_open(ifp) == -1)
 		return;
 
diff -Naurp dhcpcd-6.2.1/if-options.c dhcpcd-6.2.1.fixed/if-options.c
--- dhcpcd-6.2.1/if-options.c	2014-01-15 21:24:38.000000000 +0100
+++ dhcpcd-6.2.1.fixed/if-options.c	2014-02-21 23:12:02.981996066 +0100
@@ -1532,15 +1532,16 @@ parse_config_line(struct if_options *ifo
 static void
 finish_config(struct if_options *ifo)
 {
-
-	/* Terminate the encapsulated options */
-	if (ifo->vendor[0] && !(ifo->options & DHCPCD_VENDORRAW)) {
-		ifo->vendor[0]++;
-		ifo->vendor[ifo->vendor[0]] = DHO_END;
-		/* We are called twice.
-		 * This should be fixed, but in the meantime, this
-		 * guard should suffice */
-		ifo->options |= DHCPCD_VENDORRAW;
+	if (ifo != NULL) {
+		/* Terminate the encapsulated options */
+		if (ifo->vendor[0] && !(ifo->options & DHCPCD_VENDORRAW)) {
+			ifo->vendor[0]++;
+			ifo->vendor[ifo->vendor[0]] = DHO_END;
+			/* We are called twice.
+			 * This should be fixed, but in the meantime, this
+			 * guard should suffice */
+			ifo->options |= DHCPCD_VENDORRAW;
+		}
 	}
 }