Check for non-existing/invalid/empty devices/paths in dm plugin and utils

src/plugins/dm.c

@@ -178,19 +178,21 @@ gboolean bd_dm_remove (const gchar *map_name, GError **error) {
 gchar* bd_dm_name_from_node (const gchar *dm_node, GError **error) {
     gchar *ret = NULL;
     gboolean success = FALSE;
+    g_autofree gchar *sys_path = g_strdup_printf ("/sys/class/block/%s/dm/name", dm_node);
 
-    gchar *sys_path = g_strdup_printf ("/sys/class/block/%s/dm/name", dm_node);
+    if (!dm_node || strlen (dm_node) == 0) {
+        g_set_error (error, BD_DM_ERROR, BD_DM_ERROR_DEVICE_NOEXIST,
+                     "No DM node specified");
+        return NULL;
+    }
 
     if (access (sys_path, R_OK) != 0) {
-        g_free (sys_path);
         g_set_error (error, BD_DM_ERROR, BD_DM_ERROR_SYS,
                      "Failed to access dm node's parameters under /sys");
         return NULL;
     }
 
     success = g_file_get_contents (sys_path, &ret, NULL, error);
-    g_free (sys_path);
-
     if (!success) {
         /* error is already populated */
         g_free (ret);
@@ -211,20 +213,21 @@ gchar* bd_dm_name_from_node (const gchar *dm_node, GError **error) {
  * Tech category: %BD_DM_TECH_MAP-%BD_DM_TECH_MODE_QUERY
  */
 gchar* bd_dm_node_from_name (const gchar *map_name, GError **error) {
-    gchar *dev_path = NULL;
-    gchar *ret = NULL;
-    gchar *dev_mapper_path = g_strdup_printf ("/dev/mapper/%s", map_name);
+    g_autofree gchar *dev_path = NULL;
+    g_autofree gchar *dev_mapper_path = g_strdup_printf ("/dev/mapper/%s", map_name);
+
+    if (!map_name || strlen (map_name) == 0) {
+        g_set_error (error, BD_DM_ERROR, BD_DM_ERROR_DEVICE_NOEXIST,
+                     "No DM name specified");
+        return NULL;
+    }
 
     dev_path = bd_utils_resolve_device (dev_mapper_path, error);
-    g_free (dev_mapper_path);
     if (!dev_path)
         /* error is already populated */
         return NULL;
 
-    ret = g_path_get_basename (dev_path);
-    g_free (dev_path);
-
-    return ret;
+    return g_path_get_basename (dev_path);
 }
 
 /**

src/utils/dev_utils.c

@@ -19,6 +19,8 @@
 
 #include <glib.h>
 #include <libudev.h>
+#include <sys/stat.h>
+#include <errno.h>
 
 #include "dev_utils.h"
 
@@ -30,6 +32,24 @@ GQuark bd_utils_dev_utils_error_quark (void)
     return g_quark_from_static_string ("g-bd-utils-dev_utils-error-quark");
 }
 
+static gboolean _is_block_device (const gchar *path, GError **error) {
+    struct stat sb;
+
+    if (stat (path, &sb) != 0) {
+        g_set_error (error, BD_UTILS_DEV_UTILS_ERROR, BD_UTILS_DEV_UTILS_ERROR_FAILED,
+                     "Failed to stat '%s': %s", path, strerror_l (errno, _C_LOCALE));
+        return FALSE;
+    }
+
+    if (!S_ISBLK (sb.st_mode)) {
+        g_set_error (error, BD_UTILS_DEV_UTILS_ERROR, BD_UTILS_DEV_UTILS_ERROR_FAILED,
+                     "'%s' is not a block device", path);
+        return FALSE;
+    }
+
+    return TRUE;
+}
+
 /**
  * bd_utils_resolve_device:
  * @dev_spec: specification of the device (e.g. "/dev/sda", any symlink, or the name of a file
@@ -40,12 +60,10 @@ GQuark bd_utils_dev_utils_error_quark (void)
  *                           for "/dev/md/my_raid") or %NULL in case of error
  */
 gchar* bd_utils_resolve_device (const gchar *dev_spec, GError **error) {
-    gchar *path = NULL;
-    gchar *symlink = NULL;
+    g_autofree gchar *path = NULL;
+    g_autofree gchar *symlink = NULL;
     GError *l_error = NULL;
 
-    /* TODO: check that the resulting path is a block device? */
-
     if (!g_str_has_prefix (dev_spec, "/dev/"))
         path = g_strdup_printf ("/dev/%s", dev_spec);
     else
@@ -56,11 +74,15 @@ gchar* bd_utils_resolve_device (const gchar *dev_spec, GError **error) {
         if (g_error_matches (l_error, G_FILE_ERROR, G_FILE_ERROR_INVAL)) {
             /* invalid argument -> not a symlink -> nothing to resolve */
             g_clear_error (&l_error);
-            return path;
+            if (_is_block_device (path, error))
+                return g_steal_pointer (&path);
+            else {
+                g_prefix_error (error, "Failed to resolve '%s': ", dev_spec);
+                return NULL;
+            }
         } else {
             /* some other error, just report it */
             g_propagate_error (error, l_error);
-            g_free (path);
             return NULL;
         }
     }
@@ -70,9 +92,13 @@ gchar* bd_utils_resolve_device (const gchar *dev_spec, GError **error) {
         path = g_strdup_printf ("/dev/%s", symlink + 3);
     else
         path = g_strdup_printf ("/dev/%s", symlink);
-    g_free (symlink);
 
-    return path;
+    if (_is_block_device (path, error))
+        return g_steal_pointer (&path);
+    else {
+        g_prefix_error (error, "Failed to resolve '%s': ", dev_spec);
+        return NULL;
+    }
 }
 
 /**

src/utils/dev_utils.h

@@ -25,6 +25,9 @@
 GQuark bd_utils_dev_utils_error_quark (void);
 #define BD_UTILS_DEV_UTILS_ERROR bd_utils_dev_utils_error_quark ()
 
+/* "C" locale to get the locale-agnostic error messages */
+#define _C_LOCALE (locale_t) 0
+
 typedef enum {
     BD_UTILS_DEV_UTILS_ERROR_FAILED,
 } BDUtilsDevUtilsError;

tests/dm_test.py

@@ -153,6 +153,12 @@ def test_name_node_bijection(self):
         self.assertEqual(BlockDev.dm_name_from_node(BlockDev.dm_node_from_name("testMap")),
                          "testMap")
 
+        with self.assertRaisesRegex(GLib.GError, "No DM name specified"):
+            BlockDev.dm_node_from_name("")
+
+        with self.assertRaisesRegex(GLib.GError, "No DM node specified"):
+            BlockDev.dm_name_from_node("")
+
 
 class DMNoStorageTest(DevMapperTest):
     @tag_test(TestTags.NOSTORAGE)

tests/utils_test.py

@@ -406,57 +406,74 @@ def test_exec_large_input(self):
         self.assertTrue(status)
 
 
-class UtilsDevUtilsTestCase(UtilsTestCase):
-    @tag_test(TestTags.NOSTORAGE, TestTags.CORE)
+class UtilsStorageTestCase(UtilsTestCase):
+    def setUp(self):
+        self.addCleanup(self._clean_up)
+        self.dev_file = create_sparse_tempfile("utils_test", 1024**3)
+        try:
+            self.loop_dev = create_lio_device(self.dev_file)
+        except RuntimeError as e:
+            raise RuntimeError("Failed to setup loop device for testing: %s" % e)
+
+    def _clean_up(self):
+        try:
+            delete_lio_device(self.loop_dev)
+        except RuntimeError:
+            # just move on, we can do no better here
+            pass
+        os.unlink(self.dev_file)
+
+    def _setup_lvm(self):
+        ret, _out, _err = run_command ("pvcreate %s --config \"devices {use_devicesfile = 0}\"" % self.loop_dev)
+        self.assertEqual(ret, 0)
+        self.addCleanup(run_command, "pvremove %s --config \"devices {use_devicesfile = 0}\"" % self.loop_dev)
+
+        ret, _out, _err = run_command ("vgcreate utilsTestVG %s --config \"devices {use_devicesfile = 0}\"" % self.loop_dev)
+        self.assertEqual(ret, 0)
+        self.addCleanup(run_command, "vgremove -y utilsTestVG --config \"devices {use_devicesfile = 0}\"")
+
+        ret, _out, _err = run_command ("lvcreate -n utilsTestLV -L 12M utilsTestVG --config \"devices {use_devicesfile = 0}\"")
+        self.assertEqual(ret, 0)
+        self.addCleanup(run_command, "lvremove -y utilsTestVG/utilsTestLV --config \"devices {use_devicesfile = 0}\"")
+
+
+class UtilsDevUtilsTestCase(UtilsStorageTestCase):
+    @tag_test(TestTags.CORE)
     def test_resolve_device(self):
         """Verify that resolving device spec works as expected"""
 
         with self.assertRaises(GLib.GError):
             BlockDev.utils_resolve_device("no_such_device")
 
-        dev = "/dev/libblockdev-test-dev"
-        with open(dev, "w"):
-            pass
-        self.addCleanup(os.unlink, dev)
+        self._setup_lvm()
+        dev_link = "/dev/mapper/utilsTestVG-utilsTestLV"
+        dev_link2 = "/dev/utilsTestVG/utilsTestLV"
+        dev_node = os.path.realpath(dev_link)
 
         # full path, no symlink, should just return the same
-        self.assertEqual(BlockDev.utils_resolve_device(dev), dev)
+        self.assertEqual(BlockDev.utils_resolve_device(dev_node), dev_node)
 
         # just the name of the device, should return the full path
-        self.assertEqual(BlockDev.utils_resolve_device(dev[5:]), dev)
-
-        dev_dir = "/dev/libblockdev-test-dir"
-        os.mkdir(dev_dir)
-        self.addCleanup(os.rmdir, dev_dir)
-
-        dev_link = dev_dir + "/test-dev-link"
-        os.symlink("../" + dev[5:], dev_link)
-        self.addCleanup(os.unlink, dev_link)
+        self.assertEqual(BlockDev.utils_resolve_device(dev_node[5:]), dev_node)
 
         # should resolve the symlink
-        self.assertEqual(BlockDev.utils_resolve_device(dev_link), dev)
+        self.assertEqual(BlockDev.utils_resolve_device(dev_link), dev_node)
 
         # should resolve the symlink even without the "/dev" prefix
-        self.assertEqual(BlockDev.utils_resolve_device(dev_link[5:]), dev)
+        self.assertEqual(BlockDev.utils_resolve_device(dev_link[5:]), dev_node)
 
+        # should resolve the other symlink too
+        self.assertEqual(BlockDev.utils_resolve_device(dev_link2), dev_node)
 
-class UtilsDevUtilsSymlinksTestCase(UtilsTestCase):
-    def setUp(self):
-        self.addCleanup(self._clean_up)
-        self.dev_file = create_sparse_tempfile("lvm_test", 1024**3)
-        try:
-            self.loop_dev = create_lio_device(self.dev_file)
-        except RuntimeError as e:
-            raise RuntimeError("Failed to setup loop device for testing: %s" % e)
+        # should resolve the other symlink even without the "/dev" prefix
+        self.assertEqual(BlockDev.utils_resolve_device(dev_link2[5:]), dev_node)
 
-    def _clean_up(self):
-        try:
-            delete_lio_device(self.loop_dev)
-        except RuntimeError:
-            # just move on, we can do no better here
-            pass
-        os.unlink(self.dev_file)
+        # vg is not a block device, shouldn't be resolved
+        with self.assertRaisesRegex(GLib.GError, "not a block device"):
+            BlockDev.utils_resolve_device("/dev/utilsTestVG")
 
+
+class UtilsDevUtilsSymlinksTestCase(UtilsStorageTestCase):
     @tag_test(TestTags.CORE)
     def test_get_device_symlinks(self):
         """Verify that getting device symlinks works as expected"""
@@ -472,17 +489,7 @@ def test_get_device_symlinks(self):
         self.assertGreaterEqual(len(symlinks), 2)
 
         # create an LV to get a device with more symlinks
-        ret, _out, _err = run_command ("pvcreate %s --config \"devices {use_devicesfile = 0}\"" % self.loop_dev)
-        self.assertEqual(ret, 0)
-        self.addCleanup(run_command, "pvremove %s --config \"devices {use_devicesfile = 0}\"" % self.loop_dev)
-
-        ret, _out, _err = run_command ("vgcreate utilsTestVG %s --config \"devices {use_devicesfile = 0}\"" % self.loop_dev)
-        self.assertEqual(ret, 0)
-        self.addCleanup(run_command, "vgremove -y utilsTestVG --config \"devices {use_devicesfile = 0}\"")
-
-        ret, _out, _err = run_command ("lvcreate -n utilsTestLV -L 12M utilsTestVG --config \"devices {use_devicesfile = 0}\"")
-        self.assertEqual(ret, 0)
-        self.addCleanup(run_command, "lvremove -y utilsTestVG/utilsTestLV --config \"devices {use_devicesfile = 0}\"")
+        self._setup_lvm()
 
         symlinks = BlockDev.utils_get_device_symlinks("utilsTestVG/utilsTestLV")
         # there should be at least 4 symlinks for an LV