| Message ID | 20241218182754.2414920-2-julien.vuillaumier@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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"))
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"))
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(-)