[v2,1/1] libcamera: process: Pass stderr and reserve stdin and stdout fds
diff mbox series

Message ID 20241218182754.2414920-2-julien.vuillaumier@nxp.com
State Accepted
Headers show
Series
  • libcamera: process: Pass stderr and reserve stdin and stdout fds
Related show

Commit Message

Julien Vuillaumier Dec. 18, 2024, 6:27 p.m. UTC
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@nxp.com>
---
 src/libcamera/process.cpp | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Comments

Barnabás Pőcze April 24, 2025, 10:23 a.m. UTC | #1
Hi


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

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


Regards,
Barnabás Pőcze

>   
>   		const char *file = utils::secure_getenv("LIBCAMERA_LOG_FILE");
>   		if (file && strcmp(file, "syslog"))
Julien Vuillaumier May 16, 2025, 2:23 p.m. UTC | #2
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@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);


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?

> 
> 
> Regards,
> Barnabás Pőcze
> 
>>
>>               const char *file = 
>> utils::secure_getenv("LIBCAMERA_LOG_FILE");
>>               if (file && strcmp(file, "syslog"))
> 

Thanks,
Julien
Barnabás Pőcze May 16, 2025, 3:26 p.m. UTC | #3
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@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.

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


> 
> 
> 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
Julien Vuillaumier May 19, 2025, 8:30 a.m. UTC | #4
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@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
>
Barnabás Pőcze May 19, 2025, 8:59 a.m. UTC | #5
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@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
>>
>

Patch
diff mbox series

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);
+
+		UniqueFD fd(::open("/dev/null", O_RDWR));
+		if (fd.isValid()) {
+			dup2(fd.get(), STDIN_FILENO);
+			dup2(fd.get(), STDOUT_FILENO);
+		}
 
 		const char *file = utils::secure_getenv("LIBCAMERA_LOG_FILE");
 		if (file && strcmp(file, "syslog"))