[PATCH 1/3] nouveau: cleanup error handling during nouveau_device_wrap

[PATCH 1/3] nouveau: cleanup error handling during nouveau_device_wrap

[Emil Velikov]

In theory it's possible for any of the nouveau_getparam calls to
fail whist the last one being successful.

Thus at least one of the following (hard requirements) drmVersion,
chipset and vram/gart memory size will be filled with garbage and
sent to the userspace drivers.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 nouveau/nouveau.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c
index ee7893b..d6013db 100644
--- a/nouveau/nouveau.c
+++ b/nouveau/nouveau.c
@@ -88,27 +88,32 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev)
 	nvdev->base.fd = fd;
 
 	ver = drmGetVersion(fd);
-	if (ver) dev->drm_version = (ver->version_major << 24) |
-				    (ver->version_minor << 8) |
-				     ver->version_patchlevel;
+	if (!ver) {
+		ret = -errno;
+		goto error;
+	}
+
+	dev->drm_version = (ver->version_major << 24) |
+			    (ver->version_minor << 8) |
+			     ver->version_patchlevel;
 	drmFreeVersion(ver);
 
 	if ( dev->drm_version != 0x00000010 &&
 	    (dev->drm_version <  0x01000000 ||
 	     dev->drm_version >= 0x02000000)) {
-		nouveau_device_del(&dev);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto error;
 	}
 
 	ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_CHIPSET_ID, &chipset);
-	if (ret == 0)
+	if (ret)
+		goto error;
 	ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_FB_SIZE, &vram);
-	if (ret == 0)
+	if (ret)
+		goto error;
 	ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_AGP_SIZE, &gart);
-	if (ret) {
-		nouveau_device_del(&dev);
-		return ret;
-	}
+	if (ret)
+		goto error;
 
 	ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_HAS_BO_USAGE, &bousage);
 	if (ret == 0)
@@ -139,6 +144,9 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev)
 
 	*pdev = &nvdev->base;
 	return 0;
+error:
+	nouveau_device_del(&dev);
+	return -ret;
 }
 
 int
-- 
1.9.0

[PATCH 2/3] nouveau: sanitise NOUVEAU_LIBDRM_*_LIMIT_PERCENT input

[Emil Velikov]

Current handling relies on atoi which does not detect errors
additionally, any integer value will be considered as a valid
percent.

Resolve that by using strtol and limiting the value within
the 5-100 (percent) range.

Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 nouveau/nouveau.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c
index d6013db..06cd804 100644
--- a/nouveau/nouveau.c
+++ b/nouveau/nouveau.c
@@ -76,7 +76,7 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev)
 	struct nouveau_device *dev = &nvdev->base;
 	uint64_t chipset, vram, gart, bousage;
 	drmVersionPtr ver;
-	int ret;
+	int ret, limit;
 	char *tmp;
 
 #ifdef DEBUG
@@ -121,16 +121,22 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev)
 
 	nvdev->close = close;
 
+	nvdev->vram_limit_percent = 80;
 	tmp = getenv("NOUVEAU_LIBDRM_VRAM_LIMIT_PERCENT");
-	if (tmp)
-		nvdev->vram_limit_percent = atoi(tmp);
-	else
-		nvdev->vram_limit_percent = 80;
+	if (tmp) {
+		limit = strtol(tmp, NULL, 10);
+		if (limit >= 5 && limit <= 100)
+			nvdev->vram_limit_percent = limit;
+	}
+
+	nvdev->gart_limit_percent = 80;
 	tmp = getenv("NOUVEAU_LIBDRM_GART_LIMIT_PERCENT");
-	if (tmp)
-		nvdev->gart_limit_percent = atoi(tmp);
-	else
-		nvdev->gart_limit_percent = 80;
+	if (tmp) {
+		limit = strtol(tmp, NULL, 10);
+		if (limit >= 5 && limit <= 100)
+			nvdev->gart_limit_percent = limit;
+	}
+
 	DRMINITLISTHEAD(&nvdev->bo_list);
 	nvdev->base.object.oclass = NOUVEAU_DEVICE_CLASS;
 	nvdev->base.lib_version = 0x01000000;
-- 
1.9.0

Re: [PATCH 2/3] nouveau: sanitise NOUVEAU_LIBDRM_*_LIMIT_PERCENT input

[Ilia Mirkin]

On Wed, Mar 12, 2014 at 4:45 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> Current handling relies on atoi which does not detect errors
> additionally, any integer value will be considered as a valid
> percent.
>
> Resolve that by using strtol and limiting the value within
> the 5-100 (percent) range.
>
> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
> ---
>  nouveau/nouveau.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c
> index d6013db..06cd804 100644
> --- a/nouveau/nouveau.c
> +++ b/nouveau/nouveau.c
> @@ -76,7 +76,7 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev)
>         struct nouveau_device *dev = &nvdev->base;
>         uint64_t chipset, vram, gart, bousage;
>         drmVersionPtr ver;
> -       int ret;
> +       int ret, limit;
>         char *tmp;
>
>  #ifdef DEBUG
> @@ -121,16 +121,22 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev)
>
>         nvdev->close = close;
>
> +       nvdev->vram_limit_percent = 80;
>         tmp = getenv("NOUVEAU_LIBDRM_VRAM_LIMIT_PERCENT");
> -       if (tmp)
> -               nvdev->vram_limit_percent = atoi(tmp);
> -       else
> -               nvdev->vram_limit_percent = 80;
> +       if (tmp) {
> +               limit = strtol(tmp, NULL, 10);
> +               if (limit >= 5 && limit <= 100)
> +                       nvdev->vram_limit_percent = limit;
> +       }

Wouldn't it be easier to just clamp the value no matter what? i.e.
leave the current code alone, and add a clamp helper function + use
it? These are pretty rare env vars to set... presumably the person
setting them knows what they're doing. And if not, they get what they
deserve.

On a related topic, I'd personally rather not throw in arbitrary
restrictions to code like this -- it makes debugging harder later on.
(e.g. what's wrong with 0 percent? let's say i want to disable
gart/vram entirely?)

> +
> +       nvdev->gart_limit_percent = 80;
>         tmp = getenv("NOUVEAU_LIBDRM_GART_LIMIT_PERCENT");
> -       if (tmp)
> -               nvdev->gart_limit_percent = atoi(tmp);
> -       else
> -               nvdev->gart_limit_percent = 80;
> +       if (tmp) {
> +               limit = strtol(tmp, NULL, 10);
> +               if (limit >= 5 && limit <= 100)
> +                       nvdev->gart_limit_percent = limit;
> +       }
> +
>         DRMINITLISTHEAD(&nvdev->bo_list);
>         nvdev->base.object.oclass = NOUVEAU_DEVICE_CLASS;
>         nvdev->base.lib_version = 0x01000000;
> --
> 1.9.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] nouveau: sanitise NOUVEAU_LIBDRM_*_LIMIT_PERCENT input

[Emil Velikov]

On 13/03/14 00:49, Ilia Mirkin wrote:
> On Wed, Mar 12, 2014 at 4:45 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>> Current handling relies on atoi which does not detect errors
>> additionally, any integer value will be considered as a valid
>> percent.
>>
>> Resolve that by using strtol and limiting the value within
>> the 5-100 (percent) range.
>>
>> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
>> ---
>>   nouveau/nouveau.c | 24 +++++++++++++++---------
>>   1 file changed, 15 insertions(+), 9 deletions(-)
>>
>> diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c
>> index d6013db..06cd804 100644
>> --- a/nouveau/nouveau.c
>> +++ b/nouveau/nouveau.c
>> @@ -76,7 +76,7 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev)
>>          struct nouveau_device *dev = &nvdev->base;
>>          uint64_t chipset, vram, gart, bousage;
>>          drmVersionPtr ver;
>> -       int ret;
>> +       int ret, limit;
>>          char *tmp;
>>
>>   #ifdef DEBUG
>> @@ -121,16 +121,22 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev)
>>
>>          nvdev->close = close;
>>
>> +       nvdev->vram_limit_percent = 80;
>>          tmp = getenv("NOUVEAU_LIBDRM_VRAM_LIMIT_PERCENT");
>> -       if (tmp)
>> -               nvdev->vram_limit_percent = atoi(tmp);
>> -       else
>> -               nvdev->vram_limit_percent = 80;
>> +       if (tmp) {
>> +               limit = strtol(tmp, NULL, 10);
>> +               if (limit >= 5 && limit <= 100)
>> +                       nvdev->vram_limit_percent = limit;
>> +       }
>
> Wouldn't it be easier to just clamp the value no matter what? i.e.
> leave the current code alone, and add a clamp helper function + use
> it? These are pretty rare env vars to set... presumably the person
> setting them knows what they're doing. And if not, they get what they
> deserve.
>
> On a related topic, I'd personally rather not throw in arbitrary
> restrictions to code like this -- it makes debugging harder later on.
> (e.g. what's wrong with 0 percent? let's say i want to disable
> gart/vram entirely?)
>

Valid arguments, thanks.

-Emil
>> +
>> +       nvdev->gart_limit_percent = 80;
>>          tmp = getenv("NOUVEAU_LIBDRM_GART_LIMIT_PERCENT");
>> -       if (tmp)
>> -               nvdev->gart_limit_percent = atoi(tmp);
>> -       else
>> -               nvdev->gart_limit_percent = 80;
>> +       if (tmp) {
>> +               limit = strtol(tmp, NULL, 10);
>> +               if (limit >= 5 && limit <= 100)
>> +                       nvdev->gart_limit_percent = limit;
>> +       }
>> +
>>          DRMINITLISTHEAD(&nvdev->bo_list);
>>          nvdev->base.object.oclass = NOUVEAU_DEVICE_CLASS;
>>          nvdev->base.lib_version = 0x01000000;
>> --
>> 1.9.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] freedreno: do not leak drmVersion

[Emil Velikov]

Cc: Rob Clark <robclark@freedesktop.org>
Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 freedreno/freedreno_device.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/freedreno/freedreno_device.c b/freedreno/freedreno_device.c
index 598bdfb..c34963c 100644
--- a/freedreno/freedreno_device.c
+++ b/freedreno/freedreno_device.c
@@ -98,6 +98,7 @@ struct fd_device * fd_device_new(int fd)
 		ERROR_MSG("unknown device: %s", version->name);
 		dev = NULL;
 	}
+	drmFreeVersion(version);
 
 	if (!dev)
 		return NULL;
-- 
1.9.0

Re: [PATCH 3/3] freedreno: do not leak drmVersion

[Rob Clark]

On Wed, Mar 12, 2014 at 4:45 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> Cc: Rob Clark <robclark@freedesktop.org>

Thanks,

Reviewed-by: Rob Clark <robdclark@gmail.com>

> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
> ---
>  freedreno/freedreno_device.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/freedreno/freedreno_device.c b/freedreno/freedreno_device.c
> index 598bdfb..c34963c 100644
> --- a/freedreno/freedreno_device.c
> +++ b/freedreno/freedreno_device.c
> @@ -98,6 +98,7 @@ struct fd_device * fd_device_new(int fd)
>                 ERROR_MSG("unknown device: %s", version->name);
>                 dev = NULL;
>         }
> +       drmFreeVersion(version);
>
>         if (!dev)
>                 return NULL;
> --
> 1.9.0
>

Re: [PATCH 1/3] nouveau: cleanup error handling during nouveau_device_wrap

[Ilia Mirkin]

On Wed, Mar 12, 2014 at 4:45 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> In theory it's possible for any of the nouveau_getparam calls to
> fail whist the last one being successful.
>
> Thus at least one of the following (hard requirements) drmVersion,
> chipset and vram/gart memory size will be filled with garbage and
> sent to the userspace drivers.

What was wrong with the old logic again? Except annoying indentation?

ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_CHIPSET_ID, &chipset);
if (ret == 0)
ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_FB_SIZE, &vram);
if (ret == 0)
ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_AGP_SIZE, &gart);
if (ret) {
nouveau_device_del(&dev);
return ret;
}

It will only run each successive getparam if the previous one succeeded...


>
> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
> ---
>  nouveau/nouveau.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c
> index ee7893b..d6013db 100644
> --- a/nouveau/nouveau.c
> +++ b/nouveau/nouveau.c
> @@ -88,27 +88,32 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev)
>         nvdev->base.fd = fd;
>
>         ver = drmGetVersion(fd);
> -       if (ver) dev->drm_version = (ver->version_major << 24) |
> -                                   (ver->version_minor << 8) |
> -                                    ver->version_patchlevel;
> +       if (!ver) {
> +               ret = -errno;
> +               goto error;
> +       }
> +
> +       dev->drm_version = (ver->version_major << 24) |
> +                           (ver->version_minor << 8) |
> +                            ver->version_patchlevel;
>         drmFreeVersion(ver);
>
>         if ( dev->drm_version != 0x00000010 &&
>             (dev->drm_version <  0x01000000 ||
>              dev->drm_version >= 0x02000000)) {
> -               nouveau_device_del(&dev);
> -               return -EINVAL;
> +               ret = -EINVAL;
> +               goto error;
>         }
>
>         ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_CHIPSET_ID, &chipset);
> -       if (ret == 0)
> +       if (ret)
> +               goto error;
>         ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_FB_SIZE, &vram);
> -       if (ret == 0)
> +       if (ret)
> +               goto error;
>         ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_AGP_SIZE, &gart);
> -       if (ret) {
> -               nouveau_device_del(&dev);
> -               return ret;
> -       }
> +       if (ret)
> +               goto error;
>
>         ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_HAS_BO_USAGE, &bousage);
>         if (ret == 0)
> @@ -139,6 +144,9 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev)
>
>         *pdev = &nvdev->base;
>         return 0;
> +error:
> +       nouveau_device_del(&dev);
> +       return -ret;

you mean 'ret' of course.

>  }
>
>  int
> --
> 1.9.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] nouveau: cleanup error handling during nouveau_device_wrap

[Emil Velikov]

On 13/03/14 00:45, Ilia Mirkin wrote:
> On Wed, Mar 12, 2014 at 4:45 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>> In theory it's possible for any of the nouveau_getparam calls to
>> fail whist the last one being successful.
>>
>> Thus at least one of the following (hard requirements) drmVersion,
>> chipset and vram/gart memory size will be filled with garbage and
>> sent to the userspace drivers.
>
> What was wrong with the old logic again? Except annoying indentation?
>
> ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_CHIPSET_ID, &chipset);
> if (ret == 0)
> ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_FB_SIZE, &vram);
> if (ret == 0)
> ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_AGP_SIZE, &gart);
> if (ret) {
> nouveau_device_del(&dev);
> return ret;
> }
>
> It will only run each successive getparam if the previous one succeeded...
>
Good point, the lovely indentation got me. So it seems only !ver 
handling is needed.

>
>>
>> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
>> ---
>>   nouveau/nouveau.c | 30 +++++++++++++++++++-----------
>>   1 file changed, 19 insertions(+), 11 deletions(-)
>>
>> diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c
>> index ee7893b..d6013db 100644
>> --- a/nouveau/nouveau.c
>> +++ b/nouveau/nouveau.c
>> @@ -88,27 +88,32 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev)
>>          nvdev->base.fd = fd;
>>
>>          ver = drmGetVersion(fd);
>> -       if (ver) dev->drm_version = (ver->version_major << 24) |
>> -                                   (ver->version_minor << 8) |
>> -                                    ver->version_patchlevel;
>> +       if (!ver) {
>> +               ret = -errno;
>> +               goto error;
>> +       }
>> +
>> +       dev->drm_version = (ver->version_major << 24) |
>> +                           (ver->version_minor << 8) |
>> +                            ver->version_patchlevel;
>>          drmFreeVersion(ver);
>>
>>          if ( dev->drm_version != 0x00000010 &&
>>              (dev->drm_version <  0x01000000 ||
>>               dev->drm_version >= 0x02000000)) {
>> -               nouveau_device_del(&dev);
>> -               return -EINVAL;
>> +               ret = -EINVAL;
>> +               goto error;
>>          }
>>
>>          ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_CHIPSET_ID, &chipset);
>> -       if (ret == 0)
>> +       if (ret)
>> +               goto error;
>>          ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_FB_SIZE, &vram);
>> -       if (ret == 0)
>> +       if (ret)
>> +               goto error;
>>          ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_AGP_SIZE, &gart);
>> -       if (ret) {
>> -               nouveau_device_del(&dev);
>> -               return ret;
>> -       }
>> +       if (ret)
>> +               goto error;
>>
>>          ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_HAS_BO_USAGE, &bousage);
>>          if (ret == 0)
>> @@ -139,6 +144,9 @@ nouveau_device_wrap(int fd, int close, struct nouveau_device **pdev)
>>
>>          *pdev = &nvdev->base;
>>          return 0;
>> +error:
>> +       nouveau_device_del(&dev);
>> +       return -ret;
>
> you mean 'ret' of course.
>
Yikes, thanks.

-Emil

>>   }
>>
>>   int
>> --
>> 1.9.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] nouveau: cleanup error handling during nouveau_device_wrap

[Ilia Mirkin]

On Wed, Mar 12, 2014 at 9:04 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 13/03/14 00:45, Ilia Mirkin wrote:
>>
>> On Wed, Mar 12, 2014 at 4:45 PM, Emil Velikov <emil.l.velikov@gmail.com>
>> wrote:
>>>
>>> In theory it's possible for any of the nouveau_getparam calls to
>>> fail whist the last one being successful.
>>>
>>> Thus at least one of the following (hard requirements) drmVersion,
>>> chipset and vram/gart memory size will be filled with garbage and
>>> sent to the userspace drivers.
>>
>>
>> What was wrong with the old logic again? Except annoying indentation?
>>
>> ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_CHIPSET_ID, &chipset);
>> if (ret == 0)
>> ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_FB_SIZE, &vram);
>> if (ret == 0)
>> ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_AGP_SIZE, &gart);
>> if (ret) {
>> nouveau_device_del(&dev);
>> return ret;
>> }
>>
>> It will only run each successive getparam if the previous one succeeded...
>>
> Good point, the lovely indentation got me. So it seems only !ver handling is
> needed.

Not really. drm->drm_version will be 0 if ver fails.

>
>
>>
>>>
>>> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
>>> ---
>>>   nouveau/nouveau.c | 30 +++++++++++++++++++-----------
>>>   1 file changed, 19 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c
>>> index ee7893b..d6013db 100644
>>> --- a/nouveau/nouveau.c
>>> +++ b/nouveau/nouveau.c
>>> @@ -88,27 +88,32 @@ nouveau_device_wrap(int fd, int close, struct
>>> nouveau_device **pdev)
>>>          nvdev->base.fd = fd;
>>>
>>>          ver = drmGetVersion(fd);
>>> -       if (ver) dev->drm_version = (ver->version_major << 24) |
>>> -                                   (ver->version_minor << 8) |
>>> -                                    ver->version_patchlevel;
>>> +       if (!ver) {
>>> +               ret = -errno;
>>> +               goto error;
>>> +       }
>>> +
>>> +       dev->drm_version = (ver->version_major << 24) |
>>> +                           (ver->version_minor << 8) |
>>> +                            ver->version_patchlevel;
>>>          drmFreeVersion(ver);
>>>
>>>          if ( dev->drm_version != 0x00000010 &&
>>>              (dev->drm_version <  0x01000000 ||
>>>               dev->drm_version >= 0x02000000)) {
>>> -               nouveau_device_del(&dev);
>>> -               return -EINVAL;
>>> +               ret = -EINVAL;
>>> +               goto error;
>>>          }
>>>
>>>          ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_CHIPSET_ID,
>>> &chipset);
>>> -       if (ret == 0)
>>> +       if (ret)
>>> +               goto error;
>>>          ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_FB_SIZE, &vram);
>>> -       if (ret == 0)
>>> +       if (ret)
>>> +               goto error;
>>>          ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_AGP_SIZE, &gart);
>>> -       if (ret) {
>>> -               nouveau_device_del(&dev);
>>> -               return ret;
>>> -       }
>>> +       if (ret)
>>> +               goto error;
>>>
>>>          ret = nouveau_getparam(dev, NOUVEAU_GETPARAM_HAS_BO_USAGE,
>>> &bousage);
>>>          if (ret == 0)
>>> @@ -139,6 +144,9 @@ nouveau_device_wrap(int fd, int close, struct
>>> nouveau_device **pdev)
>>>
>>>          *pdev = &nvdev->base;
>>>          return 0;
>>> +error:
>>> +       nouveau_device_del(&dev);
>>> +       return -ret;
>>
>>
>> you mean 'ret' of course.
>>
> Yikes, thanks.
>
> -Emil
>
>
>>>   }
>>>
>>>   int
>>> --
>>> 1.9.0
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>

Re: [PATCH 1/3] nouveau: cleanup error handling during nouveau_device_wrap

[Emil Velikov]

On 13/03/14 01:05, Ilia Mirkin wrote:
[snip]
>
> Not really. drm->drm_version will be 0 if ver fails.
>
Indeed, dev is calloc'ated by its callers, and if they mess around with 
that's their own fault.

Sorry for the noise.
-Emil

Re: [PATCH 1/3] nouveau: cleanup error handling during nouveau_device_wrap

[Ilia Mirkin]

On Wed, Mar 12, 2014 at 9:24 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 13/03/14 01:05, Ilia Mirkin wrote:
> [snip]
>
>>
>> Not really. drm->drm_version will be 0 if ver fails.
>>
> Indeed, dev is calloc'ated by its callers, and if they mess around with
> that's their own fault.

The callers? It's calloc'd inside nouveau_device_wrap itself. Unless
calloc is changed to not init to 0 (which would be a giant spec
violation), I don't see how something could go wrong.