ifcfg: merge branch 'th/ifcfg-warn-invalid'

https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/925

src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c

@@ -2154,6 +2154,8 @@ read_aliases(NMSettingIPConfig *s_ip4, gboolean read_defroute, const char *filen
                 continue;
             }
 
+            svWarnInvalid(parsed, "alias", _NMLOG_DOMAIN);
+
             device = svGetValueStr(parsed, "DEVICE", &device_value);
             if (!device) {
                 PARSE_WARNING("alias file '%s' has no DEVICE", full_path);
@@ -6302,6 +6304,7 @@ connection_from_file_full(const char *filename,
     NMSetting *                   s_ip4;
     NMSetting *                   s_ip6;
     const char *                  ifcfg_name       = NULL;
+    gs_free char *                s_tmp            = NULL;
     gboolean                      has_ip4_defroute = FALSE;
     gboolean                      has_complex_routes_v4;
     gboolean                      has_complex_routes_v6;
@@ -6329,8 +6332,6 @@ connection_from_file_full(const char *filename,
     if (!main_ifcfg)
         return NULL;
 
-    network_ifcfg = svOpenFile(network_file, NULL);
-
     if (!svGetValueBoolean(main_ifcfg, "NM_CONTROLLED", TRUE)) {
         connection = create_unhandled_connection(filename, main_ifcfg, "unmanaged", out_unhandled);
         if (!connection) {
@@ -6344,6 +6345,16 @@ connection_from_file_full(const char *filename,
         return g_steal_pointer(&connection);
     }
 
+    if (NM_IN_STRSET(svGetValueStr(main_ifcfg, "DEVICE", &s_tmp), "lo")) {
+        /* "lo" is not handled by NetworkManager and we ignore it. */
+    } else
+        svWarnInvalid(main_ifcfg, "ifcfg", _NMLOG_DOMAIN);
+    nm_clear_g_free(&s_tmp);
+
+    network_ifcfg = svOpenFile(network_file, NULL);
+    /* we don't call svWarnInvalid(network_ifcfg), because we will load this file for
+     * every profile. So we would get a large number of duplicate warnings. */
+
     /* iBFT is handled by nm-initrd-generator during boot. */
     bootproto = svGetValueStr_cp(main_ifcfg, "BOOTPROTO");
     if (bootproto && !g_ascii_strcasecmp(bootproto, "ibft")) {

src/core/settings/plugins/ifcfg-rh/shvar.c

@@ -255,20 +255,36 @@ svEscape(const char *s, char **to_free)
     gsize    slen;
     gsize    i;
     gsize    j;
+    gboolean all_ascii = TRUE;
 
     for (slen = 0; s[slen]; slen++) {
         if (_char_req_escape(s[slen]))
             mangle++;
         else if (_char_req_quotes(s[slen]))
             requires_quotes = TRUE;
-        else if (s[slen] < ' ') {
+        else if (((guchar) s[slen]) < ' ') {
             /* if the string contains newline we can only express it using ANSI C quotation
              * (as we don't support line continuation).
              * Additionally, ANSI control characters look odd with regular quotation, so handle
              * them too. */
             return (*to_free = _escape_ansic(s));
+        } else if (((guchar) s[slen]) >= 0177) {
+            all_ascii       = FALSE;
+            requires_quotes = TRUE;
         }
     }
+
+    if (!all_ascii && !g_utf8_validate(s, -1, NULL)) {
+        /* The string is not valid ASCII/UTF-8. We can escape that via
+         * _escape_ansic(), however the reader might have a problem to
+         * do something sensible with the blob later.
+         *
+         * This is really a bug of the caller, which should not present us with
+         * non-text in the first place. But at this place, we cannot handle the
+         * error better, so just escape it. */
+        return (*to_free = _escape_ansic(s));
+    }
+
     if (!mangle && !requires_quotes) {
         *to_free = NULL;
         return s;
@@ -371,6 +387,12 @@ _strbuf_init(NMStrBuf *str, const char *value, gsize i)
 
 const char *
 svUnescape(const char *value, char **to_free)
+{
+    return svUnescape_full(value, to_free, TRUE);
+}
+
+const char *
+svUnescape_full(const char *value, char **to_free, gboolean check_utf8)
 {
     NMStrBuf str                      = NM_STR_BUF_INIT(0, FALSE);
     int      looks_like_old_svescaped = -1;
@@ -646,6 +668,8 @@ out_value:
     }
 
     if (str.allocated > 0) {
+        if (check_utf8 && !nm_str_buf_utf8_validate(&str))
+            goto out_error;
         if (str.len == 0 || nm_str_buf_get_str_unsafe(&str)[0] == '\0') {
             nm_str_buf_destroy(&str);
             *to_free = NULL;
@@ -656,6 +680,11 @@ out_value:
         }
     }
 
+    if (check_utf8 && !g_utf8_validate(value, i, NULL)) {
+        *to_free = NULL;
+        return NULL;
+    }
+
     if (value[i] != '\0') {
         *to_free = g_strndup(value, i);
         return *to_free;
@@ -1120,9 +1149,8 @@ _svGetValue(shvarFile *s, const char *key, char **to_free)
     if (line && line->line) {
         v = svUnescape(line->line, to_free);
         if (!v) {
-            /* a wrongly quoted value is treated like the empty string.
-             * See also svWriteFile(), which handles unparsable values
-             * that way. */
+            /* a wrongly quoted value or non-UTF-8 is treated like the empty string.
+             * See also svWriteFile(), which handles unparsable values that way. */
             nm_assert(!*to_free);
             return "";
         }
@@ -1495,6 +1523,91 @@ svUnsetValue(shvarFile *s, const char *key)
 
 /*****************************************************************************/
 
+void
+svWarnInvalid(shvarFile *s, const char *file_type, NMLogDomain log_domain)
+{
+    shvarLine *line;
+    gsize      n;
+
+    if (!nm_logging_enabled(LOGL_WARN, log_domain))
+        return;
+
+    n = 0;
+    c_list_for_each_entry (line, &s->lst_head, lst) {
+        gs_free char *s_tmp = NULL;
+
+        n++;
+
+        if (!line->key) {
+            const char *str;
+
+            nm_assert(line->line);
+            str = nm_str_skip_leading_spaces(line->line);
+            if (!NM_IN_SET(str[0], '\0', '#')) {
+                nm_log_warn(log_domain,
+                            "ifcfg-rh: %s,%s:%zu: invalid line ignored",
+                            file_type,
+                            s->fileName,
+                            n);
+            }
+            continue;
+        }
+
+        if (g_hash_table_lookup(s->lst_idx, line) != line) {
+            nm_log_warn(
+                log_domain,
+                "ifcfg-rh: %s,%s:%zu: key %s is duplicated and the early occurrence ignored",
+                file_type,
+                s->fileName,
+                n,
+                line->key);
+            continue;
+        }
+
+        if (!line->line) {
+            /* the line is deleted via svUnsetValue(). Ignore. */
+            continue;
+        }
+
+        if (!svUnescape(line->line, &s_tmp)) {
+            if (!svUnescape_full(line->line, &s_tmp, FALSE)) {
+                nm_log_warn(log_domain,
+                            "ifcfg-rh: %s,%s:%zu: key %s is badly quoted and is treated as \"\"",
+                            file_type,
+                            s->fileName,
+                            n,
+                            line->key);
+            } else {
+                nm_log_warn(log_domain,
+                            "ifcfg-rh: %s,%s:%zu: key %s does not contain valid UTF-8 and is "
+                            "treated as \"\"",
+                            file_type,
+                            s->fileName,
+                            n,
+                            line->key);
+            }
+            continue;
+        }
+
+        /* TODO: we read different shell scripts, and whether a key is recognized
+         * depends on the type. For example, alias files only accept a subset of
+         * known keys.
+         *
+         * Basically, depending on the @file_type, different keys are valid. */
+        if (!nms_ifcfg_rh_utils_is_well_known_key(line->key)) {
+            nm_log_dbg(log_domain,
+                       "ifcfg-rh: %s,%s:%zu: key %s is unknown and ignored",
+                       file_type,
+                       s->fileName,
+                       n,
+                       line->key);
+            continue;
+        }
+    }
+}
+
+/*****************************************************************************/
+
 /* Write the current contents iff modified.  Returns FALSE on error
  * and TRUE on success.  Do not write if no values have been modified.
  * The mode argument is only used if creating the file, not if

src/core/settings/plugins/ifcfg-rh/shvar.h

@@ -107,6 +107,7 @@ void svCloseFile(shvarFile *s);
 
 const char *svEscape(const char *s, char **to_free);
 const char *svUnescape(const char *s, char **to_free);
+const char *svUnescape_full(const char *value, char **to_free, gboolean check_utf8);
 
 static inline void
 _nm_auto_shvar_file_close(shvarFile **p_s)
@@ -120,4 +121,6 @@ _nm_auto_shvar_file_close(shvarFile **p_s)
 }
 #define nm_auto_shvar_file_close nm_auto(_nm_auto_shvar_file_close)
 
+void svWarnInvalid(shvarFile *s, const char *file_type, NMLogDomain log_domain);
+
 #endif /* _SHVAR_H */

src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wifi-wpa-eap-suite-b-192-tls

@@ -3,7 +3,6 @@ DEVICE=eth2
 HWADDR=00:16:41:11:22:33
 BOOTPROTO=dhcp
 ONBOOT=yes
-ONBOOT=yes
 USERCTL=yes
 IPV6INIT=no
 NM_CONTROLLED=yes

src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-wifi-wpa-eap-ttls-tls

@@ -4,7 +4,6 @@ DEVICE=eth2
 HWADDR=00:16:41:11:22:33
 BOOTPROTO=dhcp
 ONBOOT=yes
-ONBOOT=yes
 USERCTL=yes
 IPV6INIT=no
 NM_CONTROLLED=yes

src/core/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-write-unknown-4.expected

@@ -13,8 +13,10 @@
 #L2
 
 METRIC1=''
-METRIC2=$'\U0x'
-METRIC3=$'x\U0'
+METRIC2=
+#NM: METRIC2=$'\U0x'
+METRIC3=
+#NM: METRIC3=$'x\U0'
 
 #L4
 IPADDR=set-by-test1

src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c

@@ -738,10 +738,13 @@ test_read_variables_corner_cases(void)
     const char *         mac;
     char                 expected_mac_address[ETH_ALEN] = {0x00, 0x16, 0x41, 0x11, 0x22, 0x33};
 
+    NMTST_EXPECT_NM_WARN("*key NAME is badly quoted and is treated as \"\"*");
+    NMTST_EXPECT_NM_WARN("*key ZONE is badly quoted and is treated as \"\"*");
     connection = _connection_from_file(TEST_IFCFG_DIR "/ifcfg-test-variables-corner-cases-1",
                                        NULL,
                                        TYPE_ETHERNET,
                                        NULL);
+    g_test_assert_expected_messages();
 
     /* ===== CONNECTION SETTING ===== */
     s_con = nm_connection_get_setting_connection(connection);
@@ -830,10 +833,12 @@ test_read_unrecognized(void)
     gs_free char *       unhandled_spec     = NULL;
     guint64              expected_timestamp = 0;
 
+    NMTST_EXPECT_NM_WARN("*key NAME is badly quoted and is treated as \"\"*");
     connection = _connection_from_file(TEST_IFCFG_DIR "/ifcfg-test-unrecognized",
                                        NULL,
                                        NULL,
                                        &unhandled_spec);
+    g_test_assert_expected_messages();
     g_assert_cmpstr(unhandled_spec, ==, "unrecognized:mac:00:11:22:33");
 
     /* ===== CONNECTION SETTING ===== */
@@ -1004,10 +1009,12 @@ test_read_wired_dhcp(void)
     char                 expected_mac_address[ETH_ALEN] = {0x00, 0x11, 0x22, 0x33, 0x44, 0xee};
     const char *         mac;
 
+    NMTST_EXPECT_NM_WARN("*key IPV6INIT is duplicated and the early occurrence ignored*");
     connection = _connection_from_file(TEST_IFCFG_DIR "/ifcfg-test-wired-dhcp",
                                        NULL,
                                        TYPE_ETHERNET,
                                        &unmanaged);
+    g_test_assert_expected_messages();
     g_assert(unmanaged == NULL);
 
     /* ===== CONNECTION SETTING ===== */
@@ -3583,10 +3590,12 @@ test_read_wifi_wpa_eap_tls(void)
     char *             unmanaged                 = NULL;
     const char *       expected_privkey_password = "test1";
 
+    NMTST_EXPECT_NM_WARN("*key ONBOOT is duplicated and the early occurrence ignored*");
     connection = _connection_from_file(TEST_IFCFG_DIR "/ifcfg-test-wifi-wpa-eap-tls",
                                        NULL,
                                        TYPE_ETHERNET,
                                        &unmanaged);
+    g_test_assert_expected_messages();
     g_assert(!unmanaged);
 
     /* ===== WIRELESS SETTING ===== */
@@ -3791,10 +3800,12 @@ test_read_wifi_wep_eap_ttls_chap(void)
     NMSetting8021x *           s_8021x;
     char *                     unmanaged = NULL;
 
+    NMTST_EXPECT_NM_WARN("*key ONBOOT is duplicated and the early occurrence ignored*");
     connection = _connection_from_file(TEST_IFCFG_DIR "/ifcfg-test-wifi-wep-eap-ttls-chap",
                                        NULL,
                                        TYPE_WIRELESS,
                                        &unmanaged);
+    g_test_assert_expected_messages();
     g_assert(!unmanaged);
 
     /* ===== WIRELESS SETTING ===== */
@@ -10476,7 +10487,7 @@ _svUnescape(const char *str, char **to_free)
         str = (str_free = g_strdup(str));
     }
 
-    s = svUnescape(str, to_free);
+    s = svUnescape_full(str, to_free, FALSE);
     if (*to_free) {
         g_assert(s == *to_free);
         g_assert(s[0]);
@@ -10484,6 +10495,37 @@ _svUnescape(const char *str, char **to_free)
         g_assert(s == NULL || (!s[0] && (s < str || s > strchr(str, '\0')))
                  || (s[0] && s >= str && s <= strchr(str, '\0')));
     }
+
+    {
+        const char *  s2;
+        gs_free char *to_free2 = NULL;
+
+        gboolean is_utf8 = s && g_utf8_validate(s, -1, NULL);
+
+        s2 = svUnescape_full(str, &to_free2, TRUE);
+        if (NM_IN_STRSET(str, "$'\\U0x'", "$'\\x0'", "$'\\008'", "$'\\08'")) {
+            g_assert_cmpstr(s2, ==, NULL);
+            g_assert(!to_free2);
+            g_assert_cmpstr(s, ==, "");
+            g_assert(!*to_free);
+        } else if (NM_IN_STRSET(str, "$'x\\U0'")) {
+            g_assert_cmpstr(s2, ==, NULL);
+            g_assert(!to_free2);
+            g_assert_cmpstr(s, ==, "x");
+            g_assert(*to_free == s);
+        } else if (!is_utf8) {
+            g_assert(!s2);
+            g_assert(!to_free2);
+        } else if (!to_free2) {
+            g_assert_cmpstr(s, ==, s2);
+            g_assert(s == s2);
+        } else {
+            g_assert_cmpstr(s, ==, s2);
+            g_assert(s != s2);
+            g_assert(s2 == to_free2);
+        }
+    }
+
     return s;
 }
 
@@ -10665,6 +10707,9 @@ test_svUnescape(void)
         V1("\"\\'\"''", "\\'"),
         V0("\"b\\~b\" ", "b\\~b"),
         V1("\"b\\~b\"x", "b\\~bx"),
+
+        V0("$'x\\U0'", "x"),
+        V0("$'\\U0x'", ""),
     };
     const UnescapeTestData data_ansi[] = {
         /* strings inside $''. They cannot be compared directly, but must
@@ -10851,7 +10896,7 @@ test_write_unknown(gconstpointer test_data)
         _svGetValue_check(sv, "METRIC", NULL);
         _svGetValue_check(sv, "METRIC1", "");
         _svGetValue_check(sv, "METRIC2", "");
-        _svGetValue_check(sv, "METRIC3", "x");
+        _svGetValue_check(sv, "METRIC3", "");
 
         _svGetValue_check(sv, "IPADDR", "set-by-test1");
         _svGetValue_check(sv, "IPADDR2", "set-by-test2");

src/libnm-glib-aux/nm-str-buf.h

@@ -504,4 +504,11 @@ nm_str_buf_destroy(NMStrBuf *strbuf)
 
 #define nm_auto_str_buf nm_auto(nm_str_buf_destroy)
 
+static inline gboolean
+nm_str_buf_utf8_validate(NMStrBuf *strbuf)
+{
+    _nm_str_buf_assert(strbuf);
+    return strbuf->_priv_len == 0 || g_utf8_validate(strbuf->_priv_str, strbuf->_priv_len, NULL);
+}
+
 #endif /* __NM_STR_BUF_H__ */