On 12/06/15 05:52 PM, Emil Velikov wrote:
> On 12/06/15 19:05, Derek Foreman wrote:
>> On 12/06/15 11:29 AM, Emil Velikov wrote:
>>> Hi Derek,
>>
>> Hi, thanks for looking at this. :)
>>
> Props goes to Matt, for the reminder. It kind of fell of my radar.
>
>>> On 1 May 2015 at 18:34, Derek Foreman <der...@osg.samsung.com> wrote:
>>>> These fds can propagate to child processes if we don't set CLOEXEC,
>>>> so make a best effort to do that.
>>>>
>>>> Signed-off-by: Derek Foreman <der...@osg.samsung.com>
>>>> ---
>>>> Noticed this when fixing up similar problems in weston - weston's
>>>> drm fd gets dup()ed here and loses CLOEXEC and ends up in every child
>>>> process the shell launches.
>>>>
>>>> src/egl/drivers/dri2/platform_drm.c | 14 +++++++++++---
>>>> 1 file changed, 11 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/src/egl/drivers/dri2/platform_drm.c
>>>> b/src/egl/drivers/dri2/platform_drm.c
>>>> index 486b003..c326d6c 100644
>>>> --- a/src/egl/drivers/dri2/platform_drm.c
>>>> +++ b/src/egl/drivers/dri2/platform_drm.c
>>>> @@ -596,7 +596,11 @@ dri2_initialize_drm(_EGLDriver *drv, _EGLDisplay
>>>> *disp)
>>>> struct dri2_egl_display *dri2_dpy;
>>>> struct gbm_device *gbm;
>>>> int fd = -1;
>>>> - int i;
>>>> + int i, flags = O_RDWR;
>>>> +
>>>> +#ifdef O_CLOEXEC
>>>> + flags |= O_CLOEXEC;
>>>> +#endif
>>>>
>>>> loader_set_logger(_eglLog);
>>>>
>>>> @@ -611,9 +615,9 @@ dri2_initialize_drm(_EGLDriver *drv, _EGLDisplay *disp)
>>>> char buf[64];
>>>> int n = snprintf(buf, sizeof(buf), DRM_DEV_NAME, DRM_DIR_NAME, 0);
>>>> if (n != -1 && n < sizeof(buf))
>>>> - fd = open(buf, O_RDWR);
>>>> + fd = open(buf, flags);
>>>> if (fd < 0)
>>>> - fd = open("/dev/dri/card0", O_RDWR);
>>>> + fd = open("/dev/dri/card0", flags);
>>>> dri2_dpy->own_device = 1;
>>>> gbm = gbm_create_device(fd);
>>>> if (gbm == NULL)
>>>> @@ -639,6 +643,10 @@ dri2_initialize_drm(_EGLDriver *drv, _EGLDisplay
>>>> *disp)
>>>> }
>>>> }
>>>>
>>>> + flags = fcntl(dri2_dpy->fd, F_GETFD);
>>>> + if (flags >= 0 && !(flags & FD_CLOEXEC))
>>>> + fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
>>>> +
>>> In other places in mesa we explicitly check for EINVAL if open()
>>> fails, and use that as indication that O_CLOEXEC is not supported
>>> (note that we only use the O_RDWR and O_CLOEXEC flags). Curious which
>>> would be the better approach to use through mesa ?
>>
>> Oops - I think that way is better.
>>
>> There's a simple function in loader.c called drm_open_device() that does
>> the open that way, could we just rename that to loader_open_device() and
>> make it non-static and call it from dri2_initialize_drm()?
>>
> That's exactly what I was thinking actually :-)
>
>> That said...
>>
>> Unfortunately, it appears my previous testing was insufficient and
>> there's still a leaked fd - dup() creates fds without CLOEXEC set.
>>
> Seems that we ignore that in a few places through mesa - how about we
> replace dup() + fcntl(F_SETFD, fcntl(F_GETFD)|FD_CLOEXEC) with
> fcntl(F_DUPFD_CLOEXEC) ?
>
> Its requirements are almost the same as O_CLOEXEC (kernel 2.6.{23,24}
> and glibc 2.7) as opposed to dup2/dup3 which requires glibc 2.9 (not
> sure about the kernel).
I like it - do you want me to do that?
> Cheers,
> Emil
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
