Bug 1519299

The describer behavior happens to all the ipv4.dhcp-* properties and some others (always ipv4 related) when method is changed to "shared":
ipv4.dhcp-client-id
ipv4.dhcp-timeout
ipv4.dhcp-send-hostname
ipv4.dhcp-hostname
ipv4.dhcp-fqdn
ipv4.ignore-auto-routes
ipv4.ignore-auto-dns
ipv4.may-fail
...

When a connection is read from a ifcfg file, if the method is "shared" the read is stopped before reading the affected properties.
This was not spotted before because a recent change makes each write being followed by a read back, updating the connection from the last read.
Before you would get the same effect only restarting NetworkManager or when reloading the connections with:
# nmcli connection reload

Pushed a patch on branch: fg/dhcp-hostname_persist-rh1519299

This will allow to read all the properties when ipv4.method == shared (so that both in memory and on-disk state will be preserved).
Please review.

if (!g_ascii_strcasecmp (method, NM_SETTING_IP4_CONFIG_METHOD_SHARED)) {

nm_streq()



+              g_free (gateway);
+              if (!read_ip4_address (ifcfg, "GATEWAY", NULL, &a, error))
+                   return NULL

This may crash due to dangling pointer. Don't free gateway yet here.



Could you move the entire 
  if (nm_streq (method, NM_SETTING_IP4_CONFIG_METHOD_SHARED)) {
check outside the loop? There is no need to read the GATEWAY multiple times, and it's not immediately obvious, that the loop is guaranteed to run at least once (though actually it always will).

(In reply to Thomas Haller from comment #3)
> if (!g_ascii_strcasecmp (method, NM_SETTING_IP4_CONFIG_METHOD_SHARED)) {
> 
> nm_streq()
> 
> 
> 
> +              g_free (gateway);
> +              if (!read_ip4_address (ifcfg, "GATEWAY", NULL, &a, error))
> +                   return NULL
> 
> This may crash due to dangling pointer. Don't free gateway yet here.

Cannot spot it, probably I'm missing something... gateway is initialized at NULL at declaration; then only:
   if (!read_full_ip4_address (ifcfg, i, NULL, &addr, &gateway, error))
may fill it with a g_strdup().
At this point g_free() will be safe...

> 
> 
> 
> Could you move the entire 
>   if (nm_streq (method, NM_SETTING_IP4_CONFIG_METHOD_SHARED)) {
> check outside the loop? There is no need to read the GATEWAY multiple times,
> and it's not immediately obvious, that the loop is guaranteed to run at
> least once (though actually it always will).

Well, I forgot to put a "break;" there:
+               if (!g_ascii_strcasecmp (method, NM_SETTING_IP4_CONFIG_METHOD_SHARED)) {
+                       g_free (gateway);
+                       if (!read_ip4_address (ifcfg, "GATEWAY", NULL, &a, error))
+                               return NULL;
+                       gateway = g_strdup (nm_utils_inet4_ntop (a, inet_buf));
--->                    break;
+               }

This was the main idea, as the original code checked for "GATEWAY" only if an IP_ADDRESS was found.. using the loop, we would do that only if "addr" had value.


Anyway...
I realized that we may not want this at all. I would just remove that snippet, as the GATEWAY should be read by read_full_ip4_address():
       if (!read_full_ip4_address (ifcfg, i, NULL, &addr, &gateway, error))

The only different behavior we could have is when in method == SHARED, we have IPADDR empty, but an IPADDR$X is defined as well as GATEWAY. In this case we will miss the GATEWAY.
Moreover we will also catch GATEWAY$X if associated to IPADDR$X now (we missed it before).

If we want to strictly follow behavior as it used to be for the above cases, I would be for moving the check of method == SHARED outside the loop, as suggested. But as I think we don't want, proposing the updated branch:
fg/dhcp-hostname_persist-rh1519299

(In reply to Francesco Giudici from comment #4)
> (In reply to Thomas Haller from comment #3)
> > if (!g_ascii_strcasecmp (method, NM_SETTING_IP4_CONFIG_METHOD_SHARED)) {
> > 
> > nm_streq()
> > 
> > 
> > 
> > +              g_free (gateway);
> > +              if (!read_ip4_address (ifcfg, "GATEWAY", NULL, &a, error))
> > +                   return NULL
> > 
> > This may crash due to dangling pointer. Don't free gateway yet here.
> 
> Cannot spot it, probably I'm missing something... gateway is initialized at
> NULL at declaration; then only:
>    if (!read_full_ip4_address (ifcfg, i, NULL, &addr, &gateway, error))
> may fill it with a g_strdup().
> At this point g_free() will be safe...

during "return NULL", gateway will be freed, because it's declared with gs_free. Cannot leave a dangling pointer in this case, and dangling pointers are ugly in general.


> > Could you move the entire 
> >   if (nm_streq (method, NM_SETTING_IP4_CONFIG_METHOD_SHARED)) {
> > check outside the loop? There is no need to read the GATEWAY multiple times,
> > and it's not immediately obvious, that the loop is guaranteed to run at
> > least once (though actually it always will).
> 
> Well, I forgot to put a "break;" there:
> +               if (!g_ascii_strcasecmp (method,
> NM_SETTING_IP4_CONFIG_METHOD_SHARED)) {
> +                       g_free (gateway);
> +                       if (!read_ip4_address (ifcfg, "GATEWAY", NULL, &a,
> error))
> +                               return NULL;
> +                       gateway = g_strdup (nm_utils_inet4_ntop (a,
> inet_buf));
> --->                    break;
> +               }
> 
> This was the main idea, as the original code checked for "GATEWAY" only if
> an IP_ADDRESS was found.. using the loop, we would do that only if "addr"
> had value.
> 
> 
> Anyway...
> I realized that we may not want this at all. I would just remove that
> snippet, as the GATEWAY should be read by read_full_ip4_address():
>        if (!read_full_ip4_address (ifcfg, i, NULL, &addr, &gateway, error))
> 
> The only different behavior we could have is when in method == SHARED, we
> have IPADDR empty, but an IPADDR$X is defined as well as GATEWAY. In this
> case we will miss the GATEWAY.
> Moreover we will also catch GATEWAY$X if associated to IPADDR$X now (we
> missed it before).
> 
> If we want to strictly follow behavior as it used to be for the above cases,
> I would be for moving the check of method == SHARED outside the loop, as
> suggested. But as I think we don't want, proposing the updated branch:
> fg/dhcp-hostname_persist-rh1519299

if (strcmp (method, NM_SETTING_IP4_CONFIG_METHOD_DISABLED) == 0)

Please let's use nm_streq() or NM_IN_STRSET()



Anyway, NMSettingIP4Config's verify() has code like 


»···} else if (   !strcmp (method, NM_SETTING_IP4_CONFIG_METHOD_LINK_LOCAL)
»···           || !strcmp (method, NM_SETTING_IP4_CONFIG_METHOD_SHARED)
    ....
»···»···if (nm_setting_ip_config_get_num_dns (s_ip) > 0) {
»···»···»···g_set_error (error,
»···»···»···             NM_CONNECTION_ERROR,
»···»···»···             NM_CONNECTION_ERROR_INVALID_PROPERTY,
»···»···»···             _("this property is not allowed for '%s=%s'"),
»···»···»···             NM_SETTING_IP_CONFIG_METHOD, method);
»···»···»···g_prefix_error (error, "%s.%s: ", NM_SETTING_IP4_CONFIG_SETTING_NAME, NM_SETTING_IP_CONFIG_DNS);
»···»···»···return FALSE;
»···»···}


that means, a shared connection with DNS servers is rejected as invalid. Hence, if a user has an ifcfg file on disc with method=shared *and* DNS servers, previously the DNS servers would have been silently ignored, now it is rejected with error.
That might be fine though, since writer always only writes connections that verify(). Hence, writer supposedly never wrote such a connection (which would need investigation and confirmation). So, at worst, the user edited such files by hand and they will stop working because we now declare them as invalid.
That's still kinda ugly. Can we avoid rejecting files that we accepted previously?

(In reply to Thomas Haller from comment #5)
> (In reply to Francesco Giudici from comment #4)
> > (In reply to Thomas Haller from comment #3)
> > > if (!g_ascii_strcasecmp (method, NM_SETTING_IP4_CONFIG_METHOD_SHARED)) {
> > > 
> > > nm_streq()
> > > 
> > > 
> > > 
> > > +              g_free (gateway);
> > > +              if (!read_ip4_address (ifcfg, "GATEWAY", NULL, &a, error))
> > > +                   return NULL
> > > 
> > > This may crash due to dangling pointer. Don't free gateway yet here.
> > 
> > Cannot spot it, probably I'm missing something... gateway is initialized at
> > NULL at declaration; then only:
> >    if (!read_full_ip4_address (ifcfg, i, NULL, &addr, &gateway, error))
> > may fill it with a g_strdup().
> > At this point g_free() will be safe...
> 
> during "return NULL", gateway will be freed, because it's declared with
> gs_free. Cannot leave a dangling pointer in this case, and dangling pointers
> are ugly in general.

Oh, sure... I missed that gateway is declared gs_free... thanks.

> 
> 
> > > Could you move the entire 
> > >   if (nm_streq (method, NM_SETTING_IP4_CONFIG_METHOD_SHARED)) {
> > > check outside the loop? There is no need to read the GATEWAY multiple times,
> > > and it's not immediately obvious, that the loop is guaranteed to run at
> > > least once (though actually it always will).
> > 
> > Well, I forgot to put a "break;" there:
> > +               if (!g_ascii_strcasecmp (method,
> > NM_SETTING_IP4_CONFIG_METHOD_SHARED)) {
> > +                       g_free (gateway);
> > +                       if (!read_ip4_address (ifcfg, "GATEWAY", NULL, &a,
> > error))
> > +                               return NULL;
> > +                       gateway = g_strdup (nm_utils_inet4_ntop (a,
> > inet_buf));
> > --->                    break;
> > +               }
> > 
> > This was the main idea, as the original code checked for "GATEWAY" only if
> > an IP_ADDRESS was found.. using the loop, we would do that only if "addr"
> > had value.
> > 
> > 
> > Anyway...
> > I realized that we may not want this at all. I would just remove that
> > snippet, as the GATEWAY should be read by read_full_ip4_address():
> >        if (!read_full_ip4_address (ifcfg, i, NULL, &addr, &gateway, error))
> > 
> > The only different behavior we could have is when in method == SHARED, we
> > have IPADDR empty, but an IPADDR$X is defined as well as GATEWAY. In this
> > case we will miss the GATEWAY.
> > Moreover we will also catch GATEWAY$X if associated to IPADDR$X now (we
> > missed it before).
> > 
> > If we want to strictly follow behavior as it used to be for the above cases,
> > I would be for moving the check of method == SHARED outside the loop, as
> > suggested. But as I think we don't want, proposing the updated branch:
> > fg/dhcp-hostname_persist-rh1519299
> 
> if (strcmp (method, NM_SETTING_IP4_CONFIG_METHOD_DISABLED) == 0)
> 
> Please let's use nm_streq() or NM_IN_STRSET()
> 
> 
> 
> Anyway, NMSettingIP4Config's verify() has code like 
> 
> 
> »···} else if (   !strcmp (method, NM_SETTING_IP4_CONFIG_METHOD_LINK_LOCAL)
> »···           || !strcmp (method, NM_SETTING_IP4_CONFIG_METHOD_SHARED)
>     ....
> »···»···if (nm_setting_ip_config_get_num_dns (s_ip) > 0) {
> »···»···»···g_set_error (error,
> »···»···»···             NM_CONNECTION_ERROR,
> »···»···»···             NM_CONNECTION_ERROR_INVALID_PROPERTY,
> »···»···»···             _("this property is not allowed for '%s=%s'"),
> »···»···»···             NM_SETTING_IP_CONFIG_METHOD, method);
> »···»···»···g_prefix_error (error, "%s.%s: ",
> NM_SETTING_IP4_CONFIG_SETTING_NAME, NM_SETTING_IP_CONFIG_DNS);
> »···»···»···return FALSE;
> »···»···}
> 
> 
> that means, a shared connection with DNS servers is rejected as invalid.
> Hence, if a user has an ifcfg file on disc with method=shared *and* DNS
> servers, previously the DNS servers would have been silently ignored, now it
> is rejected with error.
> That might be fine though, since writer always only writes connections that
> verify(). Hence, writer supposedly never wrote such a connection (which
> would need investigation and confirmation). So, at worst, the user edited
> such files by hand and they will stop working because we now declare them as
> invalid.
> That's still kinda ugly. Can we avoid rejecting files that we accepted
> previously?

My aim was to be coherent in dealing with properties unused in current ipv4.method: write them all. Not happy to be incoherent here, saving just a part of them... but we don't want to break running config on updates, agreed.
Also moved back in place the NM_SETTING_IP4_CONFIG_METHOD_DISABLED check, skipping the dhcp properties from reading (we don't write them in first place as instead we do for the SHARED method).

Updated branch fg/dhcp-hostname_persist-rh1519299

(In reply to Francesco Giudici from comment #6)
> (In reply to Thomas Haller from comment #5)

> > That's still kinda ugly. Can we avoid rejecting files that we accepted
> > previously?
> 
> My aim was to be coherent in dealing with properties unused in current
> ipv4.method: write them all. Not happy to be incoherent here, saving just a
> part of them... but we don't want to break running config on updates, agreed.
> Also moved back in place the NM_SETTING_IP4_CONFIG_METHOD_DISABLED check,
> skipping the dhcp properties from reading (we don't write them in first
> place as instead we do for the SHARED method).
> 
> Updated branch fg/dhcp-hostname_persist-rh1519299

nm-setting-ip4-config.c:verify() only rejects dns and dns-searches for "shared" method. All other settings are valid as far as libnm is concerned (regardless of whether NM actually makes use of them with "shared" method or not).

Properties like NM_SETTING_IP_CONFIG_DNS_PRIORITY are not forbidden by libnm, so a user can set them together with shared method, and then ifcfg-rh reader will fail to read the same setting back. One question is whether (in this example) DNS-PRIORITY makes sense or nm_connection_normalize() should get rid of it. But as far as ifcfg-reader/writer are concerned, they should read and write every possible libnm configuration (of a verified, normalized connection).

So, better not shortcut everything, just skip DNS and DNS-SEARCHES.

(In reply to Thomas Haller from comment #7)
> (In reply to Francesco Giudici from comment #6)
> [...]
> 
> nm-setting-ip4-config.c:verify() only rejects dns and dns-searches for
> "shared" method. All other settings are valid as far as libnm is concerned
> (regardless of whether NM actually makes use of them with "shared" method or
> not).
> 
> Properties like NM_SETTING_IP_CONFIG_DNS_PRIORITY are not forbidden by
> libnm, so a user can set them together with shared method, and then ifcfg-rh
> reader will fail to read the same setting back. One question is whether (in
> this example) DNS-PRIORITY makes sense or nm_connection_normalize() should
> get rid of it. But as far as ifcfg-reader/writer are concerned, they should
> read and write every possible libnm configuration (of a verified, normalized
> connection).
> 
> So, better not shortcut everything, just skip DNS and DNS-SEARCHES.

Well, I'm not convinced 100%: I think we should enforce writing all the config... yes, some mangled configuration files would then be dropped. But, hey, they are wrong after all, also if we did not detected them in the past (we just silently ignored the values there). Skipping "dns" and "dns-search" but not "dns-priority" looks pretty weird to me.
Anyway, as this is an unlikely corner case, let's stay on the "safe" side, and avoid reading "dns" and "dns-searches" only.

Branch fg/dhcp-hostname_persist-rh1519299 updated and rebased.

The point is, nm_connection_verifiy() is the guideline what constitutes a valid connection and what not. Reader/Writer must persist and load every valid connection.

If you dislike the behaviour of verify(), then fix it in verify() Since this is already existing API, just tightening up verify() could break existing connections. For that, there is nm_connection_normalize(), to silently rectify a connection. If you don't like that either, then the issue cannot be fixed, and a connection can have unused/inconsistent values.

If you dislike the inconsistency, you could also relax verify() to allow DNS and DNS_SEARCH properties. Then it's more relaxed, but at least consistently so.


> Branch fg/dhcp-hostname_persist-rh1519299 updated and rebased.

lgtm

Yeah, I would like to get rid of the inconsistency... but don't relax the checks on verify, instead tighten them... without breaking existing configurations.
Yes, I basically want to eat my cake and have it, too :-)

So, ok, let us be on the safe side preserving working connections (and current checks).

> lgtm
thanks!

Patch merged on master:
https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=ff1884a219

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2018:3207