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

[Emil Velikov <emil.l.velikov@gmail.com>]

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