Re: [PATCH] libxl: Don't leak self pipes

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Tue, May 24, 2022 at 12:31:52PM -0400, Jason Andryuk wrote:
> libxl is leaking self pipes to child processes.  These can be seen when
> running with env var _LIBXL_DEBUG_EXEC_FDS=1:
> 
> libxl: debug: libxl_aoutils.c:593:libxl__async_exec_start: forking to 
> execute: /etc/xen/scripts/vif-bridge online
> [Detaching after fork from child process 5099]
> libxl: execing /etc/xen/scripts/vif-bridge: fd 4 is open to pipe:[46805] with 
> flags 0
> libxl: execing /etc/xen/scripts/vif-bridge: fd 13 is open to pipe:[46807] 
> with flags 0
> libxl: execing /etc/xen/scripts/vif-bridge: fd 14 is open to pipe:[46807] 
> with flags 0
> libxl: execing /etc/xen/scripts/vif-bridge: fd 19 is open to pipe:[48570] 
> with flags 0
> libxl: execing /etc/xen/scripts/vif-bridge: fd 20 is open to pipe:[48570] 
> with flags 0
> 
> (fd 3 is also open, but the check only starts at 4 for some reason.)
> 
> For xl, this is the poller created by libxl_ctx_alloc, the poller
> created by do_domain_create -> libxl__ao_create, and the self pipe for
> libxl__sigchld_needed.  Set CLOEXEC on the FDs so they are not leaked
> into children.
> 
> Signed-off-by: Jason Andryuk <jandryuk@xxxxxxxxx>
> ---
> Maybe the setting wants to move into libxl__pipe_nonblock()?  Poller &
> sigchld are the only callers of that function.

No because we could want a pipe which survive fork/exec. The function
would need to be renamed. I think it's fine to set cloexec on the spot.

> ---
> diff --git a/tools/libs/light/libxl_event.c b/tools/libs/light/libxl_event.c
> index c8bcd13960..8d24613921 100644
> --- a/tools/libs/light/libxl_event.c
> +++ b/tools/libs/light/libxl_event.c
> @@ -1800,6 +1800,9 @@ int libxl__poller_init(libxl__gc *gc, libxl__poller *p)
>      rc = libxl__pipe_nonblock(CTX, p->wakeup_pipe);
>      if (rc) goto out;
>  
> +    libxl_fd_set_cloexec(CTX, p->wakeup_pipe[0], 1);
> +    libxl_fd_set_cloexec(CTX, p->wakeup_pipe[1], 1);

I think that's ok. I tried to find out if pollers needs to survive a
fork/exec, but that doesn't seems to be the case. Pollers are only used
by libxl's event machinery, and in a single CTX I think.

> diff --git a/tools/libs/light/libxl_fork.c b/tools/libs/light/libxl_fork.c
> index 676a14bb28..b13659d231 100644
> --- a/tools/libs/light/libxl_fork.c
> +++ b/tools/libs/light/libxl_fork.c
> @@ -387,6 +387,8 @@ int libxl__sigchld_needed(libxl__gc *gc) /* 
> non-reentrant, idempotent */
>      if (CTX->sigchld_selfpipe[0] < 0) {
>          rc = libxl__pipe_nonblock(CTX, CTX->sigchld_selfpipe);
>          if (rc) goto out;
> +        libxl_fd_set_cloexec(CTX, CTX->sigchld_selfpipe[0], 1);
> +        libxl_fd_set_cloexec(CTX, CTX->sigchld_selfpipe[1], 1);

These ones should be also ok, as the pipe is only used for the SIGCHLD
handler so not shared outside of libxl.


So the patch looks good, and hopefully we don't break libvirt.

Reviewed-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>

Thanks,

-- 
Anthony PERARD