* [PATCH libdrm] xf86drm: fix valgrind warning in drmParsePciBusInfo
@ 2017-01-13 2:16 Nicholas Miell
2017-01-13 11:34 ` Jan Vesely
0 siblings, 1 reply; 6+ messages in thread
From: Nicholas Miell @ 2017-01-13 2:16 UTC (permalink / raw)
To: dri-devel
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH libdrm] xf86drm: fix valgrind warning in drmParsePciBusInfo
2017-01-13 2:16 [PATCH libdrm] xf86drm: fix valgrind warning in drmParsePciBusInfo Nicholas Miell
@ 2017-01-13 11:34 ` Jan Vesely
2017-01-13 17:57 ` Emil Velikov
0 siblings, 1 reply; 6+ messages in thread
From: Jan Vesely @ 2017-01-13 11:34 UTC (permalink / raw)
To: Nicholas Miell, dri-devel
[-- 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH libdrm] xf86drm: fix valgrind warning in drmParsePciBusInfo
2017-01-13 11:34 ` Jan Vesely
@ 2017-01-13 17:57 ` Emil Velikov
2017-01-13 21:11 ` Nicholas Miell
0 siblings, 1 reply; 6+ messages in thread
From: Emil Velikov @ 2017-01-13 17:57 UTC (permalink / raw)
To: Jan Vesely; +Cc: Nicholas Miell, ML dri-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH libdrm] xf86drm: fix valgrind warning in drmParsePciBusInfo
2017-01-13 17:57 ` Emil Velikov
@ 2017-01-13 21:11 ` Nicholas Miell
2017-01-16 14:53 ` Emil Velikov
0 siblings, 1 reply; 6+ messages in thread
From: Nicholas Miell @ 2017-01-13 21:11 UTC (permalink / raw)
To: Emil Velikov, Jan Vesely; +Cc: ML dri-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH libdrm] xf86drm: fix valgrind warning in drmParsePciBusInfo
2017-01-13 21:11 ` Nicholas Miell
@ 2017-01-16 14:53 ` Emil Velikov
2017-01-19 10:06 ` Thierry Reding
0 siblings, 1 reply; 6+ messages in thread
From: Emil Velikov @ 2017-01-16 14:53 UTC (permalink / raw)
To: Nicholas Miell; +Cc: Jan Vesely, ML dri-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH libdrm] xf86drm: fix valgrind warning in drmParsePciBusInfo
2017-01-16 14:53 ` Emil Velikov
@ 2017-01-19 10:06 ` Thierry Reding
0 siblings, 0 replies; 6+ messages in thread
From: Thierry Reding @ 2017-01-19 10:06 UTC (permalink / raw)
To: Emil Velikov; +Cc: Nicholas Miell, Jan Vesely, ML dri-devel
[-- 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-01-19 10:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-13 2:16 [PATCH libdrm] xf86drm: fix valgrind warning in drmParsePciBusInfo Nicholas Miell
2017-01-13 11:34 ` Jan Vesely
2017-01-13 17:57 ` Emil Velikov
2017-01-13 21:11 ` Nicholas Miell
2017-01-16 14:53 ` Emil Velikov
2017-01-19 10:06 ` Thierry Reding
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.