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

[Thierry Reding <thierry.reding@gmail.com>]

[-- 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