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

[Julien Vuillaumier]

Hello Barnabás,

On 21/05/2025 22:53, Barnabás Pőcze wrote:
> Hi
> 
> 
> 2025. 05. 19. 14:23 keltezéssel, Julien Vuillaumier írta:
>> 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);
>> +             closeAllFdsExcept(v);
>> +
>> +             const auto tryDevNullLowestFd = [](int expected, int 
>> oflag) {
>> +                     int fd = open("/dev/null", oflag);
>> +                     if (fd < 0)
>> +                             exit(EXIT_FAILURE);
> 
> I have missed this earlier, but I think this should be `_exit()` since 
> calling
> any `at{,_quick_}exit()` handlers is likely problematic.

After some googling, recommendation seems to be using _exit() instead of 
exit() in the child process, for occurrences that are not invoked within 
an exec() - that is to avoid interfering with the parent process [1].

So I'll add that change, thanks for the suggestion.

While at it, the `exit(EXIT_FAILURE)` in case of execv() failure in the 
existing implementation does not seem correct and presumably needs to be 
converted to an _exit() as well.

[1] https://stackoverflow.com/a/5423108

Thanks,
Julien