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?