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

[Julien Vuillaumier]

Hello Barnabás,

On 16/05/2025 17:26, Barnabás Pőcze wrote:
> 
> Hi
> 
> 2025. 05. 16. 16:23 keltezéssel, Julien Vuillaumier írta:
>> Hello Barnabás,
>>
>>>
>>> 2024. 12. 18. 19:27 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 stdin and stdout are redirected to /dev/null so
>>>> that file descriptors 0 and 1 are not reused for any other
>>>> usage, that could be corrupted by the presence of printf(),
>>>> assert() or alike.
>>>> - Child process inherits from parent's stderr in order to share
>>>> the same logging descriptor.
>>>>
>>>> Signed-off-by: Julien Vuillaumier <julien.vuillaumier at nxp.com>
>>>> ---
>>>>   src/libcamera/process.cpp | 11 ++++++++++-
>>>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp
>>>> index bc9833f4..86a887fb 100644
>>>> --- a/src/libcamera/process.cpp
>>>> +++ b/src/libcamera/process.cpp
>>>> @@ -12,6 +12,7 @@
>>>>   #include <fcntl.h>
>>>>   #include <list>
>>>>   #include <signal.h>
>>>> +#include <stdio.h>
>>>>   #include <string.h>
>>>>   #include <sys/socket.h>
>>>>   #include <sys/types.h>
>>>> @@ -259,7 +260,15 @@ 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);
>>>
>>> I believe the above part is fine.
>>>
>>>
>>>> +
>>>> +             UniqueFD fd(::open("/dev/null", O_RDWR));
>>>> +             if (fd.isValid()) {
>>>> +                     dup2(fd.get(), STDIN_FILENO);
>>>> +                     dup2(fd.get(), STDOUT_FILENO);
>>>> +             }
>>>
>>> But I am not sure about this. First, it does not handle the case when 
>>> `STD{IN,OUT}_FILENO`
>>> is already in `fds`. In that case the file descriptors will be 
>>> redirected even if that was
>>> not the intention. Second, if at least one of `STD{IN,OUT}_FILENO` is 
>>> not closed in the
>>> above `closeAllFdsExcept()` call, then `fd` will be 0 or 1, so the 
>>> later calls duplicate
>>> a file descriptor into itself, which just looks a bit odd, and might 
>>> confuse the readers
>>> of the code. Third, if both `STD{IN,OUT}_FILENO` is in `fds`, then 
>>> `fd` will be "leaked"
>>> into the new process since it is never closed if `execv()` succeeds.
>>
>> About your first point, intent was to unconditionally redirect 
>> stdin/stdout file descriptors to the null device. But letting the 
>> option to the parent process to share the stdin/stdout descriptors 
>> with its child makes the implementation more generic indeed.
>>
>> I agree with your other points: the possible occurence of descriptor 
>> self duplication is awkward. Also, the null device file descriptor 
>> remains unnecessarily opened.
>>
>>>
>>> I think the following should address both issues (mostly untested):
>>>
>>>
>>>                 /* closeAllFdExcept(...) */
>>>
>>>                 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);
>>
>> Having tested your proposal, it works fine and addresses above issues. 
>> Also, to support the (unlikely?) case where stderr could be closed by 
>> the parent process, statement below may be added to make sure fd=2 is 
>> always bound to something in the child process:
>>
>>      tryDevNullLowestFd(STDERR_FILENO, O_WRONLY);
> 
> I have a version of this change locally, and in that I also added this,
> so I agree that this should be part of it.

Then I will add the STDERR_FILENO step.

> I am wondering if maybe the `fd < 0` condition should be extended with
> `&& errno != ENFILE && errno != EMFILE`.

A new process running out of file descriptors suggests there is 
something wrong in the system - aborting with exit() can be a reasonable 
option here. In the present case of the isolated IPA, it typically would 
not be able to open a calibration file anyway.

This addition may cause issues in case of ENFILE/EMFILE occurence:
- invalid file descriptor will be close() which can be confusing
- the logic of sequentially testing in order stdin/stdout/stderr no 
longer works reliably: ENFILE could be returned during stdin step, 
meaning that fd=0 would remain free - later, during the stdout step, 
null device open() may succeed and bound to fd=0 ; then the comparison 
logic with the target fd=1 will fail, and both fd=0 and fd=1 will remain 
free.

So it seems the child process could run with free file descriptors fd=0 
or fd=1 which is risky. Therefore, I would suggest not adding this 
ENFILE / EMFILE test.

Thanks,
Julien

>>
>>
>> Thus, I'll update with a V3 based on your proposal. As the resulting 
>> patch will be mostly based on your code, do you want to be mentioned 
>> with some tag in the commit message?
> 
> No, it's fine.
> 
> 
> Regards,
> Barnabás Pőcze
> 
>>
>>>
>>>
>>> Regards,
>>> Barnabás Pőcze
>>>
>>>>
>>>>               const char *file = 
>>>> utils::secure_getenv("LIBCAMERA_LOG_FILE");
>>>>               if (file && strcmp(file, "syslog"))
>>>
>>
>> Thanks,
>> Julien
>