Merge pull request systemd#12252 from keszybz/libmount-dont-unescape

Don't unescape paths from libmount

Author: keszybz

src/core/mount.c

@@ -5,18 +5,16 @@
 #include <stdio.h>
 #include <sys/epoll.h>
 
-#include <libmount.h>
-
 #include "sd-messages.h"
 
 #include "alloc-util.h"
 #include "dbus-mount.h"
 #include "dbus-unit.h"
 #include "device.h"
-#include "escape.h"
 #include "exit-status.h"
 #include "format-util.h"
 #include "fstab-util.h"
+#include "libmount-util.h"
 #include "log.h"
 #include "manager.h"
 #include "mkdir.h"
@@ -36,9 +34,6 @@
 
 #define RETRY_UMOUNT_MAX 32
 
-DEFINE_TRIVIAL_CLEANUP_FUNC(struct libmnt_table*, mnt_free_table);
-DEFINE_TRIVIAL_CLEANUP_FUNC(struct libmnt_iter*, mnt_free_iter);
-
 static const UnitActiveState state_translation_table[_MOUNT_STATE_MAX] = {
         [MOUNT_DEAD] = UNIT_INACTIVE,
         [MOUNT_MOUNTING] = UNIT_ACTIVATING,
@@ -1620,7 +1615,6 @@ static int mount_load_proc_self_mountinfo(Manager *m, bool set_flags) {
         for (;;) {
                 struct libmnt_fs *fs;
                 const char *device, *path, *options, *fstype;
-                _cleanup_free_ char *d = NULL, *p = NULL;
                 int k;
 
                 k = mnt_table_next_fs(t, i, &fs);
@@ -1637,15 +1631,9 @@ static int mount_load_proc_self_mountinfo(Manager *m, bool set_flags) {
                 if (!device || !path)
                         continue;
 
-                if (cunescape(device, UNESCAPE_RELAX, &d) < 0)
-                        return log_oom();
-
-                if (cunescape(path, UNESCAPE_RELAX, &p) < 0)
-                        return log_oom();
-
-                device_found_node(m, d, DEVICE_FOUND_MOUNT, DEVICE_FOUND_MOUNT);
+                device_found_node(m, device, DEVICE_FOUND_MOUNT, DEVICE_FOUND_MOUNT);
 
-                (void) mount_setup_unit(m, d, p, options, fstype, set_flags);
+                (void) mount_setup_unit(m, device, path, options, fstype, set_flags);
         }
 
         return 0;

src/shared/libmount-util.h

@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: LGPL-2.1+ */
+#pragma once
+
+/* This needs to be after sys/mount.h */
+#include <libmount.h>
+
+#include "macro.h"
+
+DEFINE_TRIVIAL_CLEANUP_FUNC(struct libmnt_table*, mnt_free_table);
+DEFINE_TRIVIAL_CLEANUP_FUNC(struct libmnt_iter*, mnt_free_iter);

src/shared/meson.build

@@ -95,6 +95,7 @@ shared_sources = files('''
         json-internal.h
         json.c
         json.h
+        libmount-util.h
         lockfile-util.c
         lockfile-util.h
         log-link.h

src/shutdown/umount.c

@@ -13,9 +13,6 @@
 #include <sys/types.h>
 #include <unistd.h>
 
-/* This needs to be after sys/mount.h :( */
-#include <libmount.h>
-
 #include "sd-device.h"
 
 #include "alloc-util.h"
@@ -25,6 +22,7 @@
 #include "escape.h"
 #include "fd-util.h"
 #include "fstab-util.h"
+#include "libmount-util.h"
 #include "linux-3.13/dm-ioctl.h"
 #include "mount-setup.h"
 #include "mount-util.h"
@@ -38,9 +36,6 @@
 #include "util.h"
 #include "virt.h"
 
-DEFINE_TRIVIAL_CLEANUP_FUNC(struct libmnt_table*, mnt_free_table);
-DEFINE_TRIVIAL_CLEANUP_FUNC(struct libmnt_iter*, mnt_free_iter);
-
 static void mount_point_free(MountPoint **head, MountPoint *m) {
         assert(head);
         assert(m);
@@ -79,11 +74,10 @@ int mount_points_list_get(const char *mountinfo, MountPoint **head) {
                 struct libmnt_fs *fs;
                 const char *path, *fstype;
                 _cleanup_free_ char *options = NULL;
-                _cleanup_free_ char *p = NULL;
                 unsigned long remount_flags = 0u;
                 _cleanup_free_ char *remount_options = NULL;
                 bool try_remount_ro;
-                MountPoint *m;
+                _cleanup_free_ MountPoint *m = NULL;
 
                 r = mnt_table_next_fs(t, i, &fs);
                 if (r == 1)
@@ -95,18 +89,15 @@ int mount_points_list_get(const char *mountinfo, MountPoint **head) {
                 if (!path)
                         continue;
 
-                if (cunescape(path, UNESCAPE_RELAX, &p) < 0)
-                        return log_oom();
-
                 fstype = mnt_fs_get_fstype(fs);
 
                 /* Combine the generic VFS options with the FS-specific
                  * options. Duplicates are not a problem here, because the only
                  * options that should come up twice are typically ro/rw, which
-                 * are turned into MS_RDONLY or the invertion of it.
+                 * are turned into MS_RDONLY or the inversion of it.
                  *
                  * Even if there are duplicates later in mount_option_mangle()
-                 * it shouldn't hurt anyways as they override each other.
+                 * they shouldn't hurt anyways as they override each other.
                  */
                 if (!strextend_with_separator(&options, ",",
                                               mnt_fs_get_vfs_options(fs),
@@ -124,9 +115,9 @@ int mount_points_list_get(const char *mountinfo, MountPoint **head) {
                  * and hence not worth spending time on. Also, in
                  * unprivileged containers we might lack the rights to
                  * unmount these things, hence don't bother. */
-                if (mount_point_is_api(p) ||
-                    mount_point_ignore(p) ||
-                    PATH_STARTSWITH_SET(p, "/dev", "/sys", "/proc"))
+                if (mount_point_is_api(path) ||
+                    mount_point_ignore(path) ||
+                    PATH_STARTSWITH_SET(path, "/dev", "/sys", "/proc"))
                         continue;
 
                 /* If we are in a container, don't attempt to
@@ -172,12 +163,15 @@ int mount_points_list_get(const char *mountinfo, MountPoint **head) {
                 if (!m)
                         return log_oom();
 
-                free_and_replace(m->path, p);
-                free_and_replace(m->remount_options, remount_options);
+                m->path = strdup(path);
+                if (!m->path)
+                        return log_oom();
+
+                m->remount_options = TAKE_PTR(remount_options);
                 m->remount_flags = remount_flags;
                 m->try_remount_ro = try_remount_ro;
 
-                LIST_PREPEND(mount_point, *head, m);
+                LIST_PREPEND(mount_point, *head, TAKE_PTR(m));
         }
 
         return 0;
@@ -201,10 +195,8 @@ int swap_list_get(const char *swaps, MountPoint **head) {
 
         for (;;) {
                 struct libmnt_fs *fs;
-
-                MountPoint *swap;
+                _cleanup_free_ MountPoint *swap = NULL;
                 const char *source;
-                _cleanup_free_ char *d = NULL;
 
                 r = mnt_table_next_fs(t, i, &fs);
                 if (r == 1)
@@ -216,16 +208,15 @@ int swap_list_get(const char *swaps, MountPoint **head) {
                 if (!source)
                         continue;
 
-                r = cunescape(source, UNESCAPE_RELAX, &d);
-                if (r < 0)
-                        return r;
-
                 swap = new0(MountPoint, 1);
                 if (!swap)
                         return -ENOMEM;
 
-                free_and_replace(swap->path, d);
-                LIST_PREPEND(mount_point, *head, swap);
+                swap->path = strdup(source);
+                if (!swap->path)
+                        return -ENOMEM;
+
+                LIST_PREPEND(mount_point, *head, TAKE_PTR(swap));
         }
 
         return 0;

src/test/meson.build

@@ -228,6 +228,11 @@ tests += [
          [],
          []],
 
+        [['src/test/test-libmount.c'],
+         [],
+         [threads,
+          libmount]],
+
         [['src/test/test-mount-util.c'],
          [],
          []],

src/test/test-libmount.c

@@ -0,0 +1,118 @@
+/* SPDX-License-Identifier: LGPL-2.1+ */
+
+#include "alloc-util.h"
+#include "fd-util.h"
+#include "escape.h"
+#include "libmount-util.h"
+#include "tests.h"
+
+static void test_libmount_unescaping_one(
+                const char *title,
+                const char *string,
+                bool may_fail,
+                const char *expected_source,
+                const char *expected_target) {
+        /* A test for libmount really */
+        int r;
+
+        log_info("/* %s %s */", __func__, title);
+
+        _cleanup_(mnt_free_tablep) struct libmnt_table *table = NULL;
+        _cleanup_(mnt_free_iterp) struct libmnt_iter *iter = NULL;
+        _cleanup_fclose_ FILE *f = NULL;
+
+        assert_se(table = mnt_new_table());
+        assert_se(iter = mnt_new_iter(MNT_ITER_FORWARD));
+
+        f = fmemopen((char*) string, strlen(string), "re");
+        assert_se(f);
+
+        assert_se(mnt_table_parse_stream(table, f, title) >= 0);
+
+        struct libmnt_fs *fs;
+        const char *source, *target;
+        _cleanup_free_ char *x = NULL, *cs = NULL, *s = NULL, *ct = NULL, *t = NULL;
+
+        /* We allow this call and the checks below to fail in some cases. See the case definitions below. */
+
+        r = mnt_table_next_fs(table, iter, &fs);
+        if (r != 0 && may_fail) {
+                log_error_errno(r, "mnt_table_next_fs failed: %m");
+                return;
+        }
+        assert_se(r == 0);
+
+        assert_se(x = cescape(string));
+
+        assert_se(source = mnt_fs_get_source(fs));
+        assert_se(target = mnt_fs_get_target(fs));
+
+        assert_se(cs = cescape(source));
+        assert_se(ct = cescape(target));
+
+        assert_se(cunescape(source, UNESCAPE_RELAX, &s) >= 0);
+        assert_se(cunescape(target, UNESCAPE_RELAX, &t) >= 0);
+
+        log_info("from '%s'", x);
+        log_info("source: '%s'", source);
+        log_info("source: '%s'", cs);
+        log_info("source: '%s'", s);
+        log_info("expected: '%s'", strna(expected_source));
+        log_info("target: '%s'", target);
+        log_info("target: '%s'", ct);
+        log_info("target: '%s'", t);
+        log_info("expected: '%s'", strna(expected_target));
+
+        assert_se(may_fail || streq(source, expected_source));
+        assert_se(may_fail || streq(target, expected_target));
+
+        assert_se(mnt_table_next_fs(table, iter, &fs) == 1);
+}
+
+static void test_libmount_unescaping(void) {
+        test_libmount_unescaping_one(
+                        "escaped space + utf8",
+                        "729 38 0:59 / /tmp/„zupa\\040zębowa” rw,relatime shared:395 - tmpfs die\\040Brühe rw,seclabel",
+                        false,
+                        "die Brühe",
+                        "/tmp/„zupa zębowa”"
+        );
+
+        test_libmount_unescaping_one(
+                        "escaped newline",
+                        "729 38 0:59 / /tmp/x\\012y rw,relatime shared:395 - tmpfs newline rw,seclabel",
+                        false,
+                        "newline",
+                        "/tmp/x\ny"
+        );
+
+        /* The result of "mount -t tmpfs '' /tmp/emptysource".
+         * This will fail with libmount <= v2.33.
+         * See https://github.com/karelzak/util-linux/commit/18a52a5094.
+         */
+        test_libmount_unescaping_one(
+                        "empty source",
+                        "760 38 0:60 / /tmp/emptysource rw,relatime shared:410 - tmpfs  rw,seclabel",
+                        true,
+                        "",
+                        "/tmp/emptysource"
+        );
+
+        /* The kernel leaves \r as is.
+         * Also see https://github.com/karelzak/util-linux/issues/780.
+         */
+        test_libmount_unescaping_one(
+                        "foo\\rbar",
+                        "790 38 0:61 / /tmp/foo\rbar rw,relatime shared:425 - tmpfs tmpfs rw,seclabel",
+                        true,
+                        "tmpfs",
+                        "/tmp/foo\rbar"
+        );
+}
+
+int main(int argc, char *argv[]) {
+        test_setup_logging(LOG_DEBUG);
+
+        test_libmount_unescaping();
+        return 0;
+}