[EXT] Re: [PATCH v1] libcamera: ipc_unixsocket: Share stdin and stdout with IPA proxy

[Julien Vuillaumier]

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 at 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