On 07/05/2024 3:23 pm, Marek Marczykowski-Górecki wrote: > On Tue, May 07, 2024 at 03:15:48PM +0100, Andrew Cooper wrote: >> 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. > But logfile is a global variable, and it looks to be used in dolog()...
Urgh, fine. Lets go back to your suggestion of setting CLOEXEC after dup()ing. ~Andrew
