[PATCH libdrm 1/2] xf86drm: Fix error handling for drmGetDevice()

[PATCH libdrm 1/2] xf86drm: Fix error handling for drmGetDevice()

[Matt Roper]

Some of the error conditions in drmGetDevice() can lead to us calling
closedir(NULL) or leaking memory.  Fix these conditions the same way we
did for drmGetDevices() in commit:

        commit 8c4a1cbd98bd8d185d489395f33302a17db643a9
        Author: Matt Roper <matthew.d.roper@intel.com>
        Date:   Wed Sep 30 09:30:51 2015 -0700

            xf86drm: Fix error handling for drmGetDevices()

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

diff --git a/xf86drm.c b/xf86drm.c
index a29db42..951edbb 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -3108,7 +3108,7 @@ int drmGetDevice(int fd, drmDevicePtr *device)
     sysdir = opendir(DRM_DIR_NAME);
     if (!sysdir) {
         ret = -errno;
-        goto close_sysdir;
+        goto free_locals;
     }
 
     i = 0;
@@ -3165,16 +3165,16 @@ int drmGetDevice(int fd, drmDevicePtr *device)
     for (i = 1; i < node_count && local_devices[i]; i++)
             drmFreeDevice(&local_devices[i]);
 
-    free(local_devices);
     closedir(sysdir);
+    free(local_devices);
     return 0;
 
 free_devices:
     drmFreeDevices(local_devices, i);
-    free(local_devices);
-
-close_sysdir:
     closedir(sysdir);
+
+free_locals:
+    free(local_devices);
     return ret;
 }
 
-- 
2.1.4

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

[PATCH libdrm 2/2] xf86drm: Handle unrecognized subsystems safely in drmGetDevice[s]()

[Matt Roper]

Both drmGetDevice() and drmGetDevices() currently print a warning when
they encounter an unknown (non-PCI) subsystem type for a device node,
but they still proceed to assume that the drmDevicePtr was initialized
and try to add it to the local device array.  Add a 'continue' to the
error case handling to bypass the rest of the processing for devices we
can't handle.

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

diff --git a/xf86drm.c b/xf86drm.c
index 951edbb..7e28b4f 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -3140,7 +3140,7 @@ int drmGetDevice(int fd, drmDevicePtr *device)
             break;
         default:
             fprintf(stderr, "The subsystem type is not supported yet\n");
-            break;
+            continue;
         }
 
         if (i >= max_count) {
@@ -3244,7 +3244,7 @@ int drmGetDevices(drmDevicePtr devices[], int max_devices)
             break;
         default:
             fprintf(stderr, "The subsystem type is not supported yet\n");
-            break;
+            continue;
         }
 
         if (i >= max_count) {
-- 
2.1.4

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

Re: [PATCH libdrm 2/2] xf86drm: Handle unrecognized subsystems safely in drmGetDevice[s]()

[Alex Deucher]

On Fri, Oct 16, 2015 at 6:11 PM, Matt Roper <matthew.d.roper@intel.com> wrote:
> Both drmGetDevice() and drmGetDevices() currently print a warning when
> they encounter an unknown (non-PCI) subsystem type for a device node,
> but they still proceed to assume that the drmDevicePtr was initialized
> and try to add it to the local device array.  Add a 'continue' to the
> error case handling to bypass the rest of the processing for devices we
> can't handle.
>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  xf86drm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xf86drm.c b/xf86drm.c
> index 951edbb..7e28b4f 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -3140,7 +3140,7 @@ int drmGetDevice(int fd, drmDevicePtr *device)
>              break;
>          default:
>              fprintf(stderr, "The subsystem type is not supported yet\n");
> -            break;
> +            continue;
>          }
>
>          if (i >= max_count) {
> @@ -3244,7 +3244,7 @@ int drmGetDevices(drmDevicePtr devices[], int max_devices)
>              break;
>          default:
>              fprintf(stderr, "The subsystem type is not supported yet\n");
> -            break;
> +            continue;
>          }
>
>          if (i >= max_count) {
> --
> 2.1.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 2/2] xf86drm: Handle unrecognized subsystems safely in drmGetDevice[s]()

[Emil Velikov]

On 16 October 2015 at 23:11, Matt Roper <matthew.d.roper@intel.com> wrote:
> Both drmGetDevice() and drmGetDevices() currently print a warning when
> they encounter an unknown (non-PCI) subsystem type for a device node,
> but they still proceed to assume that the drmDevicePtr was initialized
> and try to add it to the local device array.  Add a 'continue' to the
> error case handling to bypass the rest of the processing for devices we
> can't handle.
>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>
Looks like a left over as I moved the realloc() after the switch statement.

For the series:
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>

Out of curiosity did you notice these while going through the code or
do you actually have (work on) platform drm devices ? Can I volunteer
you to add support for them ;-)

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

Re: [PATCH libdrm 2/2] xf86drm: Handle unrecognized subsystems safely in drmGetDevice[s]()

[Matt Roper]

On Fri, Oct 16, 2015 at 11:27:10PM +0100, Emil Velikov wrote:
> On 16 October 2015 at 23:11, Matt Roper <matthew.d.roper@intel.com> wrote:
> > Both drmGetDevice() and drmGetDevices() currently print a warning when
> > they encounter an unknown (non-PCI) subsystem type for a device node,
> > but they still proceed to assume that the drmDevicePtr was initialized
> > and try to add it to the local device array.  Add a 'continue' to the
> > error case handling to bypass the rest of the processing for devices we
> > can't handle.
> >
> > Cc: Emil Velikov <emil.l.velikov@gmail.com>
> Looks like a left over as I moved the realloc() after the switch statement.
> 
> For the series:
> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
> 
> Out of curiosity did you notice these while going through the code or
> do you actually have (work on) platform drm devices ? Can I volunteer
> you to add support for them ;-)

Naw, these were just caught by some static analysis tools we run
internally.  We're not doing anything with platform devices, but the
whole libdrm source tree gets run through the tool, and I figured it was
easy enough to just go ahead and write the fixes.


Matt


> 
> Thanks
> Emil

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 2/2] xf86drm: Handle unrecognized subsystems safely in drmGetDevice[s]()

[Emil Velikov]

On 16 October 2015 at 23:31, Matt Roper <matthew.d.roper@intel.com> wrote:
> On Fri, Oct 16, 2015 at 11:27:10PM +0100, Emil Velikov wrote:
>> On 16 October 2015 at 23:11, Matt Roper <matthew.d.roper@intel.com> wrote:
>> > Both drmGetDevice() and drmGetDevices() currently print a warning when
>> > they encounter an unknown (non-PCI) subsystem type for a device node,
>> > but they still proceed to assume that the drmDevicePtr was initialized
>> > and try to add it to the local device array.  Add a 'continue' to the
>> > error case handling to bypass the rest of the processing for devices we
>> > can't handle.
>> >
>> > Cc: Emil Velikov <emil.l.velikov@gmail.com>
>> Looks like a left over as I moved the realloc() after the switch statement.
>>
>> For the series:
>> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
>>
>> Out of curiosity did you notice these while going through the code or
>> do you actually have (work on) platform drm devices ? Can I volunteer
>> you to add support for them ;-)
>
> Naw, these were just caught by some static analysis tools we run
> internally.  We're not doing anything with platform devices, but the
> whole libdrm source tree gets run through the tool, and I figured it was
> easy enough to just go ahead and write the fixes.
>
Unfortunate :( Fwiw if there are other issues and you don't have the
time to look into them feel free to forward the summary (if possible
of course).
These should be in master now. Thanks for the patches!

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