libnm: fix inconsistencies and assertions of "ipv[46].dns" handing

- nm_setting_ip_config_add_dns() and nm_setting_ip_config_remove_dns_by_value()
  used to assert that the provided input is valid. That is not
  documented and highly problematic.
  Our parsing code for keyfile, ifcfg-rh and GVariant rightly just call
  add. Likewise, nmcli. We cannot reasonably expect them to pre-validate
  the input. Why would we anyway?
  This is wrong in particular because we usually want the user to be
  able to construct invalid settings. That is often necessary, because
  whether a value is valid depends on other values. So in general, we
  can only validate when all properties are set. We have
  nm_connection_verify() for that, and asserting/validating during add
  is very wrong. Note that "add" still filters out duplicates, which
  may be an inconsistency, but well.
  Also, the user could set any bogus value via the NM_SETTING_IP_CONFIG_DNS
  property. Those should be allowed to be removed, and the same values
  should be allowed to be added via the add method.

- add() does a normalization, presumably so that the values look nice.
  Do the same normalization also when using the NM_SETTING_IP_CONFIG_DNS
  property setter.

- previously, the setter could also set unnormalized values. As
  nm_setting_ip_config_remove_dns_by_value() looked for the normalized
  value, you couldn't remove such values anymore. That is fixed now,
  by letting the property setter do the same normalization.

- don't allocate a GPtrArray unless we need it. No need for the extra
  allocation.

- in the property setter, first set the new value before destroying the
  previous GPtrArray. It might not be possible, but it's not clear to me
  whether the strv argument from the GValue is always deep-copied or
  whether it could contain strings to the DNS property itself.

4 files changed, 79 insertions, 65 deletions

diff --git a/src/libnm-core-impl/nm-setting-ip-config.c b/src/libnm-core-impl/nm-setting-ip-config.c
index 7db55c2da1..e2bcebf2d5 100644
--- a/src/libnm-core-impl/nm-setting-ip-config.c
+++ b/src/libnm-core-impl/nm-setting-ip-config.c
@@ -4014,7 +4014,7 @@ nm_setting_ip_config_get_num_dns(NMSettingIPConfig *setting)
 {
     g_return_val_if_fail(NM_IS_SETTING_IP_CONFIG(setting), 0);
 
-    return NM_SETTING_IP_CONFIG_GET_PRIVATE(setting)->dns->len;
+    return nm_g_ptr_array_len(NM_SETTING_IP_CONFIG_GET_PRIVATE(setting)->dns);
 }
 
 /**
@@ -4032,11 +4032,34 @@ nm_setting_ip_config_get_dns(NMSettingIPConfig *setting, int idx)
     g_return_val_if_fail(NM_IS_SETTING_IP_CONFIG(setting), NULL);
 
     priv = NM_SETTING_IP_CONFIG_GET_PRIVATE(setting);
-    g_return_val_if_fail(idx >= 0 && idx < priv->dns->len, NULL);
+    g_return_val_if_fail(idx >= 0 && ((guint) idx) < nm_g_ptr_array_len(priv->dns), NULL);
 
     return priv->dns->pdata[idx];
 }
 
+static gboolean
+_ip_config_add_dns(NMSettingIPConfig *setting, const char *dns)
+{
+    NMSettingIPConfigPrivate *priv;
+    const int                 addr_family = NM_SETTING_IP_CONFIG_GET_ADDR_FAMILY(setting);
+    NMIPAddr                  dns_bin;
+    char                      dns_canonical[NM_INET_ADDRSTRLEN];
+
+    nm_assert(NM_IS_SETTING_IP_CONFIG(setting));
+    nm_assert(dns);
+
+    priv = NM_SETTING_IP_CONFIG_GET_PRIVATE(setting);
+
+    if (valid_ip(addr_family, dns, &dns_bin, NULL))
+        dns = nm_inet_ntop(addr_family, &dns_bin, dns_canonical);
+
+    if (nm_strv_ptrarray_contains(priv->dns, dns))
+        return FALSE;
+
+    nm_strv_ptrarray_add_string_dup(nm_strv_ptrarray_ensure(&priv->dns), dns);
+    return TRUE;
+}
+
 /**
  * nm_setting_ip_config_add_dns:
  * @setting: the #NMSettingIPConfig
@@ -4046,36 +4069,20 @@ nm_setting_ip_config_get_dns(NMSettingIPConfig *setting, int idx)
  *
  * Returns: %TRUE if the DNS server was added; %FALSE if the server was already
  * known
+ *
+ * Before 1.42, setting @dns to an invalid string was treated as user-error.
+ * Now, also invalid DNS values can be set, but will be rejected later during
+ * nm_connection_verify().
  **/
 gboolean
 nm_setting_ip_config_add_dns(NMSettingIPConfig *setting, const char *dns)
 {
-    NMSettingIPConfigPrivate *priv;
-    int                       addr_family;
-    NMIPAddr                  dns_bin;
-    char                      dns_canonical[NM_INET_ADDRSTRLEN];
-    guint                     i;
-
     g_return_val_if_fail(NM_IS_SETTING_IP_CONFIG(setting), FALSE);
+    g_return_val_if_fail(dns, FALSE);
 
-    addr_family = NM_SETTING_IP_CONFIG_GET_ADDR_FAMILY(setting);
-
-    if (!valid_ip(addr_family, dns, &dns_bin, NULL)) {
-        g_return_val_if_fail(dns != NULL, FALSE);
-        g_return_val_if_fail(nm_inet_is_valid(addr_family, dns), FALSE);
-        nm_assert_not_reached();
-    }
-
-    priv = NM_SETTING_IP_CONFIG_GET_PRIVATE(setting);
-
-    nm_inet_ntop(addr_family, &dns_bin, dns_canonical);
-
-    for (i = 0; i < priv->dns->len; i++) {
-        if (nm_streq(dns_canonical, priv->dns->pdata[i]))
-            return FALSE;
-    }
+    if (!_ip_config_add_dns(setting, dns))
+        return FALSE;
 
-    g_ptr_array_add(priv->dns, g_strdup(dns_canonical));
     _notify(setting, PROP_DNS);
     return TRUE;
 }
@@ -4095,7 +4102,8 @@ nm_setting_ip_config_remove_dns(NMSettingIPConfig *setting, int idx)
     g_return_if_fail(NM_IS_SETTING_IP_CONFIG(setting));
 
     priv = NM_SETTING_IP_CONFIG_GET_PRIVATE(setting);
-    g_return_if_fail(idx >= 0 && idx < priv->dns->len);
+
+    g_return_if_fail(idx >= 0 && ((guint) idx) < nm_g_ptr_array_len(priv->dns));
 
     g_ptr_array_remove_index(priv->dns, idx);
     _notify(setting, PROP_DNS);
@@ -4109,6 +4117,8 @@ nm_setting_ip_config_remove_dns(NMSettingIPConfig *setting, int idx)
  * Removes the DNS server @dns.
  *
  * Returns: %TRUE if the DNS server was found and removed; %FALSE if it was not.
+ *
+ * Before 1.42, setting @dns to an invalid string was treated as user-error.
  **/
 gboolean
 nm_setting_ip_config_remove_dns_by_value(NMSettingIPConfig *setting, const char *dns)
@@ -4117,30 +4127,25 @@ nm_setting_ip_config_remove_dns_by_value(NMSettingIPConfig *setting, const char
     int                       addr_family;
     NMIPAddr                  dns_bin;
     char                      dns_canonical[NM_INET_ADDRSTRLEN];
-    guint                     i;
+    gssize                    idx;
 
     g_return_val_if_fail(NM_IS_SETTING_IP_CONFIG(setting), FALSE);
+    g_return_val_if_fail(dns, FALSE);
+
+    priv = NM_SETTING_IP_CONFIG_GET_PRIVATE(setting);
 
     addr_family = NM_SETTING_IP_CONFIG_GET_ADDR_FAMILY(setting);
 
-    if (!valid_ip(addr_family, dns, &dns_bin, NULL)) {
-        g_return_val_if_fail(dns != NULL, FALSE);
-        g_return_val_if_fail(nm_inet_is_valid(addr_family, dns), FALSE);
-        nm_assert_not_reached();
-    }
+    if (valid_ip(addr_family, dns, &dns_bin, NULL))
+        dns = nm_inet_ntop(addr_family, &dns_bin, dns_canonical);
 
-    priv = NM_SETTING_IP_CONFIG_GET_PRIVATE(setting);
-
-    nm_inet_ntop(addr_family, &dns_bin, dns_canonical);
+    idx = nm_strv_ptrarray_find_first(priv->dns, dns);
+    if (idx < 0)
+        return FALSE;
 
-    for (i = 0; i < priv->dns->len; i++) {
-        if (nm_streq(dns_canonical, priv->dns->pdata[i])) {
-            g_ptr_array_remove_index(priv->dns, i);
-            _notify(setting, PROP_DNS);
-            return TRUE;
-        }
-    }
-    return FALSE;
+    g_ptr_array_remove_index(nm_strv_ptrarray_ensure(&priv->dns), idx);
+    _notify(setting, PROP_DNS);
+    return TRUE;
 }
 
 /**
@@ -4158,7 +4163,7 @@ nm_setting_ip_config_clear_dns(NMSettingIPConfig *setting)
 
     priv = NM_SETTING_IP_CONFIG_GET_PRIVATE(setting);
 
-    if (priv->dns->len != 0) {
+    if (nm_g_ptr_array_len(priv->dns) != 0) {
         g_ptr_array_set_size(priv->dns, 0);
         _notify(setting, PROP_DNS);
     }
@@ -5433,20 +5438,22 @@ verify(NMSetting *setting, NMConnection *connection, GError **error)
     }
 
     /* Validate DNS */
-    for (i = 0; i < priv->dns->len; i++) {
-        const char *dns = priv->dns->pdata[i];
+    if (priv->dns) {
+        for (i = 0; i < priv->dns->len; i++) {
+            const char *dns = priv->dns->pdata[i];
 
-        if (!nm_inet_is_valid(NM_SETTING_IP_CONFIG_GET_ADDR_FAMILY(setting), dns)) {
-            g_set_error(error,
-                        NM_CONNECTION_ERROR,
-                        NM_CONNECTION_ERROR_INVALID_PROPERTY,
-                        _("%d. DNS server address is invalid"),
-                        (int) (i + 1));
-            g_prefix_error(error,
-                           "%s.%s: ",
-                           nm_setting_get_name(setting),
-                           NM_SETTING_IP_CONFIG_DNS);
-            return FALSE;
+            if (!nm_inet_is_valid(NM_SETTING_IP_CONFIG_GET_ADDR_FAMILY(setting), dns)) {
+                g_set_error(error,
+                            NM_CONNECTION_ERROR,
+                            NM_CONNECTION_ERROR_INVALID_PROPERTY,
+                            _("%d. DNS server address is invalid"),
+                            (int) (i + 1));
+                g_prefix_error(error,
+                               "%s.%s: ",
+                               nm_setting_get_name(setting),
+                               NM_SETTING_IP_CONFIG_DNS);
+                return FALSE;
+            }
         }
     }
 
@@ -6068,9 +6075,17 @@ set_property(GObject *object, guint prop_id, const GValue *value, GParamSpec *ps
 
     switch (prop_id) {
     case PROP_DNS:
-        g_ptr_array_unref(priv->dns);
-        priv->dns = nm_strv_to_ptrarray(g_value_get_boxed(value));
+    {
+        gs_unref_ptrarray GPtrArray *dns_old = NULL;
+
+        dns_old = g_steal_pointer(&priv->dns);
+        strv    = g_value_get_boxed(value);
+        if (strv) {
+            for (i = 0; strv[i]; i++)
+                _ip_config_add_dns(setting, strv[i]);
+        }
         break;
+    }
     case PROP_DNS_SEARCH:
         g_ptr_array_unref(priv->dns_search);
         priv->dns_search = nm_strv_to_ptrarray(g_value_get_boxed(value));
@@ -6122,7 +6137,6 @@ _nm_setting_ip_config_private_init(gpointer self, NMSettingIPConfigPrivate *priv
 {
     nm_assert(NM_IS_SETTING_IP_CONFIG(self));
 
-    priv->dns        = g_ptr_array_new_with_free_func(g_free);
     priv->dns_search = g_ptr_array_new_with_free_func(g_free);
     priv->addresses  = g_ptr_array_new_with_free_func((GDestroyNotify) nm_ip_address_unref);
     priv->routes     = g_ptr_array_new_with_free_func((GDestroyNotify) nm_ip_route_unref);
@@ -6140,7 +6154,7 @@ finalize(GObject *object)
     NMSettingIPConfig        *self = NM_SETTING_IP_CONFIG(object);
     NMSettingIPConfigPrivate *priv = NM_SETTING_IP_CONFIG_GET_PRIVATE(self);
 
-    g_ptr_array_unref(priv->dns);
+    nm_g_ptr_array_unref(priv->dns);
     g_ptr_array_unref(priv->dns_search);
     nm_g_ptr_array_unref(priv->dns_options);
     g_ptr_array_unref(priv->addresses);
diff --git a/src/libnm-core-impl/nm-setting-ip4-config.c b/src/libnm-core-impl/nm-setting-ip4-config.c
index 3649df3f90..230425347d 100644
--- a/src/libnm-core-impl/nm-setting-ip4-config.c
+++ b/src/libnm-core-impl/nm-setting-ip4-config.c
@@ -388,7 +388,6 @@ ip4_dns_to_dbus(_NM_SETT_INFO_PROP_TO_DBUS_FCN_ARGS _nm_nil)
     GPtrArray *dns;
 
     dns = _nm_setting_ip_config_get_dns_array(NM_SETTING_IP_CONFIG(setting));
-
     if (nm_g_ptr_array_len(dns) == 0)
         return NULL;
 
diff --git a/src/libnm-core-impl/nm-setting-ip6-config.c b/src/libnm-core-impl/nm-setting-ip6-config.c
index 6a08552c7e..a3f4d0c9dc 100644
--- a/src/libnm-core-impl/nm-setting-ip6-config.c
+++ b/src/libnm-core-impl/nm-setting-ip6-config.c
@@ -389,7 +389,6 @@ ip6_dns_to_dbus(_NM_SETT_INFO_PROP_TO_DBUS_FCN_ARGS _nm_nil)
     GPtrArray *dns;
 
     dns = _nm_setting_ip_config_get_dns_array(NM_SETTING_IP_CONFIG(setting));
-
     if (nm_g_ptr_array_len(dns) == 0)
         return NULL;
 
diff --git a/src/libnm-core-impl/tests/test-general.c b/src/libnm-core-impl/tests/test-general.c
index a45ead3b9f..d38a26471a 100644
--- a/src/libnm-core-impl/tests/test-general.c
+++ b/src/libnm-core-impl/tests/test-general.c
@@ -5247,7 +5247,8 @@ test_setting_ip4_changed_signal(void)
     ASSERT_CHANGED(nm_setting_ip_config_add_dns(s_ip4, "11.22.0.0"));
     ASSERT_CHANGED(nm_setting_ip_config_remove_dns(s_ip4, 0));
 
-    NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(idx >= 0 && idx < priv->dns->len));
+    NMTST_EXPECT_LIBNM_CRITICAL(
+        NMTST_G_RETURN_MSG(idx >= 0 && ((guint) idx) < nm_g_ptr_array_len(priv->dns)));
     ASSERT_UNCHANGED(nm_setting_ip_config_remove_dns(s_ip4, 1));
     g_test_assert_expected_messages();
 
@@ -5323,7 +5324,8 @@ test_setting_ip6_changed_signal(void)
     ASSERT_CHANGED(nm_setting_ip_config_add_dns(s_ip6, "1:2:3::4:5:6"));
     ASSERT_CHANGED(nm_setting_ip_config_remove_dns(s_ip6, 0));
 
-    NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(idx >= 0 && idx < priv->dns->len));
+    NMTST_EXPECT_LIBNM_CRITICAL(
+        NMTST_G_RETURN_MSG(idx >= 0 && ((guint) idx) < nm_g_ptr_array_len(priv->dns)));
     ASSERT_UNCHANGED(nm_setting_ip_config_remove_dns(s_ip6, 1));
     g_test_assert_expected_messages();