[Pam-developers] [PATCH] Fix gratuitous use of strdup and x_strdup

Dmitry V. Levin ldv at altlinux.org
Sat Jan 25 00:46:42 UTC 2014


There is no need to copy strings passed as arguments to execve,
the only potentially noticeable effect of using strdup/x_strdup
would be a malformed argument list in case of memory allocation error.

Also, x_strdup, being a thin wrapper around strdup, is of no benefit
when its argument is known to be non-NULL, and should not be used in
such cases.

* modules/pam_cracklib/pam_cracklib.c (password_check): Use strdup
instead of x_strdup, the latter is of no benefit in this case.
* modules/pam_ftp/pam_ftp.c (lookup): Likewise.
* modules/pam_userdb/pam_userdb.c (user_lookup): Likewise.
* modules/pam_userdb/pam_userdb.h (x_strdup): Remove.
* modules/pam_mkhomedir/pam_mkhomedir.c (create_homedir): Do not use
x_strdup for strings passed as arguments to execve.
* modules/pam_unix/pam_unix_acct.c (_unix_run_verify_binary): Likewise.
* modules/pam_unix/pam_unix_passwd.c (_unix_run_update_binary): Likewise.
* modules/pam_unix/support.c (_unix_run_helper_binary): Likewise.
(_unix_verify_password): Use strdup instead of x_strdup, the latter
is of no benefit in this case.
* modules/pam_xauth/pam_xauth.c (run_coprocess): Do not use strdup for
strings passed as arguments to execv.
---
 modules/pam_cracklib/pam_cracklib.c   |  6 +++---
 modules/pam_ftp/pam_ftp.c             |  2 +-
 modules/pam_mkhomedir/pam_mkhomedir.c | 12 ++++++------
 modules/pam_unix/pam_unix_acct.c      | 10 +++++-----
 modules/pam_unix/pam_unix_passwd.c    | 16 ++++++++--------
 modules/pam_unix/support.c            | 16 ++++++++--------
 modules/pam_userdb/pam_userdb.c       |  2 +-
 modules/pam_userdb/pam_userdb.h       |  3 ---
 modules/pam_xauth/pam_xauth.c         | 12 +++++-------
 9 files changed, 37 insertions(+), 42 deletions(-)

diff --git a/modules/pam_cracklib/pam_cracklib.c b/modules/pam_cracklib/pam_cracklib.c
index 5691347..5eefd0b 100644
--- a/modules/pam_cracklib/pam_cracklib.c
+++ b/modules/pam_cracklib/pam_cracklib.c
@@ -619,16 +619,16 @@ static const char *password_check(pam_handle_t *pamh, struct cracklib_options *o
 	    return msg;
 	}
 
-	newmono = str_lower(x_strdup(new));
+	newmono = str_lower(strdup(new));
 	if (!newmono)
 		msg = _("memory allocation error");
 
-	usermono = str_lower(x_strdup(user));
+	usermono = str_lower(strdup(user));
 	if (!usermono)
 		msg = _("memory allocation error");
 
 	if (!msg && old) {
-		oldmono = str_lower(x_strdup(old));
+		oldmono = str_lower(strdup(old));
 		if (oldmono)
 			wrapped = malloc(strlen(oldmono) * 2 + 1);
 		if (wrapped) {
diff --git a/modules/pam_ftp/pam_ftp.c b/modules/pam_ftp/pam_ftp.c
index 896a1dd..221d8f8 100644
--- a/modules/pam_ftp/pam_ftp.c
+++ b/modules/pam_ftp/pam_ftp.c
@@ -81,7 +81,7 @@ static int lookup(const char *name, const char *list, const char **_user)
 	char *list_copy, *x;
 	char *sptr = NULL;
 
-	list_copy = x_strdup(list);
+	list_copy = strdup(list);
 	x = list_copy;
 	while (list_copy && (l = strtok_r(x, ",", &sptr))) {
 	    x = NULL;
diff --git a/modules/pam_mkhomedir/pam_mkhomedir.c b/modules/pam_mkhomedir/pam_mkhomedir.c
index 0b5fc75..a867a73 100644
--- a/modules/pam_mkhomedir/pam_mkhomedir.c
+++ b/modules/pam_mkhomedir/pam_mkhomedir.c
@@ -134,7 +134,7 @@ create_homedir (pam_handle_t *pamh, options_t *opt,
         int i;
         struct rlimit rlim;
 	static char *envp[] = { NULL };
-	char *args[] = { NULL, NULL, NULL, NULL, NULL };
+	const char *args[] = { NULL, NULL, NULL, NULL, NULL };
 
 	if (getrlimit(RLIMIT_NOFILE, &rlim)==0) {
           if (rlim.rlim_max >= MAX_FD_NO)
@@ -145,12 +145,12 @@ create_homedir (pam_handle_t *pamh, options_t *opt,
 	}
 
 	/* exec the mkhomedir helper */
-	args[0] = x_strdup(MKHOMEDIR_HELPER);
-	args[1] = (char *) user;
-	args[2] = x_strdup(opt->umask);
-	args[3] = x_strdup(opt->skeldir);
+	args[0] = MKHOMEDIR_HELPER;
+	args[1] = user;
+	args[2] = opt->umask;
+	args[3] = opt->skeldir;
 
-	execve(MKHOMEDIR_HELPER, args, envp);
+	execve(MKHOMEDIR_HELPER, (char *const *) args, envp);
 
 	/* should not get here: exit with error */
 	D(("helper binary is not available"));
diff --git a/modules/pam_unix/pam_unix_acct.c b/modules/pam_unix/pam_unix_acct.c
index 8ec4449..dc505e7 100644
--- a/modules/pam_unix/pam_unix_acct.c
+++ b/modules/pam_unix/pam_unix_acct.c
@@ -101,7 +101,7 @@ int _unix_run_verify_binary(pam_handle_t *pamh, unsigned int ctrl,
     int i=0;
     struct rlimit rlim;
     static char *envp[] = { NULL };
-    char *args[] = { NULL, NULL, NULL, NULL };
+    const char *args[] = { NULL, NULL, NULL, NULL };
 
     /* reopen stdout as pipe */
     dup2(fds[1], STDOUT_FILENO);
@@ -130,11 +130,11 @@ int _unix_run_verify_binary(pam_handle_t *pamh, unsigned int ctrl,
     }
 
     /* exec binary helper */
-    args[0] = x_strdup(CHKPWD_HELPER);
-    args[1] = x_strdup(user);
-    args[2] = x_strdup("chkexpiry");
+    args[0] = CHKPWD_HELPER;
+    args[1] = user;
+    args[2] = "chkexpiry";
 
-    execve(CHKPWD_HELPER, args, envp);
+    execve(CHKPWD_HELPER, (char *const *) args, envp);
 
     pam_syslog(pamh, LOG_ERR, "helper binary execve failed: %m");
     /* should not get here: exit with error */
diff --git a/modules/pam_unix/pam_unix_passwd.c b/modules/pam_unix/pam_unix_passwd.c
index 0cfc0f4..5f3a3db 100644
--- a/modules/pam_unix/pam_unix_passwd.c
+++ b/modules/pam_unix/pam_unix_passwd.c
@@ -204,7 +204,7 @@ static int _unix_run_update_binary(pam_handle_t *pamh, unsigned int ctrl, const
         int i=0;
         struct rlimit rlim;
 	static char *envp[] = { NULL };
-	char *args[] = { NULL, NULL, NULL, NULL, NULL, NULL };
+	const char *args[] = { NULL, NULL, NULL, NULL, NULL, NULL };
         char buffer[16];
 
 	/* XXX - should really tidy up PAM here too */
@@ -222,18 +222,18 @@ static int _unix_run_update_binary(pam_handle_t *pamh, unsigned int ctrl, const
 	}
 
 	/* exec binary helper */
-	args[0] = x_strdup(UPDATE_HELPER);
-	args[1] = x_strdup(user);
-	args[2] = x_strdup("update");
+	args[0] = UPDATE_HELPER;
+	args[1] = user;
+	args[2] = "update";
 	if (on(UNIX_SHADOW, ctrl))
-		args[3] = x_strdup("1");
+		args[3] = "1";
 	else
-		args[3] = x_strdup("0");
+		args[3] = "0";
 
         snprintf(buffer, sizeof(buffer), "%d", remember);
-        args[4] = x_strdup(buffer);
+        args[4] = buffer;
 
-	execve(UPDATE_HELPER, args, envp);
+	execve(UPDATE_HELPER, (char *const *) args, envp);
 
 	/* should not get here: exit with error */
 	D(("helper binary is not available"));
diff --git a/modules/pam_unix/support.c b/modules/pam_unix/support.c
index 19d72e6..3a849c8 100644
--- a/modules/pam_unix/support.c
+++ b/modules/pam_unix/support.c
@@ -567,7 +567,7 @@ static int _unix_run_helper_binary(pam_handle_t *pamh, const char *passwd,
         int i=0;
         struct rlimit rlim;
 	static char *envp[] = { NULL };
-	char *args[] = { NULL, NULL, NULL, NULL };
+	const char *args[] = { NULL, NULL, NULL, NULL };
 
 	/* XXX - should really tidy up PAM here too */
 
@@ -593,15 +593,15 @@ static int _unix_run_helper_binary(pam_handle_t *pamh, const char *passwd,
 	}
 
 	/* exec binary helper */
-	args[0] = strdup(CHKPWD_HELPER);
-	args[1] = x_strdup(user);
+	args[0] = CHKPWD_HELPER;
+	args[1] = user;
 	if (off(UNIX__NONULL, ctrl)) {	/* this means we've succeeded */
-	  args[2]=strdup("nullok");
+	  args[2]="nullok";
 	} else {
-	  args[2]=strdup("nonull");
+	  args[2]="nonull";
 	}
 
-	execve(CHKPWD_HELPER, args, envp);
+	execve(CHKPWD_HELPER, (char *const *) args, envp);
 
 	/* should not get here: exit with error */
 	D(("helper binary is not available"));
@@ -788,10 +788,10 @@ int _unix_verify_password(pam_handle_t * pamh, const char *name
 				login_name = "";
 			    }
 
-			        new->user = x_strdup(name ? name : "");
+			        new->user = strdup(name ? name : "");
 				new->uid = getuid();
 				new->euid = geteuid();
-				new->name = x_strdup(login_name);
+				new->name = strdup(login_name);
 
 				/* any previous failures for this user ? */
 				if (pam_get_data(pamh, data_name, &void_old)
diff --git a/modules/pam_userdb/pam_userdb.c b/modules/pam_userdb/pam_userdb.c
index ff040e6..ba36ebf 100644
--- a/modules/pam_userdb/pam_userdb.c
+++ b/modules/pam_userdb/pam_userdb.c
@@ -184,7 +184,7 @@ user_lookup (pam_handle_t *pamh, const char *database, const char *cryptmode,
 	else
 	    key.dsize = strlen(key.dptr);
     } else {
-        key.dptr = x_strdup(user);
+        key.dptr = strdup(user);
         key.dsize = strlen(user);
     }
 
diff --git a/modules/pam_userdb/pam_userdb.h b/modules/pam_userdb/pam_userdb.h
index 3cd8fee..86e9b47 100644
--- a/modules/pam_userdb/pam_userdb.h
+++ b/modules/pam_userdb/pam_userdb.h
@@ -15,9 +15,6 @@
 #define PAM_USE_FPASS_ARG	0x0040
 #define PAM_TRY_FPASS_ARG	0x0080
 
-/* Useful macros */
-#define  x_strdup(s)  ( (s) ? strdup(s):NULL )
-
 /* The name of the module we are compiling */
 #ifndef MODULE_NAME
 #define MODULE_NAME	"pam_userdb"
diff --git a/modules/pam_xauth/pam_xauth.c b/modules/pam_xauth/pam_xauth.c
index 32adcf6..c7ce55a 100644
--- a/modules/pam_xauth/pam_xauth.c
+++ b/modules/pam_xauth/pam_xauth.c
@@ -127,8 +127,7 @@ run_coprocess(pam_handle_t *pamh, const char *input, char **output,
 	if (child == 0) {
 		/* We're the child. */
 		size_t j;
-		char *args[10];
-		const char *tmp;
+		const char *args[10];
 		int maxopened;
 		/* Drop privileges. */
 		if (setgid(gid) == -1)
@@ -166,16 +165,15 @@ run_coprocess(pam_handle_t *pamh, const char *input, char **output,
 		}
 		/* Convert the varargs list into a regular array of strings. */
 		va_start(ap, command);
-		args[0] = strdup(command);
+		args[0] = command;
 		for (j = 1; j < ((sizeof(args) / sizeof(args[0])) - 1); j++) {
-			tmp = va_arg(ap, const char*);
-			if (tmp == NULL) {
+			args[j] = va_arg(ap, const char*);
+			if (args[j] == NULL) {
 				break;
 			}
-			args[j] = strdup(tmp);
 		}
 		/* Run the command. */
-		execv(command, args);
+		execv(command, (char *const *) args);
 		/* Never reached. */
 		_exit(1);
 	}
-- 
ldv


More information about the Pam-developers mailing list