On Mon, Feb 24, 2020 at 09:02:21AM +0100, Tomas Mraz wrote:
On Sat, 2020-02-22 at 23:36 +0300, Dmitry V. Levin wrote:
> On Fri, Jan 17, 2020 at 04:06:15PM +0100, Tomas Mraz wrote:
> > The pam_modutil_sanitize_fd() when called to provide a dummy pipes
> > for
> > stdout and stderr closes the read end of the newly created dummy
> > pipe.
> > This is wrong as any writes to such pipe will trigger SIGPIPE. The
> > original code did a trick to avoid that by dup2-ing the read end of
> > the
> > pipe as stdin. However it does not necessarily have to be stdin, we
> > just need to ensure the read end of the pipe is not closed.
>
> I didn't want to leave the read end of the pipe open, but
> SIGPIPE is not a price I'm ready to pay for that.
>
> > The attached patch implements the fix. OK to commit?
>
> There are some technical issues with the patch.
>
> [...]
> > diff --git a/libpam/pam_modutil_sanitize.c
> > b/libpam/pam_modutil_sanitize.c
> > index 605c859..bad41eb 100644
> > --- a/libpam/pam_modutil_sanitize.c
> > +++ b/libpam/pam_modutil_sanitize.c
> > @@ -60,7 +60,16 @@ redirect_out_pipe(pam_handle_t *pamh, int fd,
> > const char *name)
>
> The function has a comment saying that it
> "Creates a pipe, closes its read end, redirects fd to its write end".
> If the contract is changed, the comment has to be updated.
OK, I'll update the comment.
> > return -1;
> > }
> >
> > - close(out[0]);
> > + /* try to keep the read end open */
>
> I think this function can either succeed or fail,
> it shall not leave the caller in an incomplete state.
I disagree here if by the fail you mean that the read end of the pipe
can be closed in case of the dup() failure. It is better to try the
subsequent execution in that case anyway, it might work.
Yet I feel uncomfortable to leave the read end of the pipe around.
Besides that, the writer can block when the pipe buffer is filled
which is also not good.
What if we use a slightly different trick: close the write end of the pipe
and return the read end instead of the write end? The unlucky writer
would get EBADF instead of EPIPE/SIGPIPE which is much better in case
of unprepared writer.
--
ldv