Re: [PATCH] tools/xl: Open xldevd.log with O_CLOEXEC

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

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 <demi@xxxxxxxxxxxxxxxxxxxxxx>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >> ---
> >> CC: Anthony PERARD <anthony@xxxxxxxxxxxxxx>
> >> CC: Juergen Gross <jgross@xxxxxxxx>
> >> CC: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
> >> CC: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> >>
> >> 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()...

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature