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:
diff --git a/libpam/pam_modutil_sanitize.c b/libpam/pam_modutil_sanitize.c
index 605c859d..58b9537c 100644
--- a/libpam/pam_modutil_sanitize.c
+++ b/libpam/pam_modutil_sanitize.c
@@ -46,34 +46,6 @@ redirect_in_pipe(pam_handle_t *pamh, int fd, const char *name)
return fd;
}
-/*
- * Creates a pipe, closes its read end, redirects fd to its write end.
- * Returns fd on success, -1 otherwise.
- */
-static int
-redirect_out_pipe(pam_handle_t *pamh, int fd, const char *name)
-{
- int out[2];
-
- if (pipe(out) < 0) {
- pam_syslog(pamh, LOG_ERR, "Could not create pipe: %m");
- return -1;
- }
-
- close(out[0]);
-
- if (out[1] == fd)
- return fd;
-
- if (dup2(out[1], fd) != fd) {
- pam_syslog(pamh, LOG_ERR, "dup2 of %s failed: %m", name);
- fd = -1;
- }
-
- close(out[1]);
- return fd;
-}
-
/*
* Opens /dev/null for writing, redirects fd there.
* Returns fd on success, -1 otherwise.
@@ -106,7 +78,7 @@ redirect_out(pam_handle_t *pamh, enum pam_modutil_redirect_fd mode,
{
switch (mode) {
case PAM_MODUTIL_PIPE_FD:
- if (redirect_out_pipe(pamh, fd, name) < 0)
+ if (redirect_in_pipe(pamh, fd, name) < 0)
return -1;
break;
case PAM_MODUTIL_NULL_FD:
--
ldv