[PATCH libdrm] xf86drm: fix valgrind warning in drmParsePciBusInfo

[Jan Vesely]

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 at 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 at 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 at rutgers.edu>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20170113/9fa5dc6b/attachment.sig>