Skip to content

Commit

Permalink
getconf: fix fallback to external (re: c3fec93, 519bb08, 66e1d44)
Browse files Browse the repository at this point in the history
The previous commit exposed a bug in libcmd's getconf(1) built-in
command. First, note that the 'builtin' built-in command can bind
a libcmd built-in command to any path: for example, 'builtin
/foo/bar/getconf' works. As of 519bb08, path-bound built-ins can
be invoked using their canonical path, not just the command name,
so invoking a command by its path no longer guarantees that an
external command is run. Consequently, if the built-in is bound to
the same path that conf.sh detected for the external command, there
is an infinite recursion ending in a C stack overflow: b_getconf()
in src/lib/libcmd/getconf.c keeps invoking itself via sh_run().

src/lib/libcmd/getconf.c:
- b_getconf(): Use 'command -x' to invoke the fallback getconf
  command to guarantee that no path-bound built-in is run.

src/lib/libast/port/astconf.c:
- Fix another bug found in lookup() while I was tracing the above
  one with ASan: an off-by-one error caused a out-of-bounds access
  just beyond the conf array when looking up certain names. The
  last element is lo + conf_elements - 1, not lo + conf_elements.
- Also fix the confusing way in which these variables are declared.
  • Loading branch information
McDutchie committed Mar 22, 2024
1 parent c3fec93 commit 8bfeb63
Show file tree
Hide file tree
Showing 5 changed files with 31 additions and 6 deletions.
5 changes: 5 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@ This documents significant changes in the 1.0 branch of ksh 93u+m.
For full details, see the git log at: https://github.com/ksh93/ksh/tree/1.0
Uppercase BUG_* IDs are shell bug IDs as used by the Modernish shell library.

2024-03-22:

- Fixed a crash in the getconf built-in that could occur after the 'builtin'
command was used to bind it to the same path as the external getconf command.

2024-03-20:

- Android with Termux is now a supported environment for ksh 93u+m. Testing
Expand Down
2 changes: 1 addition & 1 deletion src/cmd/ksh93/include/version.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

#define SH_RELEASE_FORK "93u+m" /* only change if you develop a new ksh93 fork */
#define SH_RELEASE_SVER "1.0.9-beta" /* semantic version number: https://semver.org */
#define SH_RELEASE_DATE "2024-03-20" /* must be in this format for $((.sh.version)) */
#define SH_RELEASE_DATE "2024-03-22" /* must be in this format for $((.sh.version)) */
#define SH_RELEASE_CPYR "(c) 2020-2024 Contributors to ksh " SH_RELEASE_FORK

/* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */
Expand Down
15 changes: 15 additions & 0 deletions src/cmd/ksh93/tests/path.sh
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,7 @@ if builtin getconf 2> /dev/null; then
fi

PATH=$path
builtin -d /bin/getconf

scr=$tmp/script
exp=126
Expand Down Expand Up @@ -1022,5 +1023,19 @@ got=${ whence -t whence_t_test 2>&1; }
got=${ type -t whence_t_test 2>&1; }
[[ $got == "$exp" ]] || err_exit "incorrect 'type -t' output for undefined function (expected '$exp', got '$got')"

# ======
(
builtin getconf 2>/dev/null || exit 1
p=$(getconf GETCONF)
[[ $p == /*/getconf ]] || exit 2
builtin -d getconf
builtin "$p"
PATH=${p%/getconf}
getconf some_nonexistent_config_variable # be sure to trigger fallback to external command
exit 0
) >/dev/null 2>&1
(((e = $?) > 1)) && err_exit 'getconf builtin fails when on same path as external getconf' \
"(got status $e$( ((e>128)) && print -n /SIG && kill -l "$e"))"

# ======
exit $((Errors<125?Errors:125))
6 changes: 3 additions & 3 deletions src/lib/libast/port/astconf.c
Original file line number Diff line number Diff line change
Expand Up @@ -790,9 +790,9 @@ feature(Feature_t* fp, const char* name, const char* path, const char* value, un
static int
lookup(Lookup_t* look, const char* name, unsigned int flags)
{
Conf_t* mid = (Conf_t*)conf;
Conf_t* lo = mid;
Conf_t* hi = mid + conf_elements;
Conf_t* lo = (Conf_t*)conf;
Conf_t* mid = lo;
Conf_t* hi = lo + conf_elements - 1;
int v = 0;
int c;
char* e;
Expand Down
9 changes: 7 additions & 2 deletions src/lib/libcmd/getconf.c
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ b_getconf(int argc, char** argv, Shbltin_t* context)
int flags;
int n;
char** oargv;
char** new_argv;
static const char empty[] = "-";

cmdinit(argc, argv, context, ERROR_CATALOG, 0);
Expand Down Expand Up @@ -274,8 +275,12 @@ b_getconf(int argc, char** argv, Shbltin_t* context)
/*
* Run the external getconf command
*/
oargv[0] = native;
if ((n = sh_run(context, argc, oargv)) >= EXIT_NOEXEC)
new_argv = stkalloc(stkstd, (argc + 3) * sizeof(char*));
new_argv[0] = "command";
new_argv[1] = "-x";
new_argv[2] = native;
memcpy(new_argv + 3, oargv + 1, argc * sizeof(char*));
if ((n = sh_run(context, argc + 2, new_argv)) >= EXIT_NOEXEC)
error(ERROR_SYSTEM|2, "%s: exec error [%d]", native, n);
return n;
}

0 comments on commit 8bfeb63

Please sign in to comment.