Subject: bug#15490: [PATCH] Refactor device canonicalization

From: Phillip Susi <[email-masked]>
Date: Sun, 29 Sep 2013 22:18:55 -0400

Previous attempts to use the correct name for device-mapper devices on
linux simply bypassed canonicalizing the given name if it started with
"/dev/mapper".  This gave the correct name ( as opposed to following the
symlink in /dev/mapper to /dev/dm-X ), but only when the given name was
the /dev/mapper name.  The wrong name would still be used when given some
other symlink, such as /dev/disk/by-id.  This was worked around in linux.c
by having linux_partition_get_path generate the full canonical path name
to the partition, while parted still reported the symlink as the name
of the disk.

This patch adds an arch specific get_normal_path method that, if
implemented, is used to find the canonical device path instead of the
canonicalize_file_name function from gnulib.  The linux arch implements
this by generating the correct /dev/mapper name and no longer has to
special case linux_partition_get_path to generate the base name.

The original t3000-symlink test was incorrect because it relied on the
hard coded "/dev/mapper" prefix so it has been replaced with
t6004-dm-symlink.sh instead, which creates a real dm device and a symlink
to it to verify the real name is used when the user gives the symlink.

t6000-dm.sh was also relying on a private /dev/mapper directory, which
is unworkable.
---
 include/parted/device.in.h       |   1 +
 libparted/arch/linux.c           |  52 +++++++--------
 libparted/device.c               |   6 +-
 libparted/tests/Makefile.am      |   5 +-
 libparted/tests/symlink.c        | 135 ---------------------------------------
 libparted/tests/t3000-symlink.sh |  27 --------
 tests/Makefile.am                |   1 +
 tests/t6000-dm.sh                |   2 +-
 tests/t6004-dm-symlink.sh        |  68 ++++++++++++++++++++
 9 files changed, 103 insertions(+), 194 deletions(-)

diff --git a/include/parted/device.in.h b/include/parted/device.in.h
index 7c06a66..d243fcc 100644
--- a/include/parted/device.in.h
+++ b/include/parted/device.in.h
@@ -119,6 +119,7 @@ struct _PedDeviceArchOps {
         /* These functions are optional */
         PedAlignment *(*get_minimum_alignment)(const PedDevice *dev);
         PedAlignment *(*get_optimum_alignment)(const PedDevice *dev);
+        char *(*get_normal_path)(const char *path);
 };
 
 #include <parted/constraint.h>
diff --git a/libparted/arch/linux.c b/libparted/arch/linux.c
index d3a03d6..c2a6184 100644
--- a/libparted/arch/linux.c
+++ b/libparted/arch/linux.c
@@ -2286,24 +2286,29 @@ zasprintf (const char *format, ...)
 }
 
 static char *
-dm_canonical_path (PedDevice const *dev)
+linux_get_normal_path (const char *path)
 {
-        LinuxSpecific const *arch_specific = LINUX_SPECIFIC (dev);
-
-        /* Get map name from devicemapper */
-        struct dm_task *task = dm_task_create (DM_DEVICE_INFO);
-        if (!task)
-                goto err;
-        if (!dm_task_set_major_minor (task, arch_specific->major,
-                                      arch_specific->minor, 0))
-                goto err;
-        if (!dm_task_run(task))
-                goto err;
-        char *dev_name = zasprintf ("/dev/mapper/%s", dm_task_get_name (task));
-        if (dev_name == NULL)
+        struct stat st;
+        if (stat(path, &st))
                 goto err;
-        dm_task_destroy (task);
-        return dev_name;
+        if (!S_ISBLK(st.st_mode))
+                return canonicalize_file_name(path);
+        if (_is_dm_major(major(st.st_rdev))) {
+                /* Get map name from devicemapper */
+                struct dm_task *task = dm_task_create (DM_DEVICE_INFO);
+                if (!task)
+                        goto err;
+                if (!dm_task_set_major_minor (task, major(st.st_rdev),
+                                              minor(st.st_rdev), 0))
+                        goto err;
+                if (!dm_task_run(task))
+                        goto err;
+                char *dev_name = zasprintf ("/dev/mapper/%s", dm_task_get_name (task));
+                if (dev_name == NULL)
+                        goto err;
+                dm_task_destroy (task);
+                return dev_name;
+        } else return canonicalize_file_name(path);
 err:
         return NULL;
 }
@@ -2311,27 +2316,23 @@ err:
 static char*
 _device_get_part_path (PedDevice const *dev, int num)
 {
-        char *devpath = (dev->type == PED_DEVICE_DM
-                         ? dm_canonical_path (dev) : dev->path);
-        size_t path_len = strlen (devpath);
+        size_t path_len = strlen (dev->path);
         char *result;
         /* Check for devfs-style /disc => /partN transformation
            unconditionally; the system might be using udev with devfs rules,
            and if not the test is harmless. */
-        if (5 < path_len && !strcmp (devpath + path_len - 5, "/disc")) {
+        if (5 < path_len && !strcmp (dev->path + path_len - 5, "/disc")) {
                 /* replace /disc with /part%d */
                 result = zasprintf ("%.*s/part%d",
-                                    (int) (path_len - 5), devpath, num);
+                                    (int) (path_len - 5), dev->path, num);
         } else {
                 char const *p = (dev->type == PED_DEVICE_DAC960
                                  || dev->type == PED_DEVICE_CPQARRAY
                                  || dev->type == PED_DEVICE_ATARAID
-                                 || isdigit (devpath[path_len - 1])
+                                 || isdigit (dev->path[path_len - 1])
                                  ? "p" : "");
-                result = zasprintf ("%s%s%d", devpath, p, num);
+                result = zasprintf ("%s%s%d", dev->path, p, num);
         }
-        if (dev->type == PED_DEVICE_DM)
-                free (devpath);
         return result;
 }
 
@@ -2997,6 +2998,7 @@ static PedDeviceArchOps linux_dev_ops = {
         get_minimum_alignment:	linux_get_minimum_alignment,
         get_optimum_alignment:	linux_get_optimum_alignment,
 #endif
+        get_normal_path: linux_get_normal_path,
 };
 
 PedDiskArchOps linux_disk_ops =  {
diff --git a/libparted/device.c b/libparted/device.c
index 738b320..9b67454 100644
--- a/libparted/device.c
+++ b/libparted/device.c
@@ -152,9 +152,9 @@ ped_device_get (const char* path)
 	char*		normal_path = NULL;
 
 	PED_ASSERT (path != NULL);
-	/* Don't canonicalize /dev/mapper paths, see tests/symlink.c */
-	if (strncmp (path, "/dev/mapper/", 12))
-		normal_path = canonicalize_file_name (path);
+	if (ped_architecture->dev_ops->get_normal_path)
+		normal_path = ped_architecture->dev_ops->get_normal_path(path);
+	else normal_path = canonicalize_file_name (path);
 	if (!normal_path)
 		/* Well, maybe it is just that the file does not exist.
 		 * Try it anyway.  */
diff --git a/libparted/tests/Makefile.am b/libparted/tests/Makefile.am
index bfa5790..4e99ed1 100644
--- a/libparted/tests/Makefile.am
+++ b/libparted/tests/Makefile.am
@@ -3,9 +3,9 @@
 #
 # This file may be modified and/or distributed without restriction.
 
-TESTS = t1000-label.sh t2000-disk.sh t2100-zerolen.sh t3000-symlink.sh
+TESTS = t1000-label.sh t2000-disk.sh t2100-zerolen.sh
 EXTRA_DIST = $(TESTS)
-check_PROGRAMS = label disk zerolen symlink
+check_PROGRAMS = label disk zerolen
 AM_CFLAGS = $(WARN_CFLAGS) $(WERROR_CFLAGS)
 
 LDADD = \
@@ -21,7 +21,6 @@ AM_CPPFLAGS = \
 label_SOURCES = common.h common.c label.c
 disk_SOURCES  = common.h common.c disk.c
 zerolen_SOURCES = common.h common.c zerolen.c
-symlink_SOURCES = common.h common.c symlink.c
 
 # Arrange to symlink to tests/init.sh.
 CLEANFILES = init.sh
diff --git a/libparted/tests/symlink.c b/libparted/tests/symlink.c
deleted file mode 100644
index 52e99ca..0000000
--- a/libparted/tests/symlink.c
+++ /dev/null
@@ -1,135 +0,0 @@
-/* Sometimes libparted operates on device mapper files, with a path of
-   /dev/mapper/foo. With newer lvm versions /dev/mapper/foo is a symlink to
-   /dev/dm-#. However some storage administration programs (anaconda for
-   example) may do the following:
-   1) Create a ped_device for /dev/mapper/foo
-   2) ped_get_device resolves the symlink to /dev/dm-#, and the path
-      in the PedDevice struct points to /dev/dm-#
-   3) The program does some things to lvm, causing the symlink to
-      point to a different /dev/dm-# node
-   4) The program does something with the PedDevice, which results
-      in an operation on the wrong device
-
-   Newer libparted versions do not suffer from this problem, as they
-   do not canonicalize device names under /dev/mapper. This test checks
-   for this bug. */
-
-#include <config.h>
-#include <unistd.h>
-
-#include <check.h>
-
-#include <parted/parted.h>
-
-#include "common.h"
-#include "progname.h"
-
-static char *temporary_disk;
-
-static void
-create_disk (void)
-{
-        temporary_disk = _create_disk (4096 * 1024);
-        fail_if (temporary_disk == NULL, "Failed to create temporary disk");
-}
-
-static void
-destroy_disk (void)
-{
-        unlink (temporary_disk);
-        free (temporary_disk);
-}
-
-START_TEST (test_symlink)
-{
-        char cwd[256], ln[256] = "/dev/mapper/parted-test-XXXXXX";
-
-        if (!getcwd (cwd, sizeof cwd)) {
-                fail ("Could not get cwd");
-                return;
-        }
-
-        /* Create a symlink under /dev/mapper to our
-           temporary disk */
-        int tmp_fd = mkstemp (ln);
-        if (tmp_fd == -1) {
-                fail ("Could not create tempfile");
-                return;
-        }
-
-        /* There is a temp file vulnerability symlink attack possibility
-           here, but as /dev/mapper is root owned this is a non issue */
-        close (tmp_fd);
-        unlink (ln);
-        char temp_disk_path[256];
-        snprintf (temp_disk_path, sizeof temp_disk_path, "%s/%s", cwd,
-                  temporary_disk);
-        int res = symlink (temp_disk_path, ln);
-        if (res) {
-                fail ("could not create symlink");
-                return;
-        }
-
-        PedDevice *dev = ped_device_get (ln);
-        if (dev == NULL)
-                goto exit_unlink_ln;
-
-        /* Create a second temporary_disk */
-        char *temporary_disk2 = _create_disk (4096 * 1024);
-        if (temporary_disk2 == NULL) {
-                fail ("Failed to create 2nd temporary disk");
-                goto exit_destroy_dev;
-        }
-
-        /* Remove first temporary disk, and make temporary_disk point to
-           temporary_disk2 (for destroy_disk()). */
-        unlink (temporary_disk);
-        free (temporary_disk);
-        temporary_disk = temporary_disk2;
-
-        /* Update symlink to point to our new / second temporary disk */
-        unlink (ln);
-        snprintf (temp_disk_path, sizeof temp_disk_path, "%s/%s", cwd,
-                  temporary_disk);
-        res = symlink (temp_disk_path, ln);
-        if (res) {
-                fail ("could not create 2nd symlink");
-                goto exit_destroy_dev;
-        }
-
-        /* Do something to our PedDevice, if the symlink was resolved,
-           instead of remembering the /dev/mapper/foo name, this will fail */
-        ped_disk_clobber (dev);
-
-exit_destroy_dev:
-        ped_device_destroy (dev);
-exit_unlink_ln:
-        unlink (ln);
-}
-END_TEST
-
-int
-main (int argc, char **argv)
-{
-        set_program_name (argv[0]);
-        int number_failed;
-        Suite* suite = suite_create ("Symlink");
-        TCase* tcase_symlink = tcase_create ("/dev/mapper symlink");
-
-        /* Fail when an exception is raised */
-        ped_exception_set_handler (_test_exception_handler);
-
-        tcase_add_checked_fixture (tcase_symlink, create_disk, destroy_disk);
-        tcase_add_test (tcase_symlink, test_symlink);
-        /* Disable timeout for this test */
-        tcase_set_timeout (tcase_symlink, 0);
-        suite_add_tcase (suite, tcase_symlink);
-
-        SRunner* srunner = srunner_create (suite);
-        srunner_run_all (srunner, CK_VERBOSE);
-
-        number_failed = srunner_ntests_failed (srunner);
-        srunner_free (srunner);
-
-        return (number_failed == 0) ? EXIT_SUCCESS : EXIT_FAILURE;
-}
diff --git a/libparted/tests/t3000-symlink.sh b/libparted/tests/t3000-symlink.sh
deleted file mode 100755
index 338e44a..0000000
--- a/libparted/tests/t3000-symlink.sh
+++ /dev/null
@@ -1,27 +0,0 @@
-#!/bin/sh
-# run the /dev/mapper symlink test
-
-# Copyright (C) 2007-2013 Free Software Foundation, Inc.
-
-# This program is free software; you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation; either version 3 of the License, or
-# (at your option) any later version.
-
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-
-# You should have received a copy of the GNU General Public License
-# along with this program.  If not, see <http://www.gnu.org/licenses/>.
-
-. "${abs_top_srcdir=../..}/tests/init.sh"; path_prepend_ .
-
-. $abs_top_srcdir/tests/t-lib-helpers.sh
-# Need root privileges to create a symlink under /dev/mapper.
-require_root_
-
-symlink || fail=1
-
-Exit $fail
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 16ec5d2..c5472d2 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -63,6 +63,7 @@ TESTS = \
   t6001-psep.sh \
   t6002-dm-busy.sh \
   t6003-dm-hide.sh \
+  t6004-dm-symlink.sh \
   t6100-mdraid-partitions.sh \
   t7000-scripting.sh \
   t8000-loop.sh \
diff --git a/tests/t6000-dm.sh b/tests/t6000-dm.sh
index c301dee..241f34b 100755
--- a/tests/t6000-dm.sh
+++ b/tests/t6000-dm.sh
@@ -65,7 +65,7 @@ for type in linear ; do
 
   # setup: create a mapping
   echo "$dmsetup_cmd" | dmsetup create "$type_kwd" || fail=1
-  dev="$DM_DEV_DIR/mapper/$type_kwd"
+  dev="/dev/mapper/$type_kwd"
 
   # Create msdos partition table
   parted -s $dev mklabel msdos > out 2>&1 || fail=1
diff --git a/tests/t6004-dm-symlink.sh b/tests/t6004-dm-symlink.sh
new file mode 100644
index 0000000..0cb967b
--- /dev/null
+++ b/tests/t6004-dm-symlink.sh
@@ -0,0 +1,68 @@
+#!/bin/sh
+# ensure that parted canonicalizes symlinks to dm devices correctly
+
+# Copyright (C) 2008-2013 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+. "${srcdir=.}/init.sh"; path_prepend_ ../parted
+
+require_root_
+lvm_init_root_dir_
+
+test "x$ENABLE_DEVICE_MAPPER" = xyes \
+  || skip_ "no device-mapper support"
+
+# Device maps names - should be random to not conflict with existing ones on
+# the system
+linear_=plinear-$$
+
+d1=
+f1=
+ln1=
+cleanup_fn_() {
+    dmsetup remove $linear_
+    test -n "$d1" && losetup -d "$d1"
+    rm -f "$f1" "$ln1";
+}
+
+f1=$(pwd)/1; d1=$(loop_setup_ "$f1") \
+  || skip_ "is this partition mounted with 'nodev'?"
+
+dmsetup_cmd="0 1024 linear $d1 0"
+
+  # setup: create a mapping
+  echo "$dmsetup_cmd" | dmsetup create $linear_ || fail=1
+  dev="/dev/mapper/$linear_"
+  ln1=symlink
+  ln -s "$dev" symlink || fail=1
+
+  # Create msdos partition table
+  parted -s $dev mklabel msdos
+  parted -s symlink print > out 2>&1 || fail=1
+  # Create expected output file.
+  cat <<EOF >> exp || fail=1
+Model: Linux device-mapper (linear) (dm)
+Disk $dev: 524kB
+Sector size (logical/physical): 512B/512B
+Partition Table: msdos
+Disk Flags: 
+
+Number  Start  End  Size  Type  File system  Flags
+
+EOF
+
+  compare exp out || fail=1
+
+Exit $fail
-- 
1.8.1.2