[PATCH v3 1/1] libcamera: process: Pass stderr and reserve stdin and stdout fds

[Julien Vuillaumier]

Hi Laurent,

On 22/05/2025 01:04, Laurent Pinchart wrote:
> Hi Julien,
> 
> Thank you for the patch.
> 
> On Mon, May 19, 2025 at 02:23:52PM +0200, Julien Vuillaumier wrote:
>> When a child process is started from Process::start(), the file
>> descriptors inherited from the parent process are closed, except
>> the ones explicitly listed in the fds[] argument.
>>
>> One issue is that the file descriptors for stdin, stdout and stderr
>> being closed, the subsequent file descriptors created by the child
>> process will reuse the values 0, 1 and 2 that are now available.
>> Thus, usage of printf(), assert() or alike may direct its output
>> to the new resource bound to one of these reused file descriptors.
>> The other issue is that the child process can no longer log on
>> the console because stderr has been closed.
>>
>> To address the 2 issues, Process:start() is amended as below:
>> - Child process inherits from parent's stderr fd in order to share
>> the same logging descriptor
>> - Child process stdin, stdout and stderr fds are bound to /dev/null
>> if not inherited from parent. That is to prevent those descriptors
>> to be reused for any other resource, that could be corrupted by
>> the presence of printf(), assert() or alike.
>>
>> Signed-off-by: Julien Vuillaumier <julien.vuillaumier at nxp.com>
>> ---
>>   src/libcamera/process.cpp | 16 +++++++++++++++-
>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp
>> index 68fad327..fa92f58c 100644
>> --- a/src/libcamera/process.cpp
>> +++ b/src/libcamera/process.cpp
>> @@ -259,7 +259,21 @@ int Process::start(const std::string &path,
>>                if (isolate())
>>                        _exit(EXIT_FAILURE);
>>
>> -             closeAllFdsExcept(fds);
>> +             std::vector<int> v(fds);
>> +             v.push_back(STDERR_FILENO);
> 
> There's still a risk that STDERR_FILENO refers to a different file than
> stderr. Could you test this patch (with an IPA module running in
> isolated mode) with pipewire ? If that doesn't cause any issue I'll feel
> better accepting the risk.

I tested libcamera (v0.5.0 tag) running with vimc isolated IPA, operated 
from PipeWire / WirePlumber - both components are run as systemd 
services. By default, systemd explicitly binds the standard descriptors 
of the services per mapping below:
- STDIN_FILENO: /dev/null
- STDOUT_FILENO: journal socket
- STDERR_FILENO: same as STDOUT
But this mapping could be changed in the respective service 
configuration files, where standard descriptors may also be associated 
to a dedicated file, kmsg, a socket etc - see `Logging and Standard 
Input/Output` in [1].

Thus, when libcamera is invoked from either PipeWire or WirePlumber 
service, the STDIN/STDOUT/STERR descriptors are already allocated and 
bound to some descriptors relevant for the standard input/output which 
is presumably safe. I suppose that any other configuration (standard 
descriptors not allocated to standard IO) is unsafe.

I have verified vimc IPA in isolated mode: this change enables the 
isolated IPA to output its log in the PipeWire unit journal 
(`journalctrl --user -u pipewire -f`). The journal daemon socket 
descriptor mapped onto PipeWire STDERR_FILENO is inherited by the 
isolated IPA as expected.

Side note: using isolated IPA with PW requires commenting out the line 
`RestrictNamespaces=yes` in the pipewire.service file [2].
No such issue exists in WirePlumber.

[1]https://www.freedesktop.org/software/systemd/man/latest/systemd.exec.html
[2] 
https://github.com/PipeWire/pipewire/blob/master/src/daemon/systemd/user/pipewire.service.in#L23

>> +             closeAllFdsExcept(v);
>> +
>> +             const auto tryDevNullLowestFd = [](int expected, int oflag) {
>> +                     int fd = open("/dev/null", oflag);
>> +                     if (fd < 0)
>> +                             exit(EXIT_FAILURE);
>> +                     if (fd != expected)
>> +                             close(fd);
>> +             };
>> +
>> +             tryDevNullLowestFd(STDIN_FILENO, O_RDONLY);
>> +             tryDevNullLowestFd(STDOUT_FILENO, O_WRONLY);
>> +             tryDevNullLowestFd(STDERR_FILENO, O_WRONLY);
>>
>>                const char *file = utils::secure_getenv("LIBCAMERA_LOG_FILE");
>>                if (file && strcmp(file, "syslog"))

Thanks,
Julien