From cca7c611f26d98415c0f986e5a5e731ab5e379ff Mon Sep 17 00:00:00 2001 From: Denys Vlasenko Date: Fri, 12 Jan 2018 13:21:33 +0100 Subject: [PATCH] which: fix TODO with NOFORK+malloc_failure misbehaving 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 --- 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 3bd54ac42..02f77a216 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 daccf154a..5f25b5dde 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 325dd0107..29d2a2c85 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 0a6cf3bf8..6914d5701 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;