[PATCH 1/7] don't use custom MAX macro

[PATCH 1/7] don't use custom MAX macro

[Ludwig Nussel]

Signed-off-by: Ludwig Nussel <ludwig.nussel@suse.de>
---
 login-utils/su.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/login-utils/su.c b/login-utils/su.c
index 0e89632..77722c4 100644
--- a/login-utils/su.c
+++ b/login-utils/su.c
@@ -37,10 +37,6 @@
 
    Based on an implemenation by David MacKenzie <djm@gnu.ai.mit.edu>.  */
 
-#ifndef MAX
-# define MAX(a,b) ((a) > (b) ? (a) : (b))
-#endif
-
 /* Exit statuses for programs like 'env' that exec other programs.
    EXIT_FAILURE might not be 1, so use EXIT_FAIL in such programs.  */
 enum
@@ -817,7 +813,7 @@ main (int argc, char **argv)
   if (simulate_login && chdir (pw->pw_dir) != 0)
     error (0, errno, _("warning: cannot change directory to %s"), pw->pw_dir);
 
-  run_shell (shell, command, argv + optind, MAX (0, argc - optind));
+  run_shell (shell, command, argv + optind, max (0, argc - optind));
 }
 
 // vim: sw=2 cinoptions=>4,n-2,{2,^-2,\:2,=2,g0,h2,p5,t0,+2,(0,u0,w1,m1
-- 
1.7.7

[PATCH 2/7] use EXIT_FAILURE consistently

[Ludwig Nussel]

Signed-off-by: Ludwig Nussel <ludwig.nussel@suse.de>
---
 login-utils/su.c |   15 ++++++---------
 1 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/login-utils/su.c b/login-utils/su.c
index 77722c4..b1fca5f 100644
--- a/login-utils/su.c
+++ b/login-utils/su.c
@@ -37,11 +37,8 @@
 
    Based on an implemenation by David MacKenzie <djm@gnu.ai.mit.edu>.  */
 
-/* Exit statuses for programs like 'env' that exec other programs.
-   EXIT_FAILURE might not be 1, so use EXIT_FAIL in such programs.  */
 enum
 {
-  EXIT_FAIL = 1,
   EXIT_CANNOT_INVOKE = 126,
   EXIT_ENOENT = 127
 };
@@ -555,7 +552,7 @@ init_groups (const struct passwd *pw)
   if (initgroups (pw->pw_name, pw->pw_gid) == -1)
     {
       cleanup_pam (PAM_ABORT);
-      error (EXIT_FAIL, errno, _("cannot set groups"));
+      error (EXIT_FAILURE, errno, _("cannot set groups"));
     }
   endgrent ();
 
@@ -570,9 +567,9 @@ static void
 change_identity (const struct passwd *pw)
 {
   if (setgid (pw->pw_gid))
-    error (EXIT_FAIL, errno, _("cannot set group id"));
+    error (EXIT_FAILURE, errno, _("cannot set group id"));
   if (setuid (pw->pw_uid))
-    error (EXIT_FAIL, errno, _("cannot set user id"));
+    error (EXIT_FAILURE, errno, _("cannot set user id"));
 }
 
 /* Run SHELL, or DEFAULT_SHELL if SHELL is empty.
@@ -734,7 +731,7 @@ main (int argc, char **argv)
 	  exit(EXIT_SUCCESS);
 
 	default:
-	  usage (EXIT_FAIL);
+	  usage (EXIT_FAILURE);
 	}
     }
 
@@ -751,7 +748,7 @@ main (int argc, char **argv)
   pw = getpwnam (new_user);
   if (! (pw && pw->pw_name && pw->pw_name[0] && pw->pw_dir && pw->pw_dir[0]
 	 && pw->pw_passwd))
-    error (EXIT_FAIL, 0, _("user %s does not exist"), new_user);
+    error (EXIT_FAILURE, 0, _("user %s does not exist"), new_user);
 
   /* Make a copy of the password information and point pw at the local
      copy instead.  Otherwise, some systems (e.g. Linux) would clobber
@@ -773,7 +770,7 @@ main (int argc, char **argv)
     {
       log_su (pw, false);
       sleep (getlogindefs_num ("FAIL_DELAY", 1));
-      error (EXIT_FAIL, 0, _("incorrect password"));
+      error (EXIT_FAILURE, 0, _("incorrect password"));
     }
   else
     {
-- 
1.7.7

[PATCH 3/7] use ENV_PATH resp ENV_SUPATH to be consistent with login

[Ludwig Nussel]

Signed-off-by: Ludwig Nussel <ludwig.nussel@suse.de>
---
 login-utils/su.c |   24 ++++++++++++++++--------
 1 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/login-utils/su.c b/login-utils/su.c
index b1fca5f..22b9ccc 100644
--- a/login-utils/su.c
+++ b/login-utils/su.c
@@ -476,6 +476,20 @@ clearsbin (const char *const path)
   return ret;
 }
 
+static void
+set_path(const struct passwd* pw)
+{
+  int r;
+  if (pw->pw_uid)
+    r = logindefs_setenv("PATH", "ENV_PATH", _PATH_DEFPATH);
+
+  else if ((r = logindefs_setenv("PATH", "ENV_ROOTPATH", NULL)) != 0)
+    r = logindefs_setenv("PATH", "ENV_SUPATH", _PATH_DEFPATH_ROOT);
+
+  if (r != 0)
+    err (EXIT_FAILURE,  _("failed to set PATH"));
+}
+
 /* Update `environ' for the new shell based on PW, with SHELL being
    the value for the SHELL environment variable.  */
 
@@ -497,9 +511,7 @@ modify_environment (const struct passwd *pw, const char *shell)
       xsetenv ("SHELL", shell);
       xsetenv ("USER", pw->pw_name);
       xsetenv ("LOGNAME", pw->pw_name);
-      xsetenv ("PATH", (pw->pw_uid
-			? getlogindefs_str ("PATH", _PATH_DEFPATH)
-			: getlogindefs_str ("SUPATH", _PATH_DEFPATH_ROOT)));
+      set_path(pw);
     }
   else
     {
@@ -510,11 +522,7 @@ modify_environment (const struct passwd *pw, const char *shell)
           xsetenv ("HOME", pw->pw_dir);
           xsetenv ("SHELL", shell);
 	  if (getlogindefs_bool ("ALWAYS_SET_PATH", 0))
-	    xsetenv ("PATH", (pw->pw_uid
-			      ? getlogindefs_str ("PATH",
-					    _PATH_DEFPATH)
-			      : getlogindefs_str ("SUPATH",
-					    _PATH_DEFPATH_ROOT)));
+	    set_path(pw);
 	  else
 	    {
 	      char const *path = getenv ("PATH");
-- 
1.7.7

[PATCH 4/7] introduce xsetenv globally

[Ludwig Nussel]

Signed-off-by: Ludwig Nussel <ludwig.nussel@suse.de>
---
 include/env.h    |    7 +++++++
 login-utils/su.c |   36 +++++++++++-------------------------
 2 files changed, 18 insertions(+), 25 deletions(-)

diff --git a/include/env.h b/include/env.h
index bcd0f7e..9c6a3fd 100644
--- a/include/env.h
+++ b/include/env.h
@@ -4,5 +4,12 @@
 extern void sanitize_env (void);
 extern char *safe_getenv(const char *arg);
 
+static inline void
+xsetenv (char const *name, char const *val, int overwrite)
+{
+  if (setenv (name, val, overwrite) != 0)
+    err (EXIT_FAILURE,  _("failed to set the %s environment variable"), name);
+}
+
 #endif /* UTIL_LINUX_ENV_H */
 
diff --git a/login-utils/su.c b/login-utils/su.c
index 22b9ccc..cdaf31e 100644
--- a/login-utils/su.c
+++ b/login-utils/su.c
@@ -62,6 +62,7 @@ enum
 #include "xalloc.h"
 #include "nls.h"
 #include "pathnames.h"
+#include "env.h"
 
 /* name of the pam configuration files. separate configs for su and su -  */
 #define PAM_SERVICE_NAME "su"
@@ -111,21 +112,6 @@ static struct option const longopts[] =
   {NULL, 0, NULL, 0}
 };
 
-/* Add NAME=VAL to the environment, checking for out of memory errors.  */
-
-static void
-xsetenv (char const *name, char const *val)
-{
-  size_t namelen = strlen (name);
-  size_t vallen = strlen (val);
-  char *string = xmalloc (namelen + 1 + vallen + 1);
-  strcpy (string, name);
-  string[namelen] = '=';
-  strcpy (string + namelen + 1, val);
-  if (putenv (string) != 0)
-    error (EXIT_FAILURE, 0, _("out of memory"));
-}
-
 /* Log the fact that someone has run su to the user given by PW;
    if SUCCESSFUL is true, they gave the correct password, etc.  */
 
@@ -506,11 +492,11 @@ modify_environment (const struct passwd *pw, const char *shell)
       environ = xmalloc ((6 + !!term) * sizeof (char *));
       environ[0] = NULL;
       if (term)
-	xsetenv ("TERM", term);
-      xsetenv ("HOME", pw->pw_dir);
-      xsetenv ("SHELL", shell);
-      xsetenv ("USER", pw->pw_name);
-      xsetenv ("LOGNAME", pw->pw_name);
+	xsetenv ("TERM", term, 1);
+      xsetenv ("HOME", pw->pw_dir, 1);
+      xsetenv ("SHELL", shell, 1);
+      xsetenv ("USER", pw->pw_name, 1);
+      xsetenv ("LOGNAME", pw->pw_name, 1);
       set_path(pw);
     }
   else
@@ -519,8 +505,8 @@ modify_environment (const struct passwd *pw, const char *shell)
 	 USER and LOGNAME.  */
       if (change_environment)
         {
-          xsetenv ("HOME", pw->pw_dir);
-          xsetenv ("SHELL", shell);
+          xsetenv ("HOME", pw->pw_dir, 1);
+          xsetenv ("SHELL", shell, 1);
 	  if (getlogindefs_bool ("ALWAYS_SET_PATH", 0))
 	    set_path(pw);
 	  else
@@ -535,14 +521,14 @@ modify_environment (const struct passwd *pw, const char *shell)
 
 	      if (new)
 		{
-		  xsetenv ("PATH", new);
+		  xsetenv ("PATH", new, 1);
 		  free (new);
 		}
 	    }
           if (pw->pw_uid)
             {
-              xsetenv ("USER", pw->pw_name);
-              xsetenv ("LOGNAME", pw->pw_name);
+              xsetenv ("USER", pw->pw_name, 1);
+              xsetenv ("LOGNAME", pw->pw_name, 1);
             }
         }
     }
-- 
1.7.7

Re: [PATCH 4/7] introduce xsetenv globally

[Karel Zak]

On Fri, Jun 01, 2012 at 02:51:18PM +0200, Ludwig Nussel wrote:
> +++ b/include/env.h
> @@ -4,5 +4,12 @@
>  extern void sanitize_env (void);
>  extern char *safe_getenv(const char *arg);
>  
> +static inline void
> +xsetenv (char const *name, char const *val, int overwrite)
> +{
> +  if (setenv (name, val, overwrite) != 0)
> +    err (EXIT_FAILURE,  _("failed to set the %s environment variable"), name);
> +}
> +

 It would be nice to use it also in login.c :-)

    Karel
-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

[PATCH 5/7] replace PAM_BAIL_P macro with better solution

[Ludwig Nussel]

Signed-off-by: Ludwig Nussel <ludwig.nussel@suse.de>
---
 login-utils/su.c |   56 +++++++++++++++++++++++++++---------------------------
 1 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/login-utils/su.c b/login-utils/su.c
index cdaf31e..cabf116 100644
--- a/login-utils/su.c
+++ b/login-utils/su.c
@@ -68,6 +68,8 @@ enum
 #define PAM_SERVICE_NAME "su"
 #define PAM_SERVICE_NAME_L "su-l"
 
+#define is_pam_failure(_rc)	((_rc) != PAM_SUCCESS)
+
 #include "logindefs.h"
 
 /* The shell to run if none is given in the user's passwd entry.  */
@@ -148,13 +150,6 @@ static struct pam_conv conv =
   NULL
 };
 
-# define PAM_BAIL_P(a) \
-  if (retval) \
-    { \
-      pam_end (pamh, retval); \
-      a; \
-    }
-
 static void
 cleanup_pam (int retcode)
 {
@@ -199,7 +194,7 @@ create_watching_parent (void)
   int retval;
 
   retval = pam_open_session (pamh, 0);
-  if (retval != PAM_SUCCESS)
+  if (is_pam_failure(retval))
     {
       cleanup_pam (retval);
       error (EXIT_FAILURE, 0, _("cannot not open session: %s"),
@@ -305,8 +300,8 @@ create_watching_parent (void)
   exit (status);
 }
 
-static bool
-correct_password (const struct passwd *pw)
+static void
+authenticate (const struct passwd *pw)
 {
   const struct passwd *lpw;
   const char *cp;
@@ -314,7 +309,8 @@ correct_password (const struct passwd *pw)
 
   retval = pam_start (simulate_login ? PAM_SERVICE_NAME_L : PAM_SERVICE_NAME,
 		      pw->pw_name, &conv, &pamh);
-  PAM_BAIL_P (return false);
+  if (is_pam_failure(retval))
+    goto done;
 
   if (isatty (0) && (cp = ttyname (0)) != NULL)
     {
@@ -325,7 +321,8 @@ correct_password (const struct passwd *pw)
       else
 	tty = cp;
       retval = pam_set_item (pamh, PAM_TTY, tty);
-      PAM_BAIL_P (return false);
+      if (is_pam_failure(retval))
+	goto done;
     }
 # if 0 /* Manpage discourages use of getlogin.  */
   cp = getlogin ();
@@ -335,20 +332,32 @@ correct_password (const struct passwd *pw)
   if (lpw && lpw->pw_name)
     {
       retval = pam_set_item (pamh, PAM_RUSER, (const void *) lpw->pw_name);
-      PAM_BAIL_P (return false);
+      if (is_pam_failure(retval))
+	goto done;
     }
+
   retval = pam_authenticate (pamh, 0);
-  PAM_BAIL_P (return false);
+  if (is_pam_failure(retval))
+    goto done;
+
   retval = pam_acct_mgmt (pamh, 0);
   if (retval == PAM_NEW_AUTHTOK_REQD)
     {
       /* Password has expired.  Offer option to change it.  */
       retval = pam_chauthtok (pamh, PAM_CHANGE_EXPIRED_AUTHTOK);
-      PAM_BAIL_P (return false);
     }
-  PAM_BAIL_P (return false);
-  /* Must be authenticated if this point was reached.  */
-  return true;
+
+done:
+
+  log_su (pw, !is_pam_failure(retval));
+
+  if (is_pam_failure(retval))
+    {
+      const char *msg = pam_strerror(pamh, retval);
+      pam_end(pamh, retval);
+      sleep (getlogindefs_num ("FAIL_DELAY", 1));
+      error (EXIT_FAILURE, 0, "%s", msg?msg:_("incorrect password"));
+    }
 }
 
 /* Add or clear /sbin and /usr/sbin for the su command
@@ -760,16 +769,7 @@ main (int argc, char **argv)
 			  : DEFAULT_SHELL);
   endpwent ();
 
-  if (!correct_password (pw))
-    {
-      log_su (pw, false);
-      sleep (getlogindefs_num ("FAIL_DELAY", 1));
-      error (EXIT_FAILURE, 0, _("incorrect password"));
-    }
-  else
-    {
-      log_su (pw, true);
-    }
+  authenticate (pw);
 
   if (request_same_session || !command || !pw->pw_uid)
     same_session = 1;
-- 
1.7.7

[PATCH 6/7] remove unused code

[Ludwig Nussel]

Signed-off-by: Ludwig Nussel <ludwig.nussel@suse.de>
---
 login-utils/su.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/login-utils/su.c b/login-utils/su.c
index cabf116..9585d07 100644
--- a/login-utils/su.c
+++ b/login-utils/su.c
@@ -324,10 +324,7 @@ authenticate (const struct passwd *pw)
       if (is_pam_failure(retval))
 	goto done;
     }
-# if 0 /* Manpage discourages use of getlogin.  */
-  cp = getlogin ();
-  if (!(cp && *cp && (lpw = getpwnam (cp)) != NULL && lpw->pw_uid == getuid ()))
-# endif
+
   lpw = getpwuid (getuid ());
   if (lpw && lpw->pw_name)
     {
-- 
1.7.7

[PATCH 7/7] use BSD err function instead of gnu's error()

[Ludwig Nussel]

Signed-off-by: Ludwig Nussel <ludwig.nussel@suse.de>
---
 login-utils/su.c |   38 +++++++++++++++++++-------------------
 1 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/login-utils/su.c b/login-utils/su.c
index 9585d07..944d655 100644
--- a/login-utils/su.c
+++ b/login-utils/su.c
@@ -55,7 +55,7 @@ enum
 #include <sys/wait.h>
 #include <syslog.h>
 
-#include "error.h"
+#include "err.h"
 
 #include <stdbool.h>
 #include "c.h"
@@ -180,7 +180,7 @@ export_pamenv (void)
   while (env && *env)
     {
       if (putenv (*env) != 0)
-	error (EXIT_FAILURE, 0, _("out of memory"));
+	err (EXIT_FAILURE, NULL);
       env++;
     }
 }
@@ -197,7 +197,7 @@ create_watching_parent (void)
   if (is_pam_failure(retval))
     {
       cleanup_pam (retval);
-      error (EXIT_FAILURE, 0, _("cannot not open session: %s"),
+      errx (EXIT_FAILURE, _("cannot not open session: %s"),
 	     pam_strerror (pamh, retval));
     }
   else
@@ -207,7 +207,7 @@ create_watching_parent (void)
   if (child == (pid_t) -1)
     {
       cleanup_pam (PAM_ABORT);
-      error (EXIT_FAILURE, errno, _("cannot create child process"));
+      err (EXIT_FAILURE, _("cannot create child process"));
     }
 
   /* the child proceeds to run the shell */
@@ -219,12 +219,12 @@ create_watching_parent (void)
   /* su without pam support does not have a helper that keeps
      sitting on any directory so let's go to /.  */
   if (chdir ("/") != 0)
-    error (0, errno, _("warning: cannot change directory to %s"), "/");
+    warn (_("cannot change directory to %s"), "/");
 
   sigfillset (&ourset);
   if (sigprocmask (SIG_BLOCK, &ourset, NULL))
     {
-      error (0, errno, _("cannot block signals"));
+      warn (_("cannot block signals"));
       caught_signal = true;
     }
   if (!caught_signal)
@@ -238,7 +238,7 @@ create_watching_parent (void)
       {
         if (sigaddset(&ourset, SIGINT) || sigaddset(&ourset, SIGQUIT))
           {
-            error (0, errno, _("cannot set signal handler"));
+            warn (_("cannot set signal handler"));
             caught_signal = true;
           }
       }
@@ -246,13 +246,13 @@ create_watching_parent (void)
                     || sigaddset(&ourset, SIGALRM)
                     || sigaction(SIGTERM, &action, NULL)
                     || sigprocmask(SIG_UNBLOCK, &ourset, NULL))) {
-	  error (0, errno, _("cannot set signal handler"));
+	  warn (_("cannot set signal handler"));
 	  caught_signal = true;
 	}
     if (!caught_signal && !same_session && (sigaction(SIGINT, &action, NULL)
                                      || sigaction(SIGQUIT, &action, NULL)))
       {
-        error (0, errno, _("cannot set signal handler"));
+        warn (_("cannot set signal handler"));
         caught_signal = true;
       }
     }
@@ -353,7 +353,7 @@ done:
       const char *msg = pam_strerror(pamh, retval);
       pam_end(pamh, retval);
       sleep (getlogindefs_num ("FAIL_DELAY", 1));
-      error (EXIT_FAILURE, 0, "%s", msg?msg:_("incorrect password"));
+      errx (EXIT_FAILURE, "%s", msg?msg:_("incorrect password"));
     }
 }
 
@@ -552,13 +552,13 @@ init_groups (const struct passwd *pw)
   if (initgroups (pw->pw_name, pw->pw_gid) == -1)
     {
       cleanup_pam (PAM_ABORT);
-      error (EXIT_FAILURE, errno, _("cannot set groups"));
+      err (EXIT_FAILURE, _("cannot set groups"));
     }
   endgrent ();
 
   retval = pam_setcred (pamh, PAM_ESTABLISH_CRED);
-  if (retval != PAM_SUCCESS)
-    error (EXIT_FAILURE, 0, "%s", pam_strerror (pamh, retval));
+  if (is_pam_failure(retval))
+    errx (EXIT_FAILURE, "%s", pam_strerror (pamh, retval));
   else
     _pam_cred_established = 1;
 }
@@ -567,9 +567,9 @@ static void
 change_identity (const struct passwd *pw)
 {
   if (setgid (pw->pw_gid))
-    error (EXIT_FAILURE, errno, _("cannot set group id"));
+    err (EXIT_FAILURE,  _("cannot set group id"));
   if (setuid (pw->pw_uid))
-    error (EXIT_FAILURE, errno, _("cannot set user id"));
+    err (EXIT_FAILURE,  _("cannot set user id"));
 }
 
 /* Run SHELL, or DEFAULT_SHELL if SHELL is empty.
@@ -611,7 +611,7 @@ run_shell (char const *shell, char const *command, char **additional_args,
 
   {
     int exit_status = (errno == ENOENT ? EXIT_ENOENT : EXIT_CANNOT_INVOKE);
-    error (0, errno, "%s", shell);
+    warn ("%s", shell);
     exit (exit_status);
   }
 }
@@ -748,7 +748,7 @@ main (int argc, char **argv)
   pw = getpwnam (new_user);
   if (! (pw && pw->pw_name && pw->pw_name[0] && pw->pw_dir && pw->pw_dir[0]
 	 && pw->pw_passwd))
-    error (EXIT_FAILURE, 0, _("user %s does not exist"), new_user);
+    errx (EXIT_FAILURE, _("user %s does not exist"), new_user);
 
   /* Make a copy of the password information and point pw at the local
      copy instead.  Otherwise, some systems (e.g. Linux) would clobber
@@ -779,7 +779,7 @@ main (int argc, char **argv)
 	 probably a uucp account or has restricted access.  Don't
 	 compromise the account by allowing access with a standard
 	 shell.  */
-      error (0, 0, _("using restricted shell %s"), pw->pw_shell);
+      warnx (_("using restricted shell %s"), pw->pw_shell);
       shell = NULL;
     }
   shell = xstrdup (shell ? shell : pw->pw_shell);
@@ -799,7 +799,7 @@ main (int argc, char **argv)
   modify_environment (pw, shell);
 
   if (simulate_login && chdir (pw->pw_dir) != 0)
-    error (0, errno, _("warning: cannot change directory to %s"), pw->pw_dir);
+    warn (_("warning: cannot change directory to %s"), pw->pw_dir);
 
   run_shell (shell, command, argv + optind, max (0, argc - optind));
 }
-- 
1.7.7

Re: [PATCH 1/7] don't use custom MAX macro

[Karel Zak]

On Fri, Jun 01, 2012 at 02:51:15PM +0200, Ludwig Nussel wrote:
>  login-utils/su.c |    6 +-----
>  1 files changed, 1 insertions(+), 5 deletions(-)

 All 7 patches applied, thanks!

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com