dhcpcd-discuss

Hi David

On 21/02/2014 23:05, David McGurty wrote:
> 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 the analysis.
Your initial assumption about the problem looks correct, however the 
patch is wrong.
The error itself is not advancing to the next arpping test when we have 
a matching address but not a matching profile.
There is a secondary error which you picked up on with the possibility 
of a NULL options being passed to finish_config which I've fixed in a 
better way.

Please test and report back. I won't have time to test extensively until 
next week maybe.

Roy

Index: arp.c
==================================================================
--- arp.c
+++ arp.c
@@ -177,12 +177,23 @@
 			    (size_t)ar.ar_hln, hwbuf, sizeof(hwbuf));
 			syslog(LOG_INFO,
 			    "%s: found %s on hardware address %s",
 			    ifp->name, inet_ntoa(ina), hwaddr);
 			if (select_profile(ifp, hwaddr) == -1 &&
-			    errno == ENOENT)
-				select_profile(ifp, inet_ntoa(ina));
+			    select_profile(ifp, inet_ntoa(ina)) == -1)
+			{
+				state->probes = 0;
+				/* We didn't find a profile for this
+				 * address or hwaddr, so move to the next
+				 * arping profile */
+				if (++state->arping_index <
+				    ifp->options->arping_len)
+				{
+					arp_probe(ifp);
+					return;
+				}
+			}
 			dhcp_close(ifp);
 			eloop_timeout_delete(ifp->ctx->eloop, NULL, ifp);
 			start_interface(ifp);
 			return;
 		}

Index: dhcpcd.c
==================================================================
--- dhcpcd.c
+++ dhcpcd.c
@@ -459,32 +459,27 @@
 
 int
 select_profile(struct interface *ifp, const char *profile)
 {
 	struct if_options *ifo;
-	int ret;
 
-	ret = 0;
 	ifo = read_config(ifp->ctx, ifp->name, ifp->ssid, profile);
 	if (ifo == NULL) {
 		syslog(LOG_DEBUG, "%s: no profile %s", ifp->name, profile);
-		ret = -1;
-		goto exit;
+		return -1;
 	}
 	if (profile != NULL) {
 		strlcpy(ifp->profile, profile, sizeof(ifp->profile));
 		syslog(LOG_INFO, "%s: selected profile %s",
 		    ifp->name, profile);
 	} else
 		*ifp->profile = '\0';
 	free_options(ifp->options);
 	ifp->options = ifo;
-
-exit:
 	if (profile)
 		configure_interface1(ifp);
-	return ret;
+	return 1;
 }
 
 static void
 configure_interface(struct interface *ifp, int argc, char **argv)
 {

Index: if-options.c
==================================================================
--- if-options.c
+++ if-options.c
@@ -1993,11 +1993,11 @@
 	free(buf);
 
 	if (profile && !have_profile) {
 		free_options(ifo);
 		errno = ENOENT;
-		ifo = NULL;
+		return NULL;
 	}
 
 	finish_config(ifo);
 	return ifo;
 }