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

[Barnabás Pőcze]

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

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