[libdrm,3/4] xf86drm: fix incorrect fd comparison in drmOpenOnce{, WithType}
diff mbox

Message ID 1436883005-6163-3-git-send-email-emil.l.velikov@gmail.com
State New
Headers show

Commit Message

Emil Velikov July 14, 2015, 2:10 p.m. UTC
Spotted by looking for similar "let's assume fd == 0 is invalid" bugs.

Cc: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
---
 xf86drm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Thierry Reding July 15, 2015, 11:47 a.m. UTC | #1
On Tue, Jul 14, 2015 at 03:10:04PM +0100, Emil Velikov wrote:
> Spotted by looking for similar "let's assume fd == 0 is invalid" bugs.
> 
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
> ---
>  xf86drm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xf86drm.c b/xf86drm.c
> index 2c17d11..39c6e2d 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -2619,7 +2619,7 @@ int drmOpenOnceWithType(const char *BusID, int *newlyopened, int type)
>  	}
>  
>      fd = drmOpenWithType(NULL, BusID, type);
> -    if (fd <= 0 || nr_fds == DRM_MAX_FDS)
> +    if (fd < 0 || nr_fds == DRM_MAX_FDS)

Consider what happens if we have DRM_MAX_FDS file descriptors open and
the call to drmOpenWithType() succeeds. We'll end up returning the file
descriptor as is, but we won't keep track.

I suppose this could have been on purpose, so that the device could be
opened even if the file descriptor couldn't be cached anymore. One
potential problem with that could be that the open-once restriction
would be silently ignored. That may not be desirable.

Thierry
Emil Velikov July 15, 2015, 12:37 p.m. UTC | #2
On 15 July 2015 at 12:47, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Tue, Jul 14, 2015 at 03:10:04PM +0100, Emil Velikov wrote:
>> Spotted by looking for similar "let's assume fd == 0 is invalid" bugs.
>>
>> Cc: Thierry Reding <thierry.reding@gmail.com>
>> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
>> ---
>>  xf86drm.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xf86drm.c b/xf86drm.c
>> index 2c17d11..39c6e2d 100644
>> --- a/xf86drm.c
>> +++ b/xf86drm.c
>> @@ -2619,7 +2619,7 @@ int drmOpenOnceWithType(const char *BusID, int *newlyopened, int type)
>>       }
>>
>>      fd = drmOpenWithType(NULL, BusID, type);
>> -    if (fd <= 0 || nr_fds == DRM_MAX_FDS)
>> +    if (fd < 0 || nr_fds == DRM_MAX_FDS)
>
> Consider what happens if we have DRM_MAX_FDS file descriptors open and
> the call to drmOpenWithType() succeeds. We'll end up returning the file
> descriptor as is, but we won't keep track.
>
> I suppose this could have been on purpose, so that the device could be
> opened even if the file descriptor couldn't be cached anymore. One
> potential problem with that could be that the open-once restriction
> would be silently ignored. That may not be desirable.
>
Thanks for reviewing !

Yes I have considered the issue. It's slightly different bug (fixed by
using dynamic allocation?) than what this patch aims at. This fix came
along for consistency sake rather than me caring about this legacy
API.

Can you please follow up if you're interested ?

Emil
Thierry Reding July 15, 2015, 12:47 p.m. UTC | #3
On Wed, Jul 15, 2015 at 01:37:22PM +0100, Emil Velikov wrote:
> On 15 July 2015 at 12:47, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Tue, Jul 14, 2015 at 03:10:04PM +0100, Emil Velikov wrote:
> >> Spotted by looking for similar "let's assume fd == 0 is invalid" bugs.
> >>
> >> Cc: Thierry Reding <thierry.reding@gmail.com>
> >> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
> >> ---
> >>  xf86drm.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/xf86drm.c b/xf86drm.c
> >> index 2c17d11..39c6e2d 100644
> >> --- a/xf86drm.c
> >> +++ b/xf86drm.c
> >> @@ -2619,7 +2619,7 @@ int drmOpenOnceWithType(const char *BusID, int *newlyopened, int type)
> >>       }
> >>
> >>      fd = drmOpenWithType(NULL, BusID, type);
> >> -    if (fd <= 0 || nr_fds == DRM_MAX_FDS)
> >> +    if (fd < 0 || nr_fds == DRM_MAX_FDS)
> >
> > Consider what happens if we have DRM_MAX_FDS file descriptors open and
> > the call to drmOpenWithType() succeeds. We'll end up returning the file
> > descriptor as is, but we won't keep track.
> >
> > I suppose this could have been on purpose, so that the device could be
> > opened even if the file descriptor couldn't be cached anymore. One
> > potential problem with that could be that the open-once restriction
> > would be silently ignored. That may not be desirable.
> >
> Thanks for reviewing !
> 
> Yes I have considered the issue. It's slightly different bug (fixed by
> using dynamic allocation?) than what this patch aims at. This fix came
> along for consistency sake rather than me caring about this legacy
> API.

Right, I had meant to say:

Reviewed-by: Thierry Reding <treding@nvidia.com>

irrespective of the above, given that they are two orthogonal issues.

> Can you please follow up if you're interested ?

I don't think I have any interest in this API, just occurred to me that
it wasn't well documented and the behaviour seemed at odds with what
documentation there is (i.e. the function name).

Thierry
Emil Velikov July 16, 2015, 3:53 p.m. UTC | #4
On 15 July 2015 at 13:47, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Wed, Jul 15, 2015 at 01:37:22PM +0100, Emil Velikov wrote:
>> On 15 July 2015 at 12:47, Thierry Reding <thierry.reding@gmail.com> wrote:
>> > On Tue, Jul 14, 2015 at 03:10:04PM +0100, Emil Velikov wrote:
>> >> Spotted by looking for similar "let's assume fd == 0 is invalid" bugs.
>> >>
>> >> Cc: Thierry Reding <thierry.reding@gmail.com>
>> >> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
>> >> ---
>> >>  xf86drm.c | 2 +-
>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/xf86drm.c b/xf86drm.c
>> >> index 2c17d11..39c6e2d 100644
>> >> --- a/xf86drm.c
>> >> +++ b/xf86drm.c
>> >> @@ -2619,7 +2619,7 @@ int drmOpenOnceWithType(const char *BusID, int *newlyopened, int type)
>> >>       }
>> >>
>> >>      fd = drmOpenWithType(NULL, BusID, type);
>> >> -    if (fd <= 0 || nr_fds == DRM_MAX_FDS)
>> >> +    if (fd < 0 || nr_fds == DRM_MAX_FDS)
>> >
>> > Consider what happens if we have DRM_MAX_FDS file descriptors open and
>> > the call to drmOpenWithType() succeeds. We'll end up returning the file
>> > descriptor as is, but we won't keep track.
>> >
>> > I suppose this could have been on purpose, so that the device could be
>> > opened even if the file descriptor couldn't be cached anymore. One
>> > potential problem with that could be that the open-once restriction
>> > would be silently ignored. That may not be desirable.
>> >
>> Thanks for reviewing !
>>
>> Yes I have considered the issue. It's slightly different bug (fixed by
>> using dynamic allocation?) than what this patch aims at. This fix came
>> along for consistency sake rather than me caring about this legacy
>> API.
>
> Right, I had meant to say:
>
> Reviewed-by: Thierry Reding <treding@nvidia.com>
>
> irrespective of the above, given that they are two orthogonal issues.
>
Ack. Greatly appreciated.

-Emil

Patch
diff mbox

diff --git a/xf86drm.c b/xf86drm.c
index 2c17d11..39c6e2d 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -2619,7 +2619,7 @@  int drmOpenOnceWithType(const char *BusID, int *newlyopened, int type)
 	}
 
     fd = drmOpenWithType(NULL, BusID, type);
-    if (fd <= 0 || nr_fds == DRM_MAX_FDS)
+    if (fd < 0 || nr_fds == DRM_MAX_FDS)
 	return fd;
    
     connection[nr_fds].BusID = strdup(BusID);