| Message ID | 20241106154306.2490129-1-julien.vuillaumier@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Julien, Quoting Julien Vuillaumier (2024-11-06 15:43:06) > When IPA is running in isolated mode, at IPA process creation time > all the inherited file descriptors are closed in the new process > after the fork, with the exception of the file descriptor relevant > to the IPC peer Unix socket. > > In order to enable the IPA logging in the console, add stdout and > stderr to the list of file descriptors to be shared with the > isolated process. > > Signed-off-by: Julien Vuillaumier <julien.vuillaumier@nxp.com> > --- > src/libcamera/ipc_pipe_unixsocket.cpp | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/src/libcamera/ipc_pipe_unixsocket.cpp b/src/libcamera/ipc_pipe_unixsocket.cpp > index 668ec73b..9ca219dc 100644 > --- a/src/libcamera/ipc_pipe_unixsocket.cpp > +++ b/src/libcamera/ipc_pipe_unixsocket.cpp > @@ -7,6 +7,7 @@ > > #include "libcamera/internal/ipc_pipe_unixsocket.h" > > +#include <unistd.h> > #include <vector> > > #include <libcamera/base/event_dispatcher.h> > @@ -42,6 +43,10 @@ IPCPipeUnixSocket::IPCPipeUnixSocket(const char *ipaModulePath, > args.push_back(std::to_string(fd.get())); > fds.push_back(fd.get()); > > + /* Share stdout and stderr with the proxy for logging purpose */ > + fds.push_back(STDOUT_FILENO); > + fds.push_back(STDERR_FILENO); > + Is this all that's required ? Is there a corresponding change in the proxy to tie the spawned process stdout/stderr to these fds ? -- Kieran > proc_ = std::make_unique<Process>(); > int ret = proc_->start(ipaProxyWorkerPath, args, fds); > if (ret) { > -- > 2.34.1 >
Quoting Kieran Bingham (2024-11-06 15:59:48) > Hi Julien, > > Quoting Julien Vuillaumier (2024-11-06 15:43:06) > > When IPA is running in isolated mode, at IPA process creation time > > all the inherited file descriptors are closed in the new process > > after the fork, with the exception of the file descriptor relevant > > to the IPC peer Unix socket. > > > > In order to enable the IPA logging in the console, add stdout and > > stderr to the list of file descriptors to be shared with the > > isolated process. > > > > Signed-off-by: Julien Vuillaumier <julien.vuillaumier@nxp.com> > > --- > > src/libcamera/ipc_pipe_unixsocket.cpp | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/src/libcamera/ipc_pipe_unixsocket.cpp b/src/libcamera/ipc_pipe_unixsocket.cpp > > index 668ec73b..9ca219dc 100644 > > --- a/src/libcamera/ipc_pipe_unixsocket.cpp > > +++ b/src/libcamera/ipc_pipe_unixsocket.cpp > > @@ -7,6 +7,7 @@ > > > > #include "libcamera/internal/ipc_pipe_unixsocket.h" > > > > +#include <unistd.h> > > #include <vector> > > > > #include <libcamera/base/event_dispatcher.h> > > @@ -42,6 +43,10 @@ IPCPipeUnixSocket::IPCPipeUnixSocket(const char *ipaModulePath, > > args.push_back(std::to_string(fd.get())); > > fds.push_back(fd.get()); > > > > + /* Share stdout and stderr with the proxy for logging purpose */ > > + fds.push_back(STDOUT_FILENO); > > + fds.push_back(STDERR_FILENO); > > + > > Is this all that's required ? Is there a corresponding change in the > proxy to tie the spawned process stdout/stderr to these fds ? Aha! So now I've dug deeper, indeed I see this is all that's required... The proc_->start() calls: closeAllFdsExcept(fds); So - by adding stdout,stderr here we keep those mapped... Which makes this ... really quite nice, simple and elegant. Right now - I don't have any reasons why we couldn't do this. I'm curious to know what security implications might come up from allowing stdout/stderr to be written to from a potentially closed source binary ... but aside from flooding a log - I can't imagine anything worse than that right now ? (cue someone telling me about vulnerabilities in terminal emulators for some carefully crafted character sequence to delete the harddrive ... in 3 ... 2 ... 1 ... ) There are already many occasions where this would have made my life much easier or things easier to diagnose when users had incorrectly configured their libcamera build such that the IPA became unnecessarily isolated, and we had no logs to determine why the IPA crashed ... so for that at least - I'd be happy to see this in: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > -- > Kieran > > > > proc_ = std::make_unique<Process>(); > > int ret = proc_->start(ipaProxyWorkerPath, args, fds); > > if (ret) { > > -- > > 2.34.1 > >
Hi Julien, Thank you for the patch. On Wed, Nov 06, 2024 at 04:43:06PM +0100, Julien Vuillaumier wrote: > When IPA is running in isolated mode, at IPA process creation time > all the inherited file descriptors are closed in the new process > after the fork, with the exception of the file descriptor relevant > to the IPC peer Unix socket. > > In order to enable the IPA logging in the console, add stdout and > stderr to the list of file descriptors to be shared with the > isolated process. > > Signed-off-by: Julien Vuillaumier <julien.vuillaumier@nxp.com> > --- > src/libcamera/ipc_pipe_unixsocket.cpp | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/src/libcamera/ipc_pipe_unixsocket.cpp b/src/libcamera/ipc_pipe_unixsocket.cpp > index 668ec73b..9ca219dc 100644 > --- a/src/libcamera/ipc_pipe_unixsocket.cpp > +++ b/src/libcamera/ipc_pipe_unixsocket.cpp > @@ -7,6 +7,7 @@ > > #include "libcamera/internal/ipc_pipe_unixsocket.h" > > +#include <unistd.h> > #include <vector> > > #include <libcamera/base/event_dispatcher.h> > @@ -42,6 +43,10 @@ IPCPipeUnixSocket::IPCPipeUnixSocket(const char *ipaModulePath, > args.push_back(std::to_string(fd.get())); > fds.push_back(fd.get()); > > + /* Share stdout and stderr with the proxy for logging purpose */ > + fds.push_back(STDOUT_FILENO); > + fds.push_back(STDERR_FILENO); Those are the default file descriptors for stdout and stderr, but an application may have remapped those streams, or even closed them. All of a sudden you may end up writing to a file or any other fd that the application would have opened after closing stderr or stdout. > + > proc_ = std::make_unique<Process>(); > int ret = proc_->start(ipaProxyWorkerPath, args, fds); > if (ret) {
On Wed, Nov 06, 2024 at 04:43:06PM +0100, Julien Vuillaumier wrote: > When IPA is running in isolated mode, at IPA process creation time > all the inherited file descriptors are closed in the new process > after the fork, with the exception of the file descriptor relevant > to the IPC peer Unix socket. > > In order to enable the IPA logging in the console, add stdout and > stderr to the list of file descriptors to be shared with the > isolated process. > > Signed-off-by: Julien Vuillaumier <julien.vuillaumier@nxp.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/libcamera/ipc_pipe_unixsocket.cpp | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/src/libcamera/ipc_pipe_unixsocket.cpp b/src/libcamera/ipc_pipe_unixsocket.cpp > index 668ec73b..9ca219dc 100644 > --- a/src/libcamera/ipc_pipe_unixsocket.cpp > +++ b/src/libcamera/ipc_pipe_unixsocket.cpp > @@ -7,6 +7,7 @@ > > #include "libcamera/internal/ipc_pipe_unixsocket.h" > > +#include <unistd.h> > #include <vector> > > #include <libcamera/base/event_dispatcher.h> > @@ -42,6 +43,10 @@ IPCPipeUnixSocket::IPCPipeUnixSocket(const char *ipaModulePath, > args.push_back(std::to_string(fd.get())); > fds.push_back(fd.get()); > > + /* Share stdout and stderr with the proxy for logging purpose */ > + fds.push_back(STDOUT_FILENO); > + fds.push_back(STDERR_FILENO); > + > proc_ = std::make_unique<Process>(); > int ret = proc_->start(ipaProxyWorkerPath, args, fds); > if (ret) { > -- > 2.34.1 >
Hi Laurent, Thank you for your review comments. On 09/12/2024 10:39, Laurent Pinchart wrote: > Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button > > > Hi Julien, > > Thank you for the patch. > > On Wed, Nov 06, 2024 at 04:43:06PM +0100, Julien Vuillaumier wrote: >> When IPA is running in isolated mode, at IPA process creation time >> all the inherited file descriptors are closed in the new process >> after the fork, with the exception of the file descriptor relevant >> to the IPC peer Unix socket. >> >> In order to enable the IPA logging in the console, add stdout and >> stderr to the list of file descriptors to be shared with the >> isolated process. >> >> Signed-off-by: Julien Vuillaumier <julien.vuillaumier@nxp.com> >> --- >> src/libcamera/ipc_pipe_unixsocket.cpp | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/src/libcamera/ipc_pipe_unixsocket.cpp b/src/libcamera/ipc_pipe_unixsocket.cpp >> index 668ec73b..9ca219dc 100644 >> --- a/src/libcamera/ipc_pipe_unixsocket.cpp >> +++ b/src/libcamera/ipc_pipe_unixsocket.cpp >> @@ -7,6 +7,7 @@ >> >> #include "libcamera/internal/ipc_pipe_unixsocket.h" >> >> +#include <unistd.h> >> #include <vector> >> >> #include <libcamera/base/event_dispatcher.h> >> @@ -42,6 +43,10 @@ IPCPipeUnixSocket::IPCPipeUnixSocket(const char *ipaModulePath, >> args.push_back(std::to_string(fd.get())); >> fds.push_back(fd.get()); >> >> + /* Share stdout and stderr with the proxy for logging purpose */ >> + fds.push_back(STDOUT_FILENO); >> + fds.push_back(STDERR_FILENO); > > Those are the default file descriptors for stdout and stderr, but an > application may have remapped those streams, or even closed them. All of > a sudden you may end up writing to a file or any other fd that the > application would have opened after closing stderr or stdout. When a daemon closes STDIN/STDOUT/STDERR file descriptors to detach from the tty it was started from, I think that a common practice is to reallocate them to /dev/null using dup2(). That is to remove the risk of having later a file descriptor created by opening a file or socket, reusing one of the 0/1/2 FDs. Such file or socket could be corrupted indeed in case of perror(), assert() or some sort of printf(). Now libcamera also uses e.g. assert() whose output message is routed to STDERR. Thus, if the calling application closes FD=2 without reallocating it to something, such risk of corruption is present. To my understanding, I would think that: - The application linking with libcamera takes some risks if it closes either of STDIN/STDOUT/STDERR FDs and keeps them unallocated - also if reallocated, it should be to something relevant to STDIN/STDOUT/STDERR usage - Likewise, starting the IPA proxy with STDIN/STDOUT/STDERR unallocated may not be a safe option: STDIN/STDOUT may better be reassigned to /dev/null, and STDERR could be inherited from the parent. (That is not what the current patch is doing... maybe what should be done?) Thanks, Julien > >> + >> proc_ = std::make_unique<Process>(); >> int ret = proc_->start(ipaProxyWorkerPath, args, fds); >> if (ret) { > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/ipc_pipe_unixsocket.cpp b/src/libcamera/ipc_pipe_unixsocket.cpp index 668ec73b..9ca219dc 100644 --- a/src/libcamera/ipc_pipe_unixsocket.cpp +++ b/src/libcamera/ipc_pipe_unixsocket.cpp @@ -7,6 +7,7 @@ #include "libcamera/internal/ipc_pipe_unixsocket.h" +#include <unistd.h> #include <vector> #include <libcamera/base/event_dispatcher.h> @@ -42,6 +43,10 @@ IPCPipeUnixSocket::IPCPipeUnixSocket(const char *ipaModulePath, args.push_back(std::to_string(fd.get())); fds.push_back(fd.get()); + /* Share stdout and stderr with the proxy for logging purpose */ + fds.push_back(STDOUT_FILENO); + fds.push_back(STDERR_FILENO); + proc_ = std::make_unique<Process>(); int ret = proc_->start(ipaProxyWorkerPath, args, fds); if (ret) {
When IPA is running in isolated mode, at IPA process creation time all the inherited file descriptors are closed in the new process after the fork, with the exception of the file descriptor relevant to the IPC peer Unix socket. In order to enable the IPA logging in the console, add stdout and stderr to the list of file descriptors to be shared with the isolated process. Signed-off-by: Julien Vuillaumier <julien.vuillaumier@nxp.com> --- src/libcamera/ipc_pipe_unixsocket.cpp | 5 +++++ 1 file changed, 5 insertions(+)