| Message ID | 20220524163152.19948-1-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libxl: Don't leak self pipes | expand |
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@gmail.com> > --- > 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@citrix.com> Thanks,
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); + return 0; out: 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); } if (!libxl__ev_fd_isregistered(&CTX->sigchld_selfpipe_efd)) { rc = libxl__ev_fd_register(gc, &CTX->sigchld_selfpipe_efd,
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@gmail.com> --- Maybe the setting wants to move into libxl__pipe_nonblock()? Poller & sigchld are the only callers of that function. --- tools/libs/light/libxl_event.c | 3 +++ tools/libs/light/libxl_fork.c | 2 ++ 2 files changed, 5 insertions(+)