[git commit] which: fix TODO with NOFORK+malloc_failure misbehaving

[Denys Vlasenko]

commit: https://git.busybox.net/busybox/commit/?id=cca7c611f26d98415c0f986e5a5e731ab5e379ff
branch: https://git.busybox.net/busybox/commit/?id=refs/heads/master

function                                             old     new   delta
find_executable                                       86     104     +18
which_main                                           202     194      -8
executable_exists                                     66      51     -15
------------------------------------------------------------------------------
(add/remove: 0/0 grow/shrink: 1/2 up/down: 18/-23)             Total: -5 bytes

Signed-off-by: Denys Vlasenko <vda.linux at googlemail.com>
---
 debianutils/which.c | 16 ++++++++--------
 include/libbb.h     | 12 +++++++++---
 libbb/executable.c  | 18 ++++++++++--------
 libbb/messages.c    |  8 +-------
 4 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/debianutils/which.c b/debianutils/which.c
index 3bd54ac..02f77a2 100644
--- a/debianutils/which.c
+++ b/debianutils/which.c
@@ -30,12 +30,15 @@
 int which_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
 int which_main(int argc UNUSED_PARAM, char **argv)
 {
-	const char *env_path;
+	char *env_path;
 	int status = 0;
+	/* This sizeof(): bb_default_root_path is shorter than BB_PATH_ROOT_PATH */
+	char buf[sizeof(BB_PATH_ROOT_PATH)];
 
 	env_path = getenv("PATH");
 	if (!env_path)
-		env_path = bb_default_root_path;
+		/* env_path must be writable, and must not alloc, so... */
+		env_path = strcpy(buf, bb_default_root_path);
 
 	getopt32(argv, "^" "a" "\0" "-1"/*at least one arg*/);
 	argv += optind;
@@ -51,20 +54,17 @@ int which_main(int argc UNUSED_PARAM, char **argv)
 			}
 		} else {
 			char *path;
-			char *tmp;
 			char *p;
 
-			path = tmp = xstrdup(env_path);
-//NOFORK FIXME: nested xmallocs (one is inside find_executable())
-//can leak memory on failure
-			while ((p = find_executable(*argv, &tmp)) != NULL) {
+			path = env_path;
+			/* NOFORK NB: xmalloc inside find_executable(), must have no allocs above! */
+			while ((p = find_executable(*argv, &path)) != NULL) {
 				missing = 0;
 				puts(p);
 				free(p);
 				if (!option_mask32) /* -a not set */
 					break;
 			}
-			free(path);
 		}
 		status |= missing;
 	} while (*++argv);
diff --git a/include/libbb.h b/include/libbb.h
index daccf15..5f25b5d 100644
--- a/include/libbb.h
+++ b/include/libbb.h
@@ -2005,10 +2005,16 @@ extern const char bb_path_wtmp_file[] ALIGN1;
 
 #define bb_dev_null "/dev/null"
 extern const char bb_busybox_exec_path[] ALIGN1;
-/* util-linux manpage says /sbin:/bin:/usr/sbin:/usr/bin,
- * but I want to save a few bytes here */
-extern const char bb_PATH_root_path[] ALIGN1; /* "PATH=/sbin:/usr/sbin:/bin:/usr/bin" */
+/* allow default system PATH to be extended via CFLAGS */
+#ifndef BB_ADDITIONAL_PATH
+#define BB_ADDITIONAL_PATH ""
+#endif
+#define BB_PATH_ROOT_PATH "PATH=/sbin:/usr/sbin:/bin:/usr/bin" BB_ADDITIONAL_PATH
+extern const char bb_PATH_root_path[] ALIGN1; /* BB_PATH_ROOT_PATH */
 #define bb_default_root_path (bb_PATH_root_path + sizeof("PATH"))
+/* util-linux manpage says /sbin:/bin:/usr/sbin:/usr/bin,
+ * but I want to save a few bytes here:
+ */
 #define bb_default_path      (bb_PATH_root_path + sizeof("PATH=/sbin:/usr/sbin"))
 
 extern const int const_int_0;
diff --git a/libbb/executable.c b/libbb/executable.c
index 325dd01..29d2a2c 100644
--- a/libbb/executable.c
+++ b/libbb/executable.c
@@ -25,7 +25,8 @@ int FAST_FUNC file_is_executable(const char *name)
  *  you may call find_executable again with this PATHp to continue
  *  (if it's not NULL).
  * return NULL otherwise; (PATHp is undefined)
- * in all cases (*PATHp) contents will be trashed (s/:/NUL/).
+ * in all cases (*PATHp) contents are temporarily modified
+ * but are restored on return (s/:/NUL/ and back).
  */
 char* FAST_FUNC find_executable(const char *filename, char **PATHp)
 {
@@ -41,14 +42,17 @@ char* FAST_FUNC find_executable(const char *filename, char **PATHp)
 
 	p = *PATHp;
 	while (p) {
+		int ex;
+
 		n = strchr(p, ':');
-		if (n)
-			*n++ = '\0';
+		if (n) *n = '\0';
 		p = concat_path_file(
 			p[0] ? p : ".", /* handle "::" case */
 			filename
 		);
-		if (file_is_executable(p)) {
+		ex = file_is_executable(p);
+		if (n) *n++ = ':';
+		if (ex) {
 			*PATHp = n;
 			return p;
 		}
@@ -64,10 +68,8 @@ char* FAST_FUNC find_executable(const char *filename, char **PATHp)
  */
 int FAST_FUNC executable_exists(const char *filename)
 {
-	char *path = xstrdup(getenv("PATH"));
-	char *tmp = path;
-	char *ret = find_executable(filename, &tmp);
-	free(path);
+	char *path = getenv("PATH");
+	char *ret = find_executable(filename, &path);
 	free(ret);
 	return ret != NULL;
 }
diff --git a/libbb/messages.c b/libbb/messages.c
index 0a6cf3b..6914d57 100644
--- a/libbb/messages.c
+++ b/libbb/messages.c
@@ -6,11 +6,6 @@
  */
 #include "libbb.h"
 
-/* allow default system PATH to be extended via CFLAGS */
-#ifndef BB_ADDITIONAL_PATH
-#define BB_ADDITIONAL_PATH ""
-#endif
-
 /* allow version to be extended, via CFLAGS */
 #ifndef BB_EXTRA_VERSION
 #define BB_EXTRA_VERSION " ("AUTOCONF_TIMESTAMP")"
@@ -36,8 +31,7 @@ const char bb_busybox_exec_path[] ALIGN1 = CONFIG_BUSYBOX_EXEC_PATH;
 const char bb_default_login_shell[] ALIGN1 = LIBBB_DEFAULT_LOGIN_SHELL;
 /* util-linux manpage says /sbin:/bin:/usr/sbin:/usr/bin,
  * but I want to save a few bytes here. Check libbb.h before changing! */
-const char bb_PATH_root_path[] ALIGN1 =
-	"PATH=/sbin:/usr/sbin:/bin:/usr/bin" BB_ADDITIONAL_PATH;
+const char bb_PATH_root_path[] ALIGN1 = BB_PATH_ROOT_PATH;
 
 
 //const int const_int_1 = 1;