[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On 02/07/2017 11:34 AM, Martin Kletzander wrote:
> 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 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.

Sure. I think it's safe to assume if the level of symlinks reaches
UINT_MAX your system is terribly broken.

Michal


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]