Re: [PATCH parted 2/2] libparted: Don't canonicalize /dev/mapper paths

[Jim Meyering]

Hans de Goede wrote:
> Besides fixing the issue displayed by libparted/tests/symlink.c,
> this has the added advantage that the output of parted p on one of these
> devices now says:
> "Disk /dev/mapper/BigVol2-lv_iscsi_disk2: 34.4GB"
>
> Which is a lot more user friendly then the output before this patch:
> "Disk /dev/dm-6: 34.4GB"
>
> * libparted/device.c: Don't canonicalize /dev/mapper paths

Thanks!
I've added a NEWS entry, adjusted the commit log to attribute Ales
and mention the BZ#, and put them in the opposite order so "make check"
passes after each commit:


>From c1eb485b9fd8919e18f192d678bc52b0488e6ee0 Mon Sep 17 00:00:00 2001
From: Hans de Goede <address@hidden>
Date: Tue, 6 Apr 2010 15:57:18 +0200
Subject: [PATCH 1/2] libparted: don't canonicalize /dev/mapper paths

Besides fixing the issue displayed by libparted/tests/symlink.c,
this has the added advantage that the output of parted p on one of these
devices now says:
"Disk /dev/mapper/BigVol2-lv_iscsi_disk2: 34.4GB"

Which is a lot more user friendly then the output before this patch:
"Disk /dev/dm-6: 34.4GB"

* libparted/device.c (ped_device_get): Don't canonicalize names
that start with "/dev/mapper/".
* NEWS (Bug fixes): Mention it.
Thanks to Ales Kozumplik for the analysis.
Details in <http://bugzilla.redhat.com/577824>.
---
 NEWS               |    5 +++++
 libparted/device.c |    6 ++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/NEWS b/NEWS
index 1c7ad4a..2471b8b 100644
--- a/NEWS
+++ b/NEWS
@@ -8,6 +8,11 @@ GNU parted NEWS                                    -*- outline 
-*-

 ** Bug fixes

+  When libparted deferenced a /dev/mapper/foo symlink, it would keep the
+  resulting /dev/dm-N name and sometimes use it later, even though it
+  had since become stale and invalid.  It no longer stores the result
+  of dereferencing a /dev/mapper symlink.
+
   libparted's msdos_partition_is_flag_available function now always reports
   that the "hidden" flag is not available for an extended partition.
   Similarly, msdos_partition_get_flag(p,PED_PARTITION_HIDDEN) always returns 0
diff --git a/libparted/device.c b/libparted/device.c
index 0f36a03..64da978 100644
--- a/libparted/device.c
+++ b/libparted/device.c
@@ -139,10 +139,12 @@ PedDevice*
 ped_device_get (const char* path)
 {
        PedDevice*      walk;
-       char*           normal_path;
+       char*           normal_path = NULL;

        PED_ASSERT (path != NULL, return NULL);
-       normal_path = canonicalize_file_name (path);
+       /* Don't canonicalize /dev/mapper paths, see tests/symlink.c */
+       if (strncmp (path, "/dev/mapper/", 12))
+               normal_path = canonicalize_file_name (path);
        if (!normal_path)
                /* Well, maybe it is just that the file does not exist.
                 * Try it anyway.  */
--
1.7.0.4.552.gc303


>From 85506df997edba889cf8b475df3b854f4887e9bb Mon Sep 17 00:00:00 2001
From: Hans de Goede <address@hidden>
Date: Tue, 6 Apr 2010 15:57:17 +0200
Subject: [PATCH 2/2] libparted: add test for /dev/mapper symlink issue

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.

* libparted/tests/symlink.c: New test which tests for this issue.
* libparted/tests/t3000-symlink.sh: New file.
* libparted/tests/Makefile.am: Include the new files.  Run the new test.
---
 libparted/tests/Makefile.am      |    5 +-
 libparted/tests/symlink.c        |  135 ++++++++++++++++++++++++++++++++++++++
 libparted/tests/t3000-symlink.sh |   29 ++++++++
 3 files changed, 167 insertions(+), 2 deletions(-)
 create mode 100644 libparted/tests/symlink.c
 create mode 100755 libparted/tests/t3000-symlink.sh

diff --git a/libparted/tests/Makefile.am b/libparted/tests/Makefile.am
index 2f63741..b433c83 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
+TESTS = t1000-label.sh t2000-disk.sh t3000-symlink.sh
 EXTRA_DIST = $(TESTS)
-check_PROGRAMS = label disk
+check_PROGRAMS = label disk symlink
 AM_CFLAGS = $(WARN_CFLAGS) $(WERROR_CFLAGS)

 LDADD = \
@@ -19,6 +19,7 @@ AM_CPPFLAGS = \

 label_SOURCES = common.h common.c label.c
 disk_SOURCES  = common.h common.c disk.c
+symlink_SOURCES = common.h common.c symlink.c

 MAINTAINERCLEANFILES = Makefile.in

diff --git a/libparted/tests/symlink.c b/libparted/tests/symlink.c
new file mode 100644
index 0000000..52e99ca
--- /dev/null
+++ b/libparted/tests/symlink.c
@@ -0,0 +1,135 @@
+/* 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
new file mode 100755
index 0000000..9c64833
--- /dev/null
+++ b/libparted/tests/t3000-symlink.sh
@@ -0,0 +1,29 @@
+#!/bin/sh
+
+# Copyright (C) 2007-2010 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/>.
+
+test_description='run the /dev/mapper symlink test'
+
+# Need root privileges to create a symlink under /dev/mapper.
+privileges_required_=1
+
+: ${top_srcdir=../..}
+. "$top_srcdir/tests/test-lib.sh"
+
+test_expect_success \
+    'run the actual tests' 'symlink'
+
+test_done
--
1.7.0.4.552.gc303