[libdrm] xf86drm: Fix error handling for drmGetDevices()
diff mbox

Message ID 1443630651-30077-1-git-send-email-matthew.d.roper@intel.com
State New
Headers show

Commit Message

Matt Roper Sept. 30, 2015, 4:30 p.m. UTC
If the opendir() call in drmGetDevices() returns failure, we jump to an
error label that calls closedir() and then returns.  However this means
that we're calling closedir(NULL) which may not be safe on all
implementations.  We are also leaking the local_devices array that was
allocated before the opendir() call.

Fix both of these issues by jumping to an earlier error label (to free
local_devices) and guarding the closedir() call with a NULL test.

Cc: Emil Velikov <emil.l.velikov@gmail.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 xf86drm.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Emil Velikov Oct. 1, 2015, 10:12 a.m. UTC | #1
Hi Matt,

On 30 September 2015 at 17:30, Matt Roper <matthew.d.roper@intel.com> wrote:
> If the opendir() call in drmGetDevices() returns failure, we jump to an
> error label that calls closedir() and then returns.  However this means
> that we're calling closedir(NULL) which may not be safe on all
> implementations.  We are also leaking the local_devices array that was
> allocated before the opendir() call.
>
> Fix both of these issues by jumping to an earlier error label (to free
> local_devices) and guarding the closedir() call with a NULL test.
>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  xf86drm.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/xf86drm.c b/xf86drm.c
> index c1cab1b..763d710 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -3209,7 +3209,7 @@ int drmGetDevices(drmDevicePtr devices[], int max_devices)
>      sysdir = opendir(DRM_DIR_NAME);
>      if (!sysdir) {
>          ret = -errno;
> -        goto close_sysdir;
> +        goto free_locals;
>      }
>
>      i = 0;
> @@ -3280,9 +3280,10 @@ int drmGetDevices(drmDevicePtr devices[], int max_devices)
>
>  free_devices:
>      drmFreeDevices(local_devices, i);
> +free_locals:
>      free(local_devices);
>
> -close_sysdir:
> -    closedir(sysdir);
> +    if (sysdir)
> +           closedir(sysdir);
Any objections if we move the new label & free() here and drop the if
check above? I can do that before pushing if that's ok with you.

Thanks for catching this.
Emil
Matt Roper Oct. 1, 2015, 3:59 p.m. UTC | #2
On Thu, Oct 01, 2015 at 11:12:34AM +0100, Emil Velikov wrote:
> Hi Matt,
> 
> On 30 September 2015 at 17:30, Matt Roper <matthew.d.roper@intel.com> wrote:
> > If the opendir() call in drmGetDevices() returns failure, we jump to an
> > error label that calls closedir() and then returns.  However this means
> > that we're calling closedir(NULL) which may not be safe on all
> > implementations.  We are also leaking the local_devices array that was
> > allocated before the opendir() call.
> >
> > Fix both of these issues by jumping to an earlier error label (to free
> > local_devices) and guarding the closedir() call with a NULL test.
> >
> > Cc: Emil Velikov <emil.l.velikov@gmail.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  xf86drm.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/xf86drm.c b/xf86drm.c
> > index c1cab1b..763d710 100644
> > --- a/xf86drm.c
> > +++ b/xf86drm.c
> > @@ -3209,7 +3209,7 @@ int drmGetDevices(drmDevicePtr devices[], int max_devices)
> >      sysdir = opendir(DRM_DIR_NAME);
> >      if (!sysdir) {
> >          ret = -errno;
> > -        goto close_sysdir;
> > +        goto free_locals;
> >      }
> >
> >      i = 0;
> > @@ -3280,9 +3280,10 @@ int drmGetDevices(drmDevicePtr devices[], int max_devices)
> >
> >  free_devices:
> >      drmFreeDevices(local_devices, i);
> > +free_locals:
> >      free(local_devices);
> >
> > -close_sysdir:
> > -    closedir(sysdir);
> > +    if (sysdir)
> > +           closedir(sysdir);
> Any objections if we move the new label & free() here and drop the if
> check above? I can do that before pushing if that's ok with you.
> 
> Thanks for catching this.
> Emil

Sure, that sounds fine too.  Thanks!


Matt
Emil Velikov Oct. 2, 2015, 12:23 p.m. UTC | #3
On 1 October 2015 at 16:59, Matt Roper <matthew.d.roper@intel.com> wrote:
> On Thu, Oct 01, 2015 at 11:12:34AM +0100, Emil Velikov wrote:
>> Hi Matt,
>>
>> On 30 September 2015 at 17:30, Matt Roper <matthew.d.roper@intel.com> wrote:
>> > If the opendir() call in drmGetDevices() returns failure, we jump to an
>> > error label that calls closedir() and then returns.  However this means
>> > that we're calling closedir(NULL) which may not be safe on all
>> > implementations.  We are also leaking the local_devices array that was
>> > allocated before the opendir() call.
>> >
>> > Fix both of these issues by jumping to an earlier error label (to free
>> > local_devices) and guarding the closedir() call with a NULL test.
>> >
>> > Cc: Emil Velikov <emil.l.velikov@gmail.com>
>> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>> > ---
>> >  xf86drm.c | 7 ++++---
>> >  1 file changed, 4 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/xf86drm.c b/xf86drm.c
>> > index c1cab1b..763d710 100644
>> > --- a/xf86drm.c
>> > +++ b/xf86drm.c
>> > @@ -3209,7 +3209,7 @@ int drmGetDevices(drmDevicePtr devices[], int max_devices)
>> >      sysdir = opendir(DRM_DIR_NAME);
>> >      if (!sysdir) {
>> >          ret = -errno;
>> > -        goto close_sysdir;
>> > +        goto free_locals;
>> >      }
>> >
>> >      i = 0;
>> > @@ -3280,9 +3280,10 @@ int drmGetDevices(drmDevicePtr devices[], int max_devices)
>> >
>> >  free_devices:
>> >      drmFreeDevices(local_devices, i);
>> > +free_locals:
>> >      free(local_devices);
>> >
>> > -close_sysdir:
>> > -    closedir(sysdir);
>> > +    if (sysdir)
>> > +           closedir(sysdir);
>> Any objections if we move the new label & free() here and drop the if
>> check above? I can do that before pushing if that's ok with you.
>>
>> Thanks for catching this.
>> Emil
>
> Sure, that sounds fine too.  Thanks!
>
Ack. Amended and pushed to master.

-Emil

Patch
diff mbox

diff --git a/xf86drm.c b/xf86drm.c
index c1cab1b..763d710 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -3209,7 +3209,7 @@  int drmGetDevices(drmDevicePtr devices[], int max_devices)
     sysdir = opendir(DRM_DIR_NAME);
     if (!sysdir) {
         ret = -errno;
-        goto close_sysdir;
+        goto free_locals;
     }
 
     i = 0;
@@ -3280,9 +3280,10 @@  int drmGetDevices(drmDevicePtr devices[], int max_devices)
 
 free_devices:
     drmFreeDevices(local_devices, i);
+free_locals:
     free(local_devices);
 
-close_sysdir:
-    closedir(sysdir);
+    if (sysdir)
+	    closedir(sysdir);
     return ret;
 }