glib-aux: merge branch 'th/no-inet-aton'

See-also: https://bugs.python.org/issue37495
https://bugzilla.redhat.com/show_bug.cgi?id=2049134

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

4 files changed, 243 insertions, 40 deletions

diff --git a/src/libnm-glib-aux/nm-inet-utils.c b/src/libnm-glib-aux/nm-inet-utils.c
index 7f710f3527..87fe3ce050 100644
--- a/src/libnm-glib-aux/nm-inet-utils.c
+++ b/src/libnm-glib-aux/nm-inet-utils.c
@@ -6,6 +6,7 @@
 
 #include <netinet/in.h>
 #include <arpa/inet.h>
+#include <dlfcn.h>
 
 /*****************************************************************************/
 
@@ -266,34 +267,118 @@ nm_ip6_addr_same_prefix_cmp(const struct in6_addr *addr_a,
 
 /*****************************************************************************/
 
-static gboolean
-_parse_legacy_addr4(const char *text, in_addr_t *out_addr, GError **error)
+static int
+_inet_aton(const char *text, in_addr_t *out_addr)
 {
-    gs_free char  *s_free = NULL;
-    struct in_addr a1;
-    guint8         bin[sizeof(a1)];
-    char          *s;
-    int            i;
+    /* Call inet_aton() via dlopen.
+     *
+     * The inet_aton() API is discouraged, and ABI checkers warn when we call
+     * it.
+     *
+     * We want to use this function, but only for testing/asserting. To avoid
+     * the ABI checker's complain, dlopen() the symbol. This is not used for
+     * production.
+     */
+    static gpointer mod_handle  = NULL;
+    static gpointer fcn_sym     = NULL;
+    static gsize    initialized = 0;
+    int (*fcn)(const char *text, struct in_addr *out_addr);
+    int       r;
+    in_addr_t a;
+
+    if (g_once_init_enter(&initialized)) {
+        mod_handle = dlopen(NULL, RTLD_LAZY);
+        if (mod_handle) {
+            fcn_sym = dlsym(mod_handle, "inet_aton");
+            if (!fcn_sym)
+                dlclose(g_steal_pointer(&mod_handle));
+        }
+        g_once_init_leave(&initialized, 1);
+    }
 
-    if (inet_aton(text, &a1) != 1) {
-        g_set_error_literal(error,
-                            NM_UTILS_ERROR,
-                            NM_UTILS_ERROR_INVALID_ARGUMENT,
-                            "address invalid according to inet_aton()");
-        return FALSE;
+    if (!fcn_sym)
+        return -ENOSYS;
+
+    g_assert(mod_handle);
+
+    fcn = fcn_sym;
+    r   = fcn(text, (gpointer) &a);
+
+    if (r != 1)
+        return -EINVAL;
+
+    NM_SET_OUT(out_addr, a);
+    return 0;
+}
+
+int
+nmtst_inet_aton(const char *text, in_addr_t *out_addr)
+{
+    return _inet_aton(text, out_addr);
+}
+
+static void
+_nm_assert_legacy_addr4(const char *text, in_addr_t addr)
+{
+#if NM_MORE_ASSERTS > 20
+    char      buf1[NM_INET_ADDRSTRLEN];
+    char      buf2[NM_INET_ADDRSTRLEN];
+    int       r;
+    in_addr_t a;
+
+    /* Our legacy parser accepted "text" as "addr".
+     *
+     * However, we want to ensure that whatever we parse is also parsed by old
+     * inet_aton(). So we want to be strictly more strict than inet_aton() in
+     * what we accept.
+     */
+
+    r = _inet_aton(text, &a);
+
+    if (r != 0) {
+        if (r == -ENOSYS)
+            return;
+        g_error("inet_aton(\"%s\") failed with \"%s\", but we expected %s",
+                text,
+                nm_strerror_native(-r),
+                nm_inet4_ntop(addr, buf2));
     }
 
-    /* OK, inet_aton() accepted the format. That's good, because we want
-     * to accept IPv4 addresses in octal format, like 255.255.000.000.
-     * That's what "legacy" means here. inet_pton() doesn't accept those.
+    if (a != addr) {
+        g_error("inet_aton(\"%s\") parsed %s, but we expected %s",
+                text,
+                nm_inet4_ntop(a, buf1),
+                nm_inet4_ntop(addr, buf2));
+    }
+#endif
+}
+
+static gboolean
+_parse_legacy_addr4(const char *text, in_addr_t *out_addr, GError **error)
+{
+    gs_free char *s_free = NULL;
+    union {
+        guint8    b[sizeof(in_addr_t)];
+        in_addr_t a;
+    } addr;
+    char *s;
+    int   i;
+
+    /* inet_pton() does strict parsing of IPv4 address. Good.
      *
-     * But inet_aton() also ignores trailing garbage and formats with fewer than
-     * 4 digits. That is just too crazy and we don't do that. Perform additional checks
-     * and reject some forms that inet_aton() accepted.
+     * However, inet_aton() used to accept much more relaxed forms (e.g. octal
+     * and hex numbers, not having 4 components but fewer, ignore any trailing
+     * garbage).
+     *
+     * Some places where we accept input, we want to be slightly more forgiving
+     * than inet_pton() and accept some (not all!) forms of what inet_aton()
+     * would accept. For example, we want to accept 255.000.000.000.
+     *
+     * We reimplement that below.
      *
      * Note that we still should (of course) accept everything that inet_pton()
-     * accepts. However this code never gets called if inet_pton() succeeds
-     * (see below, aside the assertion code). */
+     * accepts. This is ensured because the caller only calls this function
+     * after inet_pton() failed. */
 
     if (NM_STRCHAR_ANY(text, ch, (!(ch >= '0' && ch <= '9') && !NM_IN_SET(ch, '.', 'x')))) {
         /* We only accepts '.', digits, and 'x' for "0x". */
@@ -306,7 +391,7 @@ _parse_legacy_addr4(const char *text, in_addr_t *out_addr, GError **error)
 
     s = nm_memdup_maybe_a(300, text, strlen(text) + 1, &s_free);
 
-    for (i = 0; i < G_N_ELEMENTS(bin); i++) {
+    for (i = 0; i < G_N_ELEMENTS(addr.b); i++) {
         char  *current_token = s;
         gint32 v;
 
@@ -316,7 +401,7 @@ _parse_legacy_addr4(const char *text, in_addr_t *out_addr, GError **error)
             s++;
         }
 
-        if ((i == G_N_ELEMENTS(bin) - 1) != (s == NULL)) {
+        if ((i == G_N_ELEMENTS(addr.b) - 1) != (s == NULL)) {
             /* Exactly for the last digit, we expect to have no more following token.
              * But this isn't the case. Abort. */
             g_set_error(error,
@@ -344,26 +429,12 @@ _parse_legacy_addr4(const char *text, in_addr_t *out_addr, GError **error)
             return FALSE;
         }
 
-        bin[i] = v;
+        addr.b[i] = v;
     }
 
-    if (memcmp(bin, &a1, sizeof(bin)) != 0) {
-        /* our parsing did not agree with what inet_aton() gave. Something
-         * is wrong. Abort. */
-        g_set_error(
-            error,
-            NM_UTILS_ERROR,
-            NM_UTILS_ERROR_INVALID_ARGUMENT,
-            "inet_aton() result 0x%08x differs from computed value 0x%02hhx%02hhx%02hhx%02hhx",
-            a1.s_addr,
-            bin[0],
-            bin[1],
-            bin[2],
-            bin[3]);
-        return FALSE;
-    }
+    _nm_assert_legacy_addr4(text, addr.a);
 
-    *out_addr = a1.s_addr;
+    *out_addr = addr.a;
     return TRUE;
 }
 
diff --git a/src/libnm-glib-aux/nm-inet-utils.h b/src/libnm-glib-aux/nm-inet-utils.h
index 087a8af179..65ceeb2e04 100644
--- a/src/libnm-glib-aux/nm-inet-utils.h
+++ b/src/libnm-glib-aux/nm-inet-utils.h
@@ -369,6 +369,8 @@ nm_inet6_ntop_dup(const struct in6_addr *addr)
 
 /*****************************************************************************/
 
+int nmtst_inet_aton(const char *text, in_addr_t *out_addr);
+
 gboolean nm_inet_parse_bin_full(int         addr_family,
                                 gboolean    accept_legacy,
                                 const char *text,
diff --git a/src/libnm-glib-aux/tests/test-shared-general.c b/src/libnm-glib-aux/tests/test-shared-general.c
index 563c370243..84d87d4848 100644
--- a/src/libnm-glib-aux/tests/test-shared-general.c
+++ b/src/libnm-glib-aux/tests/test-shared-general.c
@@ -2313,6 +2313,130 @@ test_inet_utils(void)
 
 /*****************************************************************************/
 
+static gboolean
+_inet_parse(int addr_family, const char *str, gboolean accept_legacy, gpointer out_addr)
+{
+    int        addr_family2   = -1;
+    int *const p_addr_family2 = nmtst_get_rand_bool() ? &addr_family2 : NULL;
+    NMIPAddr   addr;
+    gboolean   success;
+
+    g_assert(NM_IN_SET(addr_family, AF_INET, AF_INET6));
+
+    success =
+        nm_inet_parse_bin_full((p_addr_family2 && nmtst_get_rand_bool()) ? AF_UNSPEC : addr_family,
+                               accept_legacy,
+                               str,
+                               p_addr_family2,
+                               &addr);
+
+    if (success) {
+        g_assert(!p_addr_family2 || NM_IN_SET(*p_addr_family2, AF_INET, AF_INET6));
+        if (p_addr_family2 && *p_addr_family2 != addr_family) {
+            success = FALSE;
+        } else
+            g_assert(!p_addr_family2 || *p_addr_family2 == addr_family);
+    } else
+        g_assert(addr_family2 == -1);
+
+    if (out_addr && success)
+        nm_ip_addr_set(addr_family, out_addr, &addr);
+
+    return success;
+}
+
+#define _inet_parse_fail(check, accept_legacy)                             \
+    G_STMT_START                                                           \
+    {                                                                      \
+        NMIPAddr _addr;                                                    \
+        gboolean _success;                                                 \
+                                                                           \
+        _success = _inet_parse(nmtst_get_rand_bool() ? AF_INET : AF_INET6, \
+                               "" check "",                                \
+                               (accept_legacy),                            \
+                               nmtst_get_rand_bool() ? &_addr : NULL);     \
+        g_assert(!_success);                                               \
+    }                                                                      \
+    G_STMT_END
+
+#define _inet_parse_good(check, expected, accept_legacy)                        \
+    G_STMT_START                                                                \
+    {                                                                           \
+        int               _accept_legacy = (accept_legacy);                     \
+        const char *const _check         = "" check "";                         \
+        const char *const _expected      = expected ?: _check;                  \
+        NMIPAddr          _addr[2];                                             \
+        gboolean          _success[2];                                          \
+                                                                                \
+        if (_accept_legacy == -1)                                               \
+            _accept_legacy = nmtst_get_rand_bool();                             \
+                                                                                \
+        _success[0] = _inet_parse(AF_INET6, _check, _accept_legacy, &_addr[0]); \
+        _success[1] = _inet_parse(AF_INET, _check, _accept_legacy, &_addr[1]);  \
+                                                                                \
+        g_assert(NM_IN_SET(_success[0], FALSE, TRUE));                          \
+        g_assert(NM_IN_SET(_success[1], FALSE, TRUE));                          \
+        g_assert(_success[0] != _success[1]);                                   \
+                                                                                \
+        if (_success[0])                                                        \
+            nmtst_assert_ip6_address(&_addr[0].addr6, _expected);               \
+        else                                                                    \
+            nmtst_assert_ip4_address(_addr[1].addr4, _expected);                \
+                                                                                \
+        if (_success[1]) {                                                      \
+            in_addr_t _a4;                                                      \
+            int       _r;                                                       \
+                                                                                \
+            _r = nmtst_inet_aton(_check, &_a4);                                 \
+            g_assert_cmpint(_r, ==, 0);                                         \
+            nmtst_assert_ip4_address(_a4, _expected);                           \
+        }                                                                       \
+    }                                                                           \
+    G_STMT_END
+
+static void
+test_inet_parse_ip4_legacy(void)
+{
+    _inet_parse_fail("", -1);
+    _inet_parse_fail(" ", -1);
+    _inet_parse_fail("a", -1);
+    _inet_parse_fail("0", -1);
+    _inet_parse_fail("0.1", -1);
+    _inet_parse_fail("0.4.1", -1);
+    _inet_parse_fail("1.2.3.05", FALSE);
+    _inet_parse_fail("192.000.002.010", FALSE);
+    _inet_parse_fail("1.2.3..5", -1);
+    _inet_parse_fail("1.2.3.0x", -1);
+    _inet_parse_fail("0xC0000234", -1);
+    _inet_parse_fail("192.0.2.2X", -1);
+    _inet_parse_fail("192.0.2.3 Y", -1);
+    _inet_parse_fail("192.0.2.4\nZ", -1);
+    _inet_parse_fail("192.0.2.5\tT", -1);
+    _inet_parse_fail("192.0.2.6 Y", -1);
+    _inet_parse_fail("192.0.2.7\n", -1);
+    _inet_parse_fail("192.0.2.7\t", -1);
+    _inet_parse_fail("192.0.2.7 ", -1);
+    _inet_parse_fail("00x0019.0000001.000000.0x1", -1);
+    _inet_parse_fail("192.0.2.7.", -1);
+    _inet_parse_fail("192.0.2.7.0", -1);
+
+    _inet_parse_good("192.0.2.1", NULL, -1);
+    _inet_parse_good("1.2.3.4", NULL, -1);
+    _inet_parse_good("192.167.3.4", NULL, -1);
+
+    _inet_parse_good("192.000.002.010", "192.0.2.8", TRUE);
+    _inet_parse_good("255.000.000.000", "255.0.0.0", TRUE);
+    _inet_parse_good("1.2.3.05", "1.2.3.5", TRUE);
+    _inet_parse_good("01.2.3.05", "1.2.3.5", TRUE);
+    _inet_parse_good("192.00167.0003.4", "192.119.3.4", TRUE);
+    _inet_parse_good("0x19.00167.0003.4", "25.119.3.4", TRUE);
+    _inet_parse_good("0x19.000000167.0000003.4", "25.119.3.4", TRUE);
+    _inet_parse_good("0x0019.000000167.0000003.04", "25.119.3.4", TRUE);
+    _inet_parse_good("0x0019.0000001.000000.0x1", "25.1.0.1", TRUE);
+}
+
+/*****************************************************************************/
+
 static void
 test_garray(void)
 {
@@ -2495,6 +2619,7 @@ main(int argc, char **argv)
     g_test_add_func("/general/test_path_simplify", test_path_simplify);
     g_test_add_func("/general/test_hostname_is_valid", test_hostname_is_valid);
     g_test_add_func("/general/test_inet_utils", test_inet_utils);
+    g_test_add_func("/general/test_inet_parse_ip4_legacy", test_inet_parse_ip4_legacy);
     g_test_add_func("/general/test_garray", test_garray);
     g_test_add_func("/general/test_nm_prioq", test_nm_prioq);
     g_test_add_func("/general/test_nm_random", test_nm_random);
diff --git a/src/nm-initrd-generator/nmi-dt-reader.c b/src/nm-initrd-generator/nmi-dt-reader.c
index f1279d5807..b273822a36 100644
--- a/src/nm-initrd-generator/nmi-dt-reader.c
+++ b/src/nm-initrd-generator/nmi-dt-reader.c
@@ -96,6 +96,11 @@ str_addr(const char *str, int *family)
 {
     NMIPAddr addr_bin;
 
+    /* For IPv4, we need to be more tolerant than inet_pton() to recognize
+     * things like the extra zeroes in "255.255.255.000".
+     *
+     * Pass accept_legacy=TRUE to nm_inet_parse_bin_full(), which also accepts
+     * such forms (but not everything which inet_aton() accepts). */
     if (!nm_inet_parse_bin_full(*family, TRUE, str, family, &addr_bin)) {
         _LOGW(LOGD_CORE, "Malformed IP address: '%s'", str);
         return NULL;