On Wed, 2020-02-26 at 16:20 +0300, Dmitry V. Levin wrote:
On Wed, Feb 26, 2020 at 04:10:29PM +0300, Dmitry V. Levin wrote:
> 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.
I mean something like this:
That's great idea. Please commit it.
--
Tomáš Mráz
No matter how far down the wrong road you've gone, turn back.
Turkish proverb
[You'll know whether the road is wrong if you carefully listen to your
conscience.]