| 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 > >
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(+)