utils: Check if resolved path in bd_utils_resolve_device is device

No point in trying to return something that isn't actually a block
device, like for example directories in /dev.

Author: vojtechtrefny

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/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