On Thu, Aug 14, 2014 at 03:50:31PM +0400, Dmitry V. Levin wrote:
Hi,
On Thu, Aug 14, 2014 at 08:40:09AM +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 :).
Besides the fact that adding close calls right before _exit is useless,
unfortunately, this patch also breaks things:
> modules/pam_exec/pam_exec.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/modules/pam_exec/pam_exec.c b/modules/pam_exec/pam_exec.c
> index 12c4444..87b542d 100644
> --- a/modules/pam_exec/pam_exec.c
> +++ b/modules/pam_exec/pam_exec.c
> @@ -371,13 +371,17 @@ call_exec (const char *pam_type, pam_handle_t *pamh,
> dup2 (STDOUT_FILENO, STDERR_FILENO) == -1)
> {
> int err = errno;
> + if (logfile) close(i);
> pam_syslog (pamh, LOG_ERR, "dup2 failed: %m");
> _exit (err);
> }
This close() call clobbers errno.
I don't understand. Errno is saved _before_
close call.
pam_syslog doesn't use errno.
> if (pam_modutil_sanitize_helper_fds(pamh, redirect_stdin,
> redirect_stdout, redirect_stdout) < 0)
> + {
> + if (logfile) close(i);
> _exit(1);
> + }
>
> if (call_setuid)
> if (setuid (geteuid ()) == -1)
> @@ -385,6 +389,7 @@ call_exec (const char *pam_type, pam_handle_t *pamh,
> int err = errno;
> pam_syslog (pamh, LOG_ERR, "setuid(%lu) failed: %m",
> (unsigned long) geteuid ());
> + if (logfile) close(i);
> _exit (err);
> }
>
> @@ -392,12 +397,15 @@ call_exec (const char *pam_type, pam_handle_t *pamh,
> {
> int err = errno;
> pam_syslog (pamh, LOG_ERR, "setsid failed: %m");
> + if (logfile) close(i);
> _exit (err);
> }
>
> arggv = calloc (argc + 4, sizeof (char *));
> - if (arggv == NULL)
> + if (arggv == NULL) {
> + if (logfile) close(i);
> _exit (ENOMEM);
> + }
>
> for (i = 0; i < (argc - optargc); i++)
After this point, i is no longer a file descriptor, so...
Yes. You are right. Call
me blind man.
> arggv[i] = strdup(argv[i+optargc]);
> @@ -417,6 +425,7 @@ call_exec (const char *pam_type, pam_handle_t *pamh,
> {
> free(envlist);
> pam_syslog (pamh, LOG_ERR, "realloc environment failed: %m");
> + if (logfile) close(i);
> _exit (ENOMEM);
> }
> envlist = tmp;
> @@ -430,6 +439,7 @@ call_exec (const char *pam_type, pam_handle_t *pamh,
> {
> free(envlist);
> pam_syslog (pamh, LOG_ERR, "prepare environment failed: %m");
> + if (logfile) close(i);
> _exit (ENOMEM);
> }
> envlist[envlen++] = envstr;
> @@ -440,6 +450,7 @@ call_exec (const char *pam_type, pam_handle_t *pamh,
> {
> free(envlist);
> pam_syslog (pamh, LOG_ERR, "prepare environment failed: %m");
> + if (logfile) close(i);
> _exit (ENOMEM);
> }
> envlist[envlen++] = envstr;
> @@ -452,6 +463,7 @@ call_exec (const char *pam_type, pam_handle_t *pamh,
> i = errno;
> pam_syslog (pamh, LOG_ERR, "execve(%s,...) failed: %m", arggv[0]);
> free(envlist);
> + if (logfile) close(i);
> _exit (i);
> }
> return PAM_SYSTEM_ERR; /* will never be reached. */
... all these added close() calls are wrong.
I must agree. Thanks for review.
--
ldv