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

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(a)redhat.com&gt; > --- > 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.