From fb36b1339be0211d5d4cd6d035df704bdd589597 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 4 Apr 2019 12:54:19 +0200 Subject: [PATCH 1/3] shared: add a single definition of libmount cleanup functions Use a trivial header file to share mnt_free_tablep and mnt_free_iterp. It would be nicer put this in mount-util.h, but libmount.h is not in the default include path, and the build system would have to be adjusted to pass pkg-config include path in various places, and it's just not worth the trouble. A separate header file works nicely. --- src/core/mount.c | 6 +----- src/shared/libmount-util.h | 10 ++++++++++ src/shared/meson.build | 1 + src/shutdown/umount.c | 7 +------ 4 files changed, 13 insertions(+), 11 deletions(-) create mode 100644 src/shared/libmount-util.h diff --git a/src/core/mount.c b/src/core/mount.c index 965dc51db5..c1b0c29d80 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -5,8 +5,6 @@ #include #include -#include - #include "sd-messages.h" #include "alloc-util.h" @@ -17,6 +15,7 @@ #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 +35,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, diff --git a/src/shared/libmount-util.h b/src/shared/libmount-util.h new file mode 100644 index 0000000000..7d94468e52 --- /dev/null +++ b/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 + +#include "macro.h" + +DEFINE_TRIVIAL_CLEANUP_FUNC(struct libmnt_table*, mnt_free_table); +DEFINE_TRIVIAL_CLEANUP_FUNC(struct libmnt_iter*, mnt_free_iter); diff --git a/src/shared/meson.build b/src/shared/meson.build index 47cbc9932c..bb28bcac2b 100644 --- a/src/shared/meson.build +++ b/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 diff --git a/src/shutdown/umount.c b/src/shutdown/umount.c index 0ed358bb32..3d6cfa08f2 100644 --- a/src/shutdown/umount.c +++ b/src/shutdown/umount.c @@ -13,9 +13,6 @@ #include #include -/* This needs to be after sys/mount.h :( */ -#include - #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); From b57adc94cde046d66a3c4f94b8cd8defa43ff6bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 5 Apr 2019 09:43:12 +0200 Subject: [PATCH 2/3] test-libmount: let's see how libmount parses stuff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With libmount-2.33.1-3.fc30.x86_64 I get: /* test_libmount_unescaping_one escaped space + utf8 */ from '729 38 0:59 / /tmp/\342\200\236zupa\\040z\304\231bowa\342\200\235 rw,relatime shared:395 - tmpfs die\\040Br\303\274he rw,seclabel' source: 'die Brühe' source: 'die Br\303\274he' source: 'die Brühe' expected: 'die Brühe' target: '/tmp/„zupa zębowa”' target: '/tmp/\342\200\236zupa z\304\231bowa\342\200\235' target: '/tmp/„zupa zębowa”' expected: '/tmp/„zupa zębowa”' /* test_libmount_unescaping_one escaped newline */ from '729 38 0:59 / /tmp/x\\012y rw,relatime shared:395 - tmpfs newline rw,seclabel' source: 'newline' source: 'newline' source: 'newline' expected: 'newline' target: '/tmp/x y' target: '/tmp/x\ny' target: '/tmp/x y' expected: '/tmp/x y' /* test_libmount_unescaping_one empty source */ from '760 38 0:60 / /tmp/emptysource rw,relatime shared:410 - tmpfs rw,seclabel' source: '' source: '' source: '' expected: '' target: '/tmp/emptysource' target: '/tmp/emptysource' target: '/tmp/emptysource' expected: '/tmp/emptysource' /* test_libmount_unescaping_one foo\rbar */ from '790 38 0:61 / /tmp/foo\rbar rw,relatime shared:425 - tmpfs tmpfs rw,seclabel' source: 'tmpfs' source: 'tmpfs' source: 'tmpfs' expected: 'tmpfs' target: '/tmp/foo' target: '/tmp/foo' target: '/tmp/foo' expected: 'n/a' With https://github.com/karelzak/util-linux/issues/780 fixed, we get /* test_libmount_unescaping_one foo\rbar */ from '790 38 0:61 / /tmp/foo\rbar rw,relatime shared:425 - tmpfs tmpfs rw,seclabel' source: 'tmpfs' source: 'tmpfs' source: 'tmpfs' expected: 'tmpfs' target: '/tmp/foo bar' target: '/tmp/foo\rbar' target: '/tmp/foo bar' expected: '/tmp/foo bar' --- src/test/meson.build | 5 ++ src/test/test-libmount.c | 118 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+) create mode 100644 src/test/test-libmount.c diff --git a/src/test/meson.build b/src/test/meson.build index 19f7a1e486..ae970cf8a0 100644 --- a/src/test/meson.build +++ b/src/test/meson.build @@ -228,6 +228,11 @@ tests += [ [], []], + [['src/test/test-libmount.c'], + [], + [threads, + libmount]], + [['src/test/test-mount-util.c'], [], []], diff --git a/src/test/test-libmount.c b/src/test/test-libmount.c new file mode 100644 index 0000000000..fc28f27d53 --- /dev/null +++ b/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; +} From 9d1b2b225242fd3514d251c517e37fd97025131b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 5 Apr 2019 10:17:03 +0200 Subject: [PATCH 3/3] pid1,shutdown: do not cunescape paths from libmount The test added in previous commit shows that libmount does the unescaping internally. --- src/core/mount.c | 12 ++---------- src/shutdown/umount.c | 40 ++++++++++++++++++---------------------- 2 files changed, 20 insertions(+), 32 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index c1b0c29d80..b7fd35fc67 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -11,7 +11,6 @@ #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" @@ -1616,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); @@ -1633,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(); + device_found_node(m, device, DEVICE_FOUND_MOUNT, DEVICE_FOUND_MOUNT); - if (cunescape(path, UNESCAPE_RELAX, &p) < 0) - return log_oom(); - - device_found_node(m, d, 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; diff --git a/src/shutdown/umount.c b/src/shutdown/umount.c index 3d6cfa08f2..928bae6ab1 100644 --- a/src/shutdown/umount.c +++ b/src/shutdown/umount.c @@ -74,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) @@ -90,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), @@ -119,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 @@ -167,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; @@ -196,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) @@ -211,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;