[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] tools/xl: Open xldevd.log with O_CLOEXEC
On Tue, May 07, 2024 at 01:32:00PM +0200, 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. The scripts shipped with Xen do redirect > stderr to a log quite early, but a) it doesn't do it for stdout, and b) > custom hotplug scripts are a valid use case. > Without that, I see at least few potential issues: > - some log messages may be lost (minor, but annoying) > - something might simply fail on writing to a closed FD, breaking the > hotplug script > - FD 1 will be used as first free FD for any open() or similar call - if > a tool later tries writing something to stdout, it will gets written > to that FD - worse of all three Wait, the above is wrong, dup does not copy the O_CLOEXEC flag over to the new FD. So, maybe your patch is correct after all. > What should be the behavior of hotplug scripts logging? Should they > always take care of their own logging? If so, the hotplug calling part > should redirect stdout/stderr to /dev/null IMO. But if `xl` should > provide some default logging for them (like, the xldevd.log here?), then > the O_CLOEXEC should be set only after duplicating logfile over stdout/err. > > > 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... > > -- > Best Regards, > Marek Marczykowski-Górecki > Invisible Things Lab -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab Attachment:
signature.asc
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |