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

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

Commit Message

Julien Vuillaumier May 27, 2025, 3:56 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 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@nxp.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/libcamera/process.cpp | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

Comments

Kieran Bingham May 27, 2025, 4:06 p.m. UTC | #1 
Quoting Julien Vuillaumier (2025-05-27 16:56:15)
> 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@nxp.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

I'm looking forward to this one being in, especially if it helps report
issues from isolated IPA modules, which have caused me support pain!

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.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 7f3a6518..d836fb07 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);
> +                       if (fd != expected)
> +                               close(fd);
> +               };
> +
> +               tryDevNullLowestFd(STDIN_FILENO, O_RDONLY);
> +               tryDevNullLowestFd(STDOUT_FILENO, O_WRONLY);
> +               tryDevNullLowestFd(STDERR_FILENO, O_WRONLY);
>  
>                 const char *file = utils::secure_getenv("LIBCAMERA_LOG_FILE");
>                 if (file && strcmp(file, "syslog"))
> -- 
> 2.34.1
>

Patch
diff mbox series 

diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp
index 7f3a6518..d836fb07 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);
+			if (fd != expected)
+				close(fd);
+		};
+
+		tryDevNullLowestFd(STDIN_FILENO, O_RDONLY);
+		tryDevNullLowestFd(STDOUT_FILENO, O_WRONLY);
+		tryDevNullLowestFd(STDERR_FILENO, O_WRONLY);
 
 		const char *file = utils::secure_getenv("LIBCAMERA_LOG_FILE");
 		if (file && strcmp(file, "syslog"))