| Message ID | 20250519122352.1675360-2-julien.vuillaumier@nxp.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Julien Vuillaumier (2025-05-19 14:23:52) > 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(-) > > diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp > index 68fad327..fa92f58c 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 >
Hi 2025. 05. 19. 14:23 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 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> > --- > 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 68fad327..fa92f58c 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); I have missed this earlier, but I think this should be `_exit()` since calling any `at{,_quick_}exit()` handlers is likely problematic. Regards, Barnabás Pőcze > + 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"))
Hi Julien, Thank you for the patch. On Mon, May 19, 2025 at 02:23:52PM +0200, Julien Vuillaumier wrote: > 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> > --- > 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 68fad327..fa92f58c 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); There's still a risk that STDERR_FILENO refers to a different file than stderr. Could you test this patch (with an IPA module running in isolated mode) with pipewire ? If that doesn't cause any issue I'll feel better accepting the risk. > + 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"))
Hello Barnabás, On 21/05/2025 22:53, Barnabás Pőcze wrote: > Hi > > > 2025. 05. 19. 14:23 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 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> >> --- >> 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 68fad327..fa92f58c 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); > > I have missed this earlier, but I think this should be `_exit()` since > calling > any `at{,_quick_}exit()` handlers is likely problematic. After some googling, recommendation seems to be using _exit() instead of exit() in the child process, for occurrences that are not invoked within an exec() - that is to avoid interfering with the parent process [1]. So I'll add that change, thanks for the suggestion. While at it, the `exit(EXIT_FAILURE)` in case of execv() failure in the existing implementation does not seem correct and presumably needs to be converted to an _exit() as well. [1] https://stackoverflow.com/a/5423108 Thanks, Julien
Hi Laurent, On 22/05/2025 01:04, Laurent Pinchart wrote: > Hi Julien, > > Thank you for the patch. > > On Mon, May 19, 2025 at 02:23:52PM +0200, Julien Vuillaumier wrote: >> 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> >> --- >> 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 68fad327..fa92f58c 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); > > There's still a risk that STDERR_FILENO refers to a different file than > stderr. Could you test this patch (with an IPA module running in > isolated mode) with pipewire ? If that doesn't cause any issue I'll feel > better accepting the risk. I tested libcamera (v0.5.0 tag) running with vimc isolated IPA, operated from PipeWire / WirePlumber - both components are run as systemd services. By default, systemd explicitly binds the standard descriptors of the services per mapping below: - STDIN_FILENO: /dev/null - STDOUT_FILENO: journal socket - STDERR_FILENO: same as STDOUT But this mapping could be changed in the respective service configuration files, where standard descriptors may also be associated to a dedicated file, kmsg, a socket etc - see `Logging and Standard Input/Output` in [1]. Thus, when libcamera is invoked from either PipeWire or WirePlumber service, the STDIN/STDOUT/STERR descriptors are already allocated and bound to some descriptors relevant for the standard input/output which is presumably safe. I suppose that any other configuration (standard descriptors not allocated to standard IO) is unsafe. I have verified vimc IPA in isolated mode: this change enables the isolated IPA to output its log in the PipeWire unit journal (`journalctrl --user -u pipewire -f`). The journal daemon socket descriptor mapped onto PipeWire STDERR_FILENO is inherited by the isolated IPA as expected. Side note: using isolated IPA with PW requires commenting out the line `RestrictNamespaces=yes` in the pipewire.service file [2]. No such issue exists in WirePlumber. [1]https://www.freedesktop.org/software/systemd/man/latest/systemd.exec.html [2] https://github.com/PipeWire/pipewire/blob/master/src/daemon/systemd/user/pipewire.service.in#L23 >> + 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")) Thanks, Julien
Hello Julien, On Mon, May 26, 2025 at 07:55:19PM +0200, Julien Vuillaumier wrote: > On 22/05/2025 01:04, Laurent Pinchart wrote: > > On Mon, May 19, 2025 at 02:23:52PM +0200, Julien Vuillaumier wrote: > >> 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> > >> --- > >> 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 68fad327..fa92f58c 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); > > > > There's still a risk that STDERR_FILENO refers to a different file than > > stderr. Could you test this patch (with an IPA module running in > > isolated mode) with pipewire ? If that doesn't cause any issue I'll feel > > better accepting the risk. > > I tested libcamera (v0.5.0 tag) running with vimc isolated IPA, operated > from PipeWire / WirePlumber - both components are run as systemd > services. By default, systemd explicitly binds the standard descriptors > of the services per mapping below: > - STDIN_FILENO: /dev/null > - STDOUT_FILENO: journal socket > - STDERR_FILENO: same as STDOUT > But this mapping could be changed in the respective service > configuration files, where standard descriptors may also be associated > to a dedicated file, kmsg, a socket etc - see `Logging and Standard > Input/Output` in [1]. > > Thus, when libcamera is invoked from either PipeWire or WirePlumber > service, the STDIN/STDOUT/STERR descriptors are already allocated and > bound to some descriptors relevant for the standard input/output which > is presumably safe. I suppose that any other configuration (standard > descriptors not allocated to standard IO) is unsafe. > > I have verified vimc IPA in isolated mode: this change enables the > isolated IPA to output its log in the PipeWire unit journal > (`journalctrl --user -u pipewire -f`). The journal daemon socket > descriptor mapped onto PipeWire STDERR_FILENO is inherited by the > isolated IPA as expected. > > Side note: using isolated IPA with PW requires commenting out the line > `RestrictNamespaces=yes` in the pipewire.service file [2]. > No such issue exists in WirePlumber. > > [1] https://www.freedesktop.org/software/systemd/man/latest/systemd.exec.html > [2] https://github.com/PipeWire/pipewire/blob/master/src/daemon/systemd/user/pipewire.service.in#L23 Thank you for the thorough testing. This alleviates my worries. With the small change requested by Barnabás (switching from exit() to _exit()), I think we're good to go. > >> + 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"))
diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp index 68fad327..fa92f58c 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"))
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> --- src/libcamera/process.cpp | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)