On 07/05/2024 12:32 pm, Marek Marczykowski-Górecki wrote: > On Tue, May 07, 2024 at 12:08:06PM +0100, Andrew Cooper wrote: >> `xl devd` has been observed leaking /var/log/xldevd.log into children. >> >> Link: https://github.com/QubesOS/qubes-issues/issues/8292 >> Reported-by: Demi Marie Obenour <d...@invisiblethingslab.com> >> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com> >> --- >> CC: Anthony PERARD <anth...@xenproject.org> >> CC: Juergen Gross <jgr...@suse.com> >> CC: Demi Marie Obenour <d...@invisiblethingslab.com> >> CC: Marek Marczykowski-Górecki <marma...@invisiblethingslab.com> >> >> Also entirely speculative based on the QubesOS ticket. >> --- >> tools/xl/xl_utils.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/tools/xl/xl_utils.c b/tools/xl/xl_utils.c >> index 17489d182954..060186db3a59 100644 >> --- a/tools/xl/xl_utils.c >> +++ b/tools/xl/xl_utils.c >> @@ -270,7 +270,7 @@ int do_daemonize(const char *name, const char *pidfile) >> exit(-1); >> } >> >> - CHK_SYSCALL(logfile = open(fullname, O_WRONLY|O_CREAT|O_APPEND, 0644)); >> + CHK_SYSCALL(logfile = open(fullname, O_WRONLY | O_CREAT | O_APPEND | >> O_CLOEXEC, 0644)); > This one might be not enough, as the FD gets dup2()-ed to stdout/stderr > just outside of the context here, and then inherited by various hotplug > script. Just adding O_CLOEXEC here means the hotplug scripts will run > with stdout/stderr closed.
Lovely :( Yes - this won't work. I guess what we want instead is: diff --git a/tools/xl/xl_utils.c b/tools/xl/xl_utils.c index 060186db3a59..a0ce7dd7fa21 100644 --- a/tools/xl/xl_utils.c +++ b/tools/xl/xl_utils.c @@ -282,6 +282,7 @@ int do_daemonize(const char *name, const char *pidfile) dup2(logfile, 2); close(nullfd); + close(logfile); CHK_SYSCALL(daemon(0, 1)); which at least means there's not a random extra fd attached to the logfile. >> free(fullname); >> assert(logfile >= 3); >> >> >> base-commit: ebab808eb1bb8f24c7d0dd41b956e48cb1824b81 >> prerequisite-patch-id: 212e50457e9b6bdfd06a97da545a5aa7155bb919 > Which one is this? I don't see it in staging, nor in any of your > branches on xenbits. Lore finds "tools/libxs: Open /dev/xen/xenbus fds > as O_CLOEXEC" which I guess is correct, but I have no idea how it > correlates it, as this hash doesn't appear anywhere in the message, nor > its headers... It's the libxs patch, but rebased over yesterday's push to staging. auto-base doesn't work quite so well in cases like this. ~Andrew
