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

[Barnabás Pőcze]

Hi


2025. 05. 19. 10:30 keltezéssel, Julien Vuillaumier írta:
> 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

That's true, but I don't think it is a big issue.

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

Ahh, you're right about ENFILE. Let's just exit when open() goes wrong for any reason.


Regards,
Barnabás Pőcze


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