[libvirt] [PATCH 08/10] qemuDomainCreateDevice: Don't loop endlessly

[Martin Kletzander]

On Fri, Jan 20, 2017 at 10:42:48AM +0100, Michal Privoznik wrote:
>When working with symlinks it is fairly easy to get into a loop.
>Don't.
>
>Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>---
> src/qemu/qemu_domain.c | 28 ++++++++++++++++++++++++----
> 1 file changed, 24 insertions(+), 4 deletions(-)
>
>diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>index 8cbfb2d16..448583313 100644
>--- a/src/qemu/qemu_domain.c
>+++ b/src/qemu/qemu_domain.c
>@@ -6954,9 +6954,10 @@ qemuDomainGetPreservedMounts(virQEMUDriverPtr driver,
>
>
> static int
>-qemuDomainCreateDevice(const char *device,
>-                       const char *path,
>-                       bool allow_noent)
>+qemuDomainCreateDeviceRecursive(const char *device,
>+                                const char *path,
>+                                bool allow_noent,
>+                                unsigned int ttl)
> {
>     char *devicePath = NULL;
>     char *target = NULL;
>@@ -6968,6 +6969,13 @@ qemuDomainCreateDevice(const char *device,
>     char *tcon = NULL;
> #endif
>
>+    if (!ttl) {
>+        virReportSystemError(ELOOP,
>+                             _("Too many levels of symbolic links: %s"),
>+                             device);
>+        return ret;
>+    }
>+
>     if (lstat(device, &sb) < 0) {
>         if (errno == ENOENT && allow_noent) {
>             /* Ignore non-existent device. */
>@@ -7057,7 +7065,8 @@ qemuDomainCreateDevice(const char *device,
>             tmp = NULL;
>         }
>
>-        if (qemuDomainCreateDevice(target, path, allow_noent) < 0)
>+        if (qemuDomainCreateDeviceRecursive(target, path,
>+                                            allow_noent, ttl - 1) < 0)
>             goto cleanup;
>     } else {
>         if (create &&
>@@ -7128,6 +7137,17 @@ qemuDomainCreateDevice(const char *device,
> }
>
>
>+static int
>+qemuDomainCreateDevice(const char *device,
>+                       const char *path,
>+                       bool allow_noent)
>+{
>+    long symloop_max = sysconf(_SC_SYMLOOP_MAX);
>+
>+    return qemuDomainCreateDeviceRecursive(device, path,
>+                                           allow_noent, symloop_max);

So you are taking 'long' that can, officially, be '-1' and pass it to a
function as unsigned int.  Having a maximum is nice, I have no idea why
on my system there is no limit apparently (sysconf() returns -1 with no
errno set), but if the system doesn't report any (like my case above) it
effectively makes the SYMLOOP_MAX = UINT_MAX in this codepath.  Should
we avoid that or just let it be?  This way it's still better than
unlimited, so ACK to this version, but I just wanted a (short)
discussion about this.

>+}
>+
>
> static int
> qemuDomainPopulateDevices(virQEMUDriverPtr driver,
>--
>2.11.0
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170207/705877e2/attachment-0001.sig>