[PATCH 2/2] pam_exec: Make coverity happy by closing descriptors.

[Robin Hack]

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 :).

---
 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);
 	}
 
       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++)
 	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. */
-- 
1.9.3

[Dmitry V. Levin]

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.

>         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...

>   	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.


-- 
ldv

[Robin Hack]

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

[Tomas Mraz]

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.

>  ---
>   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);
>   	}
>   
>         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++)
>   	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. */ 
-- 
Tomas Mraz
No matter how far down the wrong road you've gone, turn back.
                                              Turkish proverb
(You'll never know whether the road is wrong though.)

[Thorsten Kukuk]

On Thu, Aug 14, 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(). 
Since _exit closes all open file descriptors, we should not add
the additional "close()" calls.

  Thorsten

-- 
Thorsten Kukuk, Senior Architect SLES & Common Code Base
SUSE LINUX Products GmbH, Maxfeldstr. 5, D-90409 Nuernberg
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg)

[Dmitry V. Levin]

On Thu, Aug 14, 2014 at 01:59:07PM +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(). 
The only point in an explicit close() call before _exit() is testing
close() return code.  Without that testing, there is no use to call
close() right before _exit().


-- 
ldv

[Tomas Mraz]

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?
-- 
Tomas Mraz
No matter how far down the wrong road you've gone, turn back.
                                              Turkish proverb
(You'll never know whether the road is wrong though.)

[Tomas Mraz]

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.

-- 
Tomas Mraz
No matter how far down the wrong road you've gone, turn back.
                                              Turkish proverb
(You'll never know whether the road is wrong though.)

[Dmitry V. Levin]

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

[Robin Hack]

Ok. My fault. The ‘%m’ conversion prints the string corresponding to the error code in
errno.
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 :).
>  
>  ---
>   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);
>   	}
>   
>         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++)
>   	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. */
>  -- 
>  1.9.3
>