On Mon, Aug 25, 2014 at 05:40:33PM +0200, Tomas Mraz wrote:
On Pá, 2014-08-22 at 17:40 +0200, Tomas Mraz wrote:
> On Čt, 2014-08-14 at 13:59 +0200, Tomas Mraz wrote:
> > On Čt, 2014-08-14 at 08:40 +0200, Robin Hack wrote:
> > > Hi all.
> > >
> > > I know that kernel will close opened file handlers on the end of process
> > > life... This will make coverity more happy and also it's good habit
to
> > > cleanup your mess :).
> >
> > I'd like to have an opinion of other Linux PAM upstream developers
> > whether we want to fix all these cases where we do not close file
> > descriptors before _exit().
> >
> > Also there is a problem this patch is not correct although the current
> > implementation is disputable. The current implementation uses this file
> > to replace the stdout but this is done by close(STDOUT_FILENO) just
> > before the open() call but there is no guarantee that STDIN is opened at
> > that time, which means the file descriptor might be in reality
> > STDIN_FILENO.
> >
> > The attached patch fixes it. And I do not think closing STDOUT_FILENO is
> > appropriate as in theory something could be written to it even during
> > the _exit() call.
>
> diff --git a/modules/pam_exec/pam_exec.c b/modules/pam_exec/pam_exec.c
> index 12c4444..0f18fcc 100644
> --- a/modules/pam_exec/pam_exec.c
> +++ b/modules/pam_exec/pam_exec.c
> @@ -360,9 +360,20 @@ call_exec (const char *pam_type, pam_handle_t *pamh,
> logfile);
> _exit (err);
> }
> + if (i != STDOUT_FILENO)
> + {
> + if (dup2 (i, STDOUT_FILENO) == -1)
> + {
> + int err = errno;
> + pam_syslog (pamh, LOG_ERR, "dup2 failed: %m");
> + close (i);
> + _exit (err);
> + }
> + close (i);
> + }
> if (asprintf (&buffer, "*** %s", ctime (&tm)) > 0)
> {
> - pam_modutil_write (i, buffer, strlen (buffer));
> + pam_modutil_write (STDOUT_FILENO, buffer, strlen (buffer));
> free (buffer);
> }
> }
>
> Anybody wants to comment on this patch above, that tries to avoid
> depending on the stdin in pam_exec being opened?
I will commit the patch above unless anybody has objections as depending
on the open() to return STDOUT_FILENO is hacky.
close(i) before _exit(err) is not needed; besides that, the fix is OK.
There is a chance that pam_syslog will be called while stdout is closed
(if either open or dup2 fails), resulting to syslog() being called with
stdout closed, with a chance of creating a syslog descriptor
STDOUT_FILENO.
Fortunately, that is probably harmless due to subsequent _exit() call.
Of course it doesn't look as safe as traditional open+dup2+close.
--
ldv