[PATCH libdrm] xf86drm: fix valgrind warning in drmParsePciBusInfo

[PATCH libdrm] xf86drm: fix valgrind warning in drmParsePciBusInfo

[Nicholas Miell]

From 714d07f600db39498c87d7816f4dd3a7e6d9bbca Mon Sep 17 00:00:00 2001
From: Nicholas Miell <nmiell@gmail.com>
Date: Thu, 12 Jan 2017 15:43:07 -0800
Subject: [PATCH libdrm] xf86drm: fix valgrind warning in drmParsePciBusInfo

The current implementation reads (up to) 513 bytes, overwrites the 513th
byte with '\0' and then passes the buffer off to strstr() and sscanf()
without ever initializing the middle bytes. This causes valgrind
warnings and potentially fails to parse PCI_SLOT_NAME if the uevent is
unexpectedly large.

Rewrite it using getline() to fix the valgrind errors and future-proof
the function against uevent files larger than 513 bytes.

Signed-off-by: Nicholas Miell <nmiell@gmail.com>
---
 xf86drm.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/xf86drm.c b/xf86drm.c
index b8b2cfe..33261ac 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -2919,31 +2919,31 @@ static int drmParsePciBusInfo(int maj, int min, drmPciBusInfoPtr info)
 {
 #ifdef __linux__
     char path[PATH_MAX + 1];
-    char data[512 + 1];
-    char *str;
+    FILE *uevent;
+    char *line = NULL;
+    size_t n = 0;
+    bool found = false;
     int domain, bus, dev, func;
-    int fd, ret;
 
     snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device/uevent", maj, min);
-    fd = open(path, O_RDONLY);
-    if (fd < 0)
+    uevent = fopen(path, "r");
+    if (uevent == NULL)
         return -errno;
 
-    ret = read(fd, data, sizeof(data));
-    data[sizeof(data)-1] = '\0';
-    close(fd);
-    if (ret < 0)
-        return -errno;
+    while (getline(&line, &n, uevent) != -1) {
+        if (sscanf(line, "PCI_SLOT_NAME=%04x:%02x:%02x.%1u",
+                   &domain, &bus, &dev, &func) == 4)
+        {
+	        found = true;
+	        break;
+        }
+    }
+    free(line);
 
-#define TAG "PCI_SLOT_NAME="
-    str = strstr(data, TAG);
-    if (str == NULL)
-        return -EINVAL;
+    fclose(uevent);
 
-    if (sscanf(str, TAG "%04x:%02x:%02x.%1u",
-               &domain, &bus, &dev, &func) != 4)
+    if (!found)
         return -EINVAL;
-#undef TAG
 
     info->domain = domain;
     info->bus = bus;
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm] xf86drm: fix valgrind warning in drmParsePciBusInfo

[Jan Vesely]

[-- Attachment #1.1: Type: text/plain, Size: 2862 bytes --]

On Thu, 2017-01-12 at 18:16 -0800, Nicholas Miell wrote:
> From 714d07f600db39498c87d7816f4dd3a7e6d9bbca Mon Sep 17 00:00:00 2001
> From: Nicholas Miell <nmiell@gmail.com>
> Date: Thu, 12 Jan 2017 15:43:07 -0800
> Subject: [PATCH libdrm] xf86drm: fix valgrind warning in drmParsePciBusInfo
> 
> The current implementation reads (up to) 513 bytes, overwrites the 513th
> byte with '\0' and then passes the buffer off to strstr() and sscanf()
> without ever initializing the middle bytes. This causes valgrind
> warnings and potentially fails to parse PCI_SLOT_NAME if the uevent is
> unexpectedly large.

a simpler fix should also get rid of the valgrind warning:

-      ret = read(fd, data, sizeof(data));
-      data[sizeof(data)-1] = '\0';
+      ret = read(fd, data, sizeof(data) - 1);
       close(fd);
       if (ret < 0)
           return -errno
+      data[ret] = '\0';


I think that dynamic memory allocation is still a more robust approach.

regards,
Jan

> 
> Rewrite it using getline() to fix the valgrind errors and future-proof
> the function against uevent files larger than 513 bytes.
> 
> Signed-off-by: Nicholas Miell <nmiell@gmail.com>
> ---
>  xf86drm.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/xf86drm.c b/xf86drm.c
> index b8b2cfe..33261ac 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -2919,31 +2919,31 @@ static int drmParsePciBusInfo(int maj, int min, drmPciBusInfoPtr info)
>  {
>  #ifdef __linux__
>      char path[PATH_MAX + 1];
> -    char data[512 + 1];
> -    char *str;
> +    FILE *uevent;
> +    char *line = NULL;
> +    size_t n = 0;
> +    bool found = false;
>      int domain, bus, dev, func;
> -    int fd, ret;
>  
>      snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device/uevent", maj, min);
> -    fd = open(path, O_RDONLY);
> -    if (fd < 0)
> +    uevent = fopen(path, "r");
> +    if (uevent == NULL)
>          return -errno;
>  
> -    ret = read(fd, data, sizeof(data));
> -    data[sizeof(data)-1] = '\0';
> -    close(fd);
> -    if (ret < 0)
> -        return -errno;
> +    while (getline(&line, &n, uevent) != -1) {
> +        if (sscanf(line, "PCI_SLOT_NAME=%04x:%02x:%02x.%1u",
> +                   &domain, &bus, &dev, &func) == 4)
> +        {
> +	        found = true;
> +	        break;
> +        }
> +    }
> +    free(line);
>  
> -#define TAG "PCI_SLOT_NAME="
> -    str = strstr(data, TAG);
> -    if (str == NULL)
> -        return -EINVAL;
> +    fclose(uevent);
>  
> -    if (sscanf(str, TAG "%04x:%02x:%02x.%1u",
> -               &domain, &bus, &dev, &func) != 4)
> +    if (!found)
>          return -EINVAL;
> -#undef TAG
>  
>      info->domain = domain;
>      info->bus = bus;

-- 
Jan Vesely <jan.vesely@rutgers.edu>

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm] xf86drm: fix valgrind warning in drmParsePciBusInfo

[Emil Velikov]

On 13 January 2017 at 11:34, Jan Vesely <jan.vesely@rutgers.edu> wrote:
> On Thu, 2017-01-12 at 18:16 -0800, Nicholas Miell wrote:
>> From 714d07f600db39498c87d7816f4dd3a7e6d9bbca Mon Sep 17 00:00:00 2001
>> From: Nicholas Miell <nmiell@gmail.com>
>> Date: Thu, 12 Jan 2017 15:43:07 -0800
>> Subject: [PATCH libdrm] xf86drm: fix valgrind warning in drmParsePciBusInfo
>>
>> The current implementation reads (up to) 513 bytes, overwrites the 513th
>> byte with '\0' and then passes the buffer off to strstr() and sscanf()
>> without ever initializing the middle bytes. This causes valgrind
>> warnings and potentially fails to parse PCI_SLOT_NAME if the uevent is
>> unexpectedly large.
>
> a simpler fix should also get rid of the valgrind warning:
>
> -      ret = read(fd, data, sizeof(data));
> -      data[sizeof(data)-1] = '\0';
> +      ret = read(fd, data, sizeof(data) - 1);
>        close(fd);
>        if (ret < 0)
>            return -errno
> +      data[ret] = '\0';
>
We had this (better imho) patch a week or so ago. In either case the
issue is virtually since (iirc) if the string is malformed we'll bail
out either way.

>
> I think that dynamic memory allocation is still a more robust approach.
>
Yes that might be the better solution, or one could even use
getline(). The latter might be pushing it's only POSIX 2008.

Thanks
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm] xf86drm: fix valgrind warning in drmParsePciBusInfo

[Nicholas Miell]

On 01/13/2017 09:57 AM, Emil Velikov wrote:
> On 13 January 2017 at 11:34, Jan Vesely <jan.vesely@rutgers.edu> wrote:
>> On Thu, 2017-01-12 at 18:16 -0800, Nicholas Miell wrote:
>>> From 714d07f600db39498c87d7816f4dd3a7e6d9bbca Mon Sep 17 00:00:00 2001
>>> From: Nicholas Miell <nmiell@gmail.com>
>>> Date: Thu, 12 Jan 2017 15:43:07 -0800
>>> Subject: [PATCH libdrm] xf86drm: fix valgrind warning in drmParsePciBusInfo
>>>
>>> The current implementation reads (up to) 513 bytes, overwrites the 513th
>>> byte with '\0' and then passes the buffer off to strstr() and sscanf()
>>> without ever initializing the middle bytes. This causes valgrind
>>> warnings and potentially fails to parse PCI_SLOT_NAME if the uevent is
>>> unexpectedly large.
>>
>> a simpler fix should also get rid of the valgrind warning:
>>
>> -      ret = read(fd, data, sizeof(data));
>> -      data[sizeof(data)-1] = '\0';
>> +      ret = read(fd, data, sizeof(data) - 1);
>>        close(fd);
>>        if (ret < 0)
>>            return -errno
>> +      data[ret] = '\0';
>>
> We had this (better imho) patch a week or so ago. In either case the
> issue is virtually since (iirc) if the string is malformed we'll bail
> out either way.

Simpler, but potentially stops working in the future. It already stopped 
working once.

>> I think that dynamic memory allocation is still a more robust approach.
>>
> Yes that might be the better solution, or one could even use
> getline(). The latter might be pushing it's only POSIX 2008.

POSIX isn't relevant, this is a Linux-specific function.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm] xf86drm: fix valgrind warning in drmParsePciBusInfo

[Emil Velikov]

On 13 January 2017 at 21:11, Nicholas Miell <nmiell@gmail.com> wrote:
> On 01/13/2017 09:57 AM, Emil Velikov wrote:
>>
>> On 13 January 2017 at 11:34, Jan Vesely <jan.vesely@rutgers.edu> wrote:
>>>
>>> On Thu, 2017-01-12 at 18:16 -0800, Nicholas Miell wrote:
>>>>
>>>> From 714d07f600db39498c87d7816f4dd3a7e6d9bbca Mon Sep 17 00:00:00 2001
>>>> From: Nicholas Miell <nmiell@gmail.com>
>>>> Date: Thu, 12 Jan 2017 15:43:07 -0800
>>>> Subject: [PATCH libdrm] xf86drm: fix valgrind warning in
>>>> drmParsePciBusInfo
>>>>
>>>> The current implementation reads (up to) 513 bytes, overwrites the 513th
>>>> byte with '\0' and then passes the buffer off to strstr() and sscanf()
>>>> without ever initializing the middle bytes. This causes valgrind
>>>> warnings and potentially fails to parse PCI_SLOT_NAME if the uevent is
>>>> unexpectedly large.
>>>
>>>
>>> a simpler fix should also get rid of the valgrind warning:
>>>
>>> -      ret = read(fd, data, sizeof(data));
>>> -      data[sizeof(data)-1] = '\0';
>>> +      ret = read(fd, data, sizeof(data) - 1);
>>>        close(fd);
>>>        if (ret < 0)
>>>            return -errno
>>> +      data[ret] = '\0';
>>>
>> We had this (better imho) patch a week or so ago. In either case the
>> issue is virtually since (iirc) if the string is malformed we'll bail
>> out either way.
>
>
> Simpler, but potentially stops working in the future. It already stopped
> working once.
>
Stopped working = it never worked since it was introduced (initially
in mesa) circa 2014 ;-)

That aside, Thierry has some helper(s) which we can reuse here.

>>> I think that dynamic memory allocation is still a more robust approach.
>>>
>> Yes that might be the better solution, or one could even use
>> getline(). The latter might be pushing it's only POSIX 2008.
>
>
> POSIX isn't relevant, this is a Linux-specific function.
>
I'm well aware of that, it was me who added the guard or reviewed &
pushed the commit ;-)

As you may be aware other platforms also have sysfs - FreeBSD (and
derivatives?), GNU Hurd and perhaps others. Things are kept Linux only
since almost (nobody) running !Linux platform has bothered looking in
libdrm for a long time. And it's not like I haven't poked people on a
number of occasions :-P

Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm] xf86drm: fix valgrind warning in drmParsePciBusInfo

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3713 bytes --]

On Mon, Jan 16, 2017 at 02:53:52PM +0000, Emil Velikov wrote:
> On 13 January 2017 at 21:11, Nicholas Miell <nmiell@gmail.com> wrote:
> > On 01/13/2017 09:57 AM, Emil Velikov wrote:
> >>
> >> On 13 January 2017 at 11:34, Jan Vesely <jan.vesely@rutgers.edu> wrote:
> >>>
> >>> On Thu, 2017-01-12 at 18:16 -0800, Nicholas Miell wrote:
> >>>>
> >>>> From 714d07f600db39498c87d7816f4dd3a7e6d9bbca Mon Sep 17 00:00:00 2001
> >>>> From: Nicholas Miell <nmiell@gmail.com>
> >>>> Date: Thu, 12 Jan 2017 15:43:07 -0800
> >>>> Subject: [PATCH libdrm] xf86drm: fix valgrind warning in
> >>>> drmParsePciBusInfo
> >>>>
> >>>> The current implementation reads (up to) 513 bytes, overwrites the 513th
> >>>> byte with '\0' and then passes the buffer off to strstr() and sscanf()
> >>>> without ever initializing the middle bytes. This causes valgrind
> >>>> warnings and potentially fails to parse PCI_SLOT_NAME if the uevent is
> >>>> unexpectedly large.
> >>>
> >>>
> >>> a simpler fix should also get rid of the valgrind warning:
> >>>
> >>> -      ret = read(fd, data, sizeof(data));
> >>> -      data[sizeof(data)-1] = '\0';
> >>> +      ret = read(fd, data, sizeof(data) - 1);
> >>>        close(fd);
> >>>        if (ret < 0)
> >>>            return -errno
> >>> +      data[ret] = '\0';
> >>>
> >> We had this (better imho) patch a week or so ago. In either case the
> >> issue is virtually since (iirc) if the string is malformed we'll bail
> >> out either way.
> >
> >
> > Simpler, but potentially stops working in the future. It already stopped
> > working once.
> >
> Stopped working = it never worked since it was introduced (initially
> in mesa) circa 2014 ;-)
> 
> That aside, Thierry has some helper(s) which we can reuse here.
> 
> >>> I think that dynamic memory allocation is still a more robust approach.
> >>>
> >> Yes that might be the better solution, or one could even use
> >> getline(). The latter might be pushing it's only POSIX 2008.
> >
> >
> > POSIX isn't relevant, this is a Linux-specific function.
> >
> I'm well aware of that, it was me who added the guard or reviewed &
> pushed the commit ;-)
> 
> As you may be aware other platforms also have sysfs - FreeBSD (and
> derivatives?), GNU Hurd and perhaps others. Things are kept Linux only
> since almost (nobody) running !Linux platform has bothered looking in
> libdrm for a long time. And it's not like I haven't poked people on a
> number of occasions :-P

I've been doing a bit of research to justify the use of POSIX 2008
because the helpers that I posted also rely on getline(), which is
really convenient for this kind of thing.

FreeBSD has supported this since 8.0, which, according to Wikipedia,
was released in late 2009 (the FreeBSD servers don't seem to offer a
download for 8.0 anymore). glibc has had this since at least 2.10,
released in late 2009 as well. Earlier it had been available as a
GNU extension. GNU Hurd uses a fork of glibc 2.13.

Some other C runtime implementations I know about: uClibc has supported
getline since 2000 (without further research I suspect that this means
glibc has had it for at least as long). musl has had it's since the
beginning of time (0.5.0, released in November 2011).

Is there anything else that libdrm needs to support? All of the above
have supported getline() for the better part of a decade, I think that
would be safe enough.

How about we just start using it, and in case anyone ever encounters a
system that doesn't have getline() and have a really good reason to use
a recent version of libdrm without upgrading the rest of their system,
we can provide a fallback implementation.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel