Re: two system eth, chooses one w/o a gateway, makes up gateway

[Chuck Anderson <cra WPI EDU>]

On Wed, Dec 03, 2008 at 06:45:35PM -0500, Chuck Anderson wrote:
> On Wed, Dec 03, 2008 at 06:10:08PM -0500, Dan Williams wrote:
> > Do you have a gateway in /etc/sysconfig/networking at all?
> 
> No.  This is the entire /etc/sysconfig/network file:
> 
> NETWORKING=yes
> HOSTNAME=dustpuppy.wpi.edu
> NETWORKWAIT=yes
> 
> and I checked for GATEWAY in all network script files:
> 
> >grep -r GATEWAY /etc/sysconfig/networking/ /etc/sysconfig/network /etc/sysconfig/network-scripts/ifcfg-*
> /etc/sysconfig/networking/devices/ifcfg-eth1:#GATEWAY=
> /etc/sysconfig/networking/profiles/default/ifcfg-eth1:#GATEWAY=
> /etc/sysconfig/network-scripts/ifcfg-eth1:#GATEWAY=

I think I found the bug.  svGetValue returns NULL if there is no 
GATEWAY= line, and its caller get_one_ip4_addr just returns in that 
case without modifying its arguments *out_addr or **error.  
Additionally, there is no checking for that condition in 
make_ip4_setting, so that leaves tmp at whatever was passed into the 
call before, in this case, the IPADDR= value from the previous call to 
get_one_ip4_addr.  Now the GATEWAY is set to the same value as the 
IPADDR, causing this bug.

In system-settings/plugins/ifcfg-rh/reader.c:221 make_ip4_setting()

	/* Handle manual settings */
	if (!strcmp (method, NM_SETTING_IP4_CONFIG_METHOD_MANUAL)) {
		addr = nm_ip4_address_new ();

		get_one_ip4_addr (ifcfg, "IPADDR", &tmp, error);
		if (*error)
			goto error;
		nm_ip4_address_set_address (addr, tmp);

		get_one_ip4_addr (ifcfg, "GATEWAY", &tmp, error);
		if (*error)
			goto error;
		nm_ip4_address_set_gateway (addr, tmp);

		/* If no gateway in the ifcfg, try /etc/sysconfig/network instead */

First we create a nm_ip4_address_new named "addr".

Then we get_one_ip4_addr of type IPADDR and put that into tmp, then 
store it using nm_ip4_address_set_address (addr, tmp).

Then we get_one_ip4_addr of type GATEWAY and put that also into tmp 
and store it in the same way with nm_ip4_address_set_gateway(addr, 
tmp).

Drilling down into get_one_ip4_addr(), reader.c:135 we have:

get_one_ip4_addr (shvarFile *ifcfg,
                  const char *tag,
                  guint32 *out_addr,
                  GError **error)
{
	char *value = NULL;
	struct in_addr ip4_addr;

	g_return_if_fail (ifcfg != NULL);
	g_return_if_fail (tag != NULL);
	g_return_if_fail (out_addr != NULL);
	g_return_if_fail (error != NULL);
	g_return_if_fail (*error == NULL);

	value = svGetValue (ifcfg, tag);
	if (!value)
		return;

	if (inet_pton (AF_INET, value, &ip4_addr) > 0)
		*out_addr = ip4_addr.s_addr;
	else {
		g_set_error (error, ifcfg_plugin_error_quark (), 0,
		             "Invalid %s IP4 address '%s'", tag, value);
	}
	g_free (value);
}

In case svGetValue returns NULL, we just return without doing anything 
to the arguments that were passed in.  We don't even set any error 
code, so the caller has no idea there was any error and just assumes 
that the key was found and the argument contains the value.  
Unfortunately, the argument contains the exact same value that was 
passed into the function in the first place, which in the case of 
GATEWAY was the value of IPADDR from the previous call.

Checking svGetValue() in shvar.c:210:

    if (value) {
	if (value[0]) {
	    return value;
	} else {
	    g_free(value);
	    return NULL;
	}
    }

Once we find the key if its value is NULL, or if we never find the 
key, the NULL result is returned to the caller.  In either case a 
GATEWAY= line without a value or a missing GATEWAY line altogether 
will cause NULL to be returned.

My first thought to fix this was to set the error code inside 
get_one_ip4_addr in case svGetValue returns NULL, but that doesn't 
appear to be handled the way we want in get_one_ip4_addr--we don't 
want to bail if there is no GATEWAY= or a blank GATEWAY= is found--we 
want to continue looking.  So perhaps we should just NULL out tmp each 
time before we call get_one_ip4_addr.  I'll work up a patch.