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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.
>>      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