[PATCH libdrm 1/3] xf86drm: replace sprintf()+strdup() with asprintf()

[PATCH libdrm 1/3] xf86drm: replace sprintf()+strdup() with asprintf()

[Eric Engestrom]

Signed-off-by: Eric Engestrom <eric.engestrom@imgtec.com>
---
 xf86drm.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/xf86drm.c b/xf86drm.c
index 3a9d0ed2cc9b196ae7d1..b6e5d8cc1bb50ffe75a2 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -2823,7 +2823,7 @@ static char *drmGetMinorNameForFD(int fd, int type)
     struct stat sbuf;
     const char *name = drmGetMinorName(type);
     int len;
-    char dev_name[64], buf[64];
+    char *dev_name, buf[64];
     int maj, min;
 
     if (!name)
@@ -2848,20 +2848,22 @@ static char *drmGetMinorNameForFD(int fd, int type)
 
     while ((ent = readdir(sysdir))) {
         if (strncmp(ent->d_name, name, len) == 0) {
-            snprintf(dev_name, sizeof(dev_name), DRM_DIR_NAME "/%s",
-                 ent->d_name);
+            if (asprintf(&dev_name, DRM_DIR_NAME "/%s",
+                 ent->d_name) < 0) {
+                dev_name = NULL;
+            }
 
             closedir(sysdir);
-            return strdup(dev_name);
+            return dev_name;
         }
     }
     return NULL;
 #else
     struct stat sbuf;
-    char buf[PATH_MAX + 1];
+    char *buf;
     const char *dev_name;
     unsigned int maj, min;
-    int n, base;
+    int base;
 
     if (fstat(fd, &sbuf))
         return NULL;
@@ -2890,11 +2892,10 @@ static char *drmGetMinorNameForFD(int fd, int type)
     if (base < 0)
         return NULL;
 
-    n = snprintf(buf, sizeof(buf), dev_name, DRM_DIR_NAME, min - base);
-    if (n == -1 || n >= sizeof(buf))
+    if (asprintf(&buf, dev_name, DRM_DIR_NAME, min - base) < 0)
         return NULL;
 
-    return strdup(buf);
+    return buf;
 #endif
 }
 
@@ -4119,10 +4120,10 @@ char *drmGetDeviceNameFromFd2(int fd)
     return strdup(path);
 #else
     struct stat      sbuf;
-    char             node[PATH_MAX + 1];
+    char            *node;
     const char      *dev_name;
     int              node_type;
-    int              maj, min, n, base;
+    int              maj, min, base;
 
     if (fstat(fd, &sbuf))
         return NULL;
@@ -4155,11 +4156,10 @@ char *drmGetDeviceNameFromFd2(int fd)
     if (base < 0)
         return NULL;
 
-    n = snprintf(node, PATH_MAX, dev_name, DRM_DIR_NAME, min - base);
-    if (n == -1 || n >= PATH_MAX)
+    if (asprintf(&node, dev_name, DRM_DIR_NAME, min - base) < 0)
       return NULL;
 
-    return strdup(node);
+    return node;
 #endif
 }
 
-- 
Cheers,
  Eric

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

[PATCH libdrm 2/3] xf86drm: add buffer size safety to sprintf()

[Eric Engestrom]

Signed-off-by: Eric Engestrom <eric.engestrom@imgtec.com>
---
 xf86drm.c     | 6 +++---
 xf86drmMode.c | 6 ++++--
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/xf86drm.c b/xf86drm.c
index b6e5d8cc1bb50ffe75a2..5701952ae83634b47628 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -349,7 +349,7 @@ static int drmOpenDevice(dev_t dev, int minor, int type)
         return -EINVAL;
     };
 
-    sprintf(buf, dev_name, DRM_DIR_NAME, minor);
+    snprintf(buf, sizeof(buf), dev_name, DRM_DIR_NAME, minor);
     drmMsg("drmOpenDevice: node name is %s\n", buf);
 
     if (drm_server_info && drm_server_info->get_perms) {
@@ -473,7 +473,7 @@ static int drmOpenMinor(int minor, int create, int type)
         return -EINVAL;
     };
 
-    sprintf(buf, dev_name, DRM_DIR_NAME, minor);
+    snprintf(buf, sizeof(buf), dev_name, DRM_DIR_NAME, minor);
     if ((fd = open(buf, O_RDWR, 0)) >= 0)
         return fd;
     return -errno;
@@ -681,7 +681,7 @@ static int drmOpenByName(const char *name, int type)
         char *driver, *pt, *devstring;
         int  retcode;
 
-        sprintf(proc_name, "/proc/dri/%d/name", i);
+        snprintf(proc_name, sizeof(proc_name), "/proc/dri/%d/name", i);
         if ((fd = open(proc_name, 0, 0)) >= 0) {
             retcode = read(fd, buf, sizeof(buf)-1);
             close(fd);
diff --git a/xf86drmMode.c b/xf86drmMode.c
index 9a15b5e78dda9a4bb7e4..d482ef79c0dc35d639d0 100644
--- a/xf86drmMode.c
+++ b/xf86drmMode.c
@@ -753,7 +753,8 @@ int drmCheckModesettingSupported(const char *busid)
 	if (ret != 4)
 		return -EINVAL;
 
-	sprintf(pci_dev_dir, "/sys/bus/pci/devices/%04x:%02x:%02x.%d/drm",
+	snprintf(pci_dev_dir, sizeof(pci_dev_dir),
+		"/sys/bus/pci/devices/%04x:%02x:%02x.%d/drm",
 		domain, bus, dev, func);
 
 	sysdir = opendir(pci_dev_dir);
@@ -772,7 +773,8 @@ int drmCheckModesettingSupported(const char *busid)
 			return 0;
 	}
 
-	sprintf(pci_dev_dir, "/sys/bus/pci/devices/%04x:%02x:%02x.%d/",
+	snprintf(pci_dev_dir, sizeof(pci_dev_dir),
+		"/sys/bus/pci/devices/%04x:%02x:%02x.%d/",
 		domain, bus, dev, func);
 
 	sysdir = opendir(pci_dev_dir);
-- 
Cheers,
  Eric

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

[PATCH libdrm 3/3] xf86drm: replace stat() with access() to verify file existence

[Eric Engestrom]

Signed-off-by: Eric Engestrom <eric.engestrom@imgtec.com>
---
 xf86drm.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/xf86drm.c b/xf86drm.c
index 5701952ae83634b47628..47a82407df82d37a59b2 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -3767,10 +3767,8 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
         return -EINVAL;
 
     n = snprintf(node, PATH_MAX, dev_name, DRM_DIR_NAME, min - base);
-    if (n == -1 || n >= PATH_MAX)
+    if (n == -1 || n >= PATH_MAX || access(node, F_OK))
       return -errno;
-    if (stat(node, &sbuf))
-        return -EINVAL;
 
     subsystem_type = drmParseSubsystemType(maj, min);
     if (subsystem_type != DRM_BUS_PCI)
-- 
Cheers,
  Eric

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

Re: [PATCH libdrm 1/3] xf86drm: replace sprintf()+strdup() with asprintf()

[Jani Nikula]

On Mon, 26 Mar 2018, Eric Engestrom <eric.engestrom@imgtec.com> wrote:
> Signed-off-by: Eric Engestrom <eric.engestrom@imgtec.com>
> ---
>  xf86drm.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/xf86drm.c b/xf86drm.c
> index 3a9d0ed2cc9b196ae7d1..b6e5d8cc1bb50ffe75a2 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -2823,7 +2823,7 @@ static char *drmGetMinorNameForFD(int fd, int type)
>      struct stat sbuf;
>      const char *name = drmGetMinorName(type);
>      int len;
> -    char dev_name[64], buf[64];
> +    char *dev_name, buf[64];
>      int maj, min;
>  
>      if (!name)
> @@ -2848,20 +2848,22 @@ static char *drmGetMinorNameForFD(int fd, int type)
>  
>      while ((ent = readdir(sysdir))) {
>          if (strncmp(ent->d_name, name, len) == 0) {
> -            snprintf(dev_name, sizeof(dev_name), DRM_DIR_NAME "/%s",
> -                 ent->d_name);
> +            if (asprintf(&dev_name, DRM_DIR_NAME "/%s",

Just noting in passing that asprintf is a GNU extension, is that okay?

BR,
Jani.


> +                 ent->d_name) < 0) {
> +                dev_name = NULL;
> +            }
>  
>              closedir(sysdir);
> -            return strdup(dev_name);
> +            return dev_name;
>          }
>      }
>      return NULL;
>  #else
>      struct stat sbuf;
> -    char buf[PATH_MAX + 1];
> +    char *buf;
>      const char *dev_name;
>      unsigned int maj, min;
> -    int n, base;
> +    int base;
>  
>      if (fstat(fd, &sbuf))
>          return NULL;
> @@ -2890,11 +2892,10 @@ static char *drmGetMinorNameForFD(int fd, int type)
>      if (base < 0)
>          return NULL;
>  
> -    n = snprintf(buf, sizeof(buf), dev_name, DRM_DIR_NAME, min - base);
> -    if (n == -1 || n >= sizeof(buf))
> +    if (asprintf(&buf, dev_name, DRM_DIR_NAME, min - base) < 0)
>          return NULL;
>  
> -    return strdup(buf);
> +    return buf;
>  #endif
>  }
>  
> @@ -4119,10 +4120,10 @@ char *drmGetDeviceNameFromFd2(int fd)
>      return strdup(path);
>  #else
>      struct stat      sbuf;
> -    char             node[PATH_MAX + 1];
> +    char            *node;
>      const char      *dev_name;
>      int              node_type;
> -    int              maj, min, n, base;
> +    int              maj, min, base;
>  
>      if (fstat(fd, &sbuf))
>          return NULL;
> @@ -4155,11 +4156,10 @@ char *drmGetDeviceNameFromFd2(int fd)
>      if (base < 0)
>          return NULL;
>  
> -    n = snprintf(node, PATH_MAX, dev_name, DRM_DIR_NAME, min - base);
> -    if (n == -1 || n >= PATH_MAX)
> +    if (asprintf(&node, dev_name, DRM_DIR_NAME, min - base) < 0)
>        return NULL;
>  
> -    return strdup(node);
> +    return node;
>  #endif
>  }

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 1/3] xf86drm: replace sprintf()+strdup() with asprintf()

[Emil Velikov]

On 26 March 2018 at 14:57, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Mon, 26 Mar 2018, Eric Engestrom <eric.engestrom@imgtec.com> wrote:
>> Signed-off-by: Eric Engestrom <eric.engestrom@imgtec.com>
>> ---
>>  xf86drm.c | 28 ++++++++++++++--------------
>>  1 file changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/xf86drm.c b/xf86drm.c
>> index 3a9d0ed2cc9b196ae7d1..b6e5d8cc1bb50ffe75a2 100644
>> --- a/xf86drm.c
>> +++ b/xf86drm.c
>> @@ -2823,7 +2823,7 @@ static char *drmGetMinorNameForFD(int fd, int type)
>>      struct stat sbuf;
>>      const char *name = drmGetMinorName(type);
>>      int len;
>> -    char dev_name[64], buf[64];
>> +    char *dev_name, buf[64];
>>      int maj, min;
>>
>>      if (!name)
>> @@ -2848,20 +2848,22 @@ static char *drmGetMinorNameForFD(int fd, int type)
>>
>>      while ((ent = readdir(sysdir))) {
>>          if (strncmp(ent->d_name, name, len) == 0) {
>> -            snprintf(dev_name, sizeof(dev_name), DRM_DIR_NAME "/%s",
>> -                 ent->d_name);
>> +            if (asprintf(&dev_name, DRM_DIR_NAME "/%s",
>
> Just noting in passing that asprintf is a GNU extension, is that okay?
>
Was going to mention the same thing. Also POSIX please add it to the
next version ;-)
It doesn't seem to make the code shorter, so I'd go with let's drop this?

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

Re: [PATCH libdrm 2/3] xf86drm: add buffer size safety to sprintf()

[Emil Velikov]

On 26 March 2018 at 11:26, Eric Engestrom <eric.engestrom@imgtec.com> wrote:
> Signed-off-by: Eric Engestrom <eric.engestrom@imgtec.com>
> ---
>  xf86drm.c     | 6 +++---
>  xf86drmMode.c | 6 ++++--
>  2 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/xf86drm.c b/xf86drm.c
> index b6e5d8cc1bb50ffe75a2..5701952ae83634b47628 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -349,7 +349,7 @@ static int drmOpenDevice(dev_t dev, int minor, int type)
>          return -EINVAL;
>      };
>
> -    sprintf(buf, dev_name, DRM_DIR_NAME, minor);
> +    snprintf(buf, sizeof(buf), dev_name, DRM_DIR_NAME, minor);
The patch feels a big meh, for two reasons:
 - buffer contents are controlled and will never overflow
 - s{n,}printf return value is not checked

If there's a particular tool that should be silenced up sure, let's
land it. Otherwise I might as well list my 'grand master plan', we can
start deleting all this code ;-)

What do you think?
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 3/3] xf86drm: replace stat() with access() to verify file existence

[Emil Velikov]

On 26 March 2018 at 11:26, Eric Engestrom <eric.engestrom@imgtec.com> wrote:
> Signed-off-by: Eric Engestrom <eric.engestrom@imgtec.com>
> ---
>  xf86drm.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/xf86drm.c b/xf86drm.c
> index 5701952ae83634b47628..47a82407df82d37a59b2 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -3767,10 +3767,8 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
>          return -EINVAL;
>
>      n = snprintf(node, PATH_MAX, dev_name, DRM_DIR_NAME, min - base);
> -    if (n == -1 || n >= PATH_MAX)
> +    if (n == -1 || n >= PATH_MAX || access(node, F_OK))
>        return -errno;
> -    if (stat(node, &sbuf))
> -        return -EINVAL;
>
Above all - there's a beefy RATIONALE section in man 3p access and I'm
wondering if 2) isn't applicable in some odd corner case.
If things are safe - please a) document why - perf./other reasons and
b) update the other instances.

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

Re: [PATCH libdrm 1/3] xf86drm: replace sprintf()+strdup() with asprintf()

[Eric Engestrom]

On Monday, 2018-03-26 15:00:01 +0100, Emil Velikov wrote:
> On 26 March 2018 at 14:57, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > On Mon, 26 Mar 2018, Eric Engestrom <eric.engestrom@imgtec.com> wrote:
> >> Signed-off-by: Eric Engestrom <eric.engestrom@imgtec.com>
> >> ---
> >>  xf86drm.c | 28 ++++++++++++++--------------
> >>  1 file changed, 14 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/xf86drm.c b/xf86drm.c
> >> index 3a9d0ed2cc9b196ae7d1..b6e5d8cc1bb50ffe75a2 100644
> >> --- a/xf86drm.c
> >> +++ b/xf86drm.c
> >> @@ -2823,7 +2823,7 @@ static char *drmGetMinorNameForFD(int fd, int type)
> >>      struct stat sbuf;
> >>      const char *name = drmGetMinorName(type);
> >>      int len;
> >> -    char dev_name[64], buf[64];
> >> +    char *dev_name, buf[64];
> >>      int maj, min;
> >>
> >>      if (!name)
> >> @@ -2848,20 +2848,22 @@ static char *drmGetMinorNameForFD(int fd, int type)
> >>
> >>      while ((ent = readdir(sysdir))) {
> >>          if (strncmp(ent->d_name, name, len) == 0) {
> >> -            snprintf(dev_name, sizeof(dev_name), DRM_DIR_NAME "/%s",
> >> -                 ent->d_name);
> >> +            if (asprintf(&dev_name, DRM_DIR_NAME "/%s",
> >
> > Just noting in passing that asprintf is a GNU extension, is that okay?
> >
> Was going to mention the same thing. Also POSIX please add it to the
> next version ;-)
> It doesn't seem to make the code shorter, so I'd go with let's drop this?

Those were just some local changes I had done at some random point when
coming across stuff that I thought could be better. I just sent them now
since my local changes will be lost by me leaving my current job, and
I didn't care enough to push them on a branch somewhere (:

Either these were thought to be good by someone else, or, as it is,
they're not and I'm dropping all three :)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel