[git commit] hush: fix "getopts" builtin to not be upset by other builtins calling getopt()
Denys Vlasenko
vda.linux at googlemail.com
Tue Aug 29 11:38:30 UTC 2017
commit: https://git.busybox.net/busybox/commit/?id=238ff98bb85adfb5563d10b37ea4c33fef3af2f2
branch: https://git.busybox.net/busybox/commit/?id=refs/heads/master
function old new delta
builtin_getopts 363 403 +40
unset_local_var_len 185 215 +30
set_local_var 440 466 +26
reset_traps_to_defaults 151 157 +6
pseudo_exec_argv 320 326 +6
install_special_sighandlers 52 58 +6
pick_sighandler 62 65 +3
execvp_or_die 85 88 +3
------------------------------------------------------------------------------
(add/remove: 0/0 grow/shrink: 8/0 up/down: 120/0) Total: 120 bytes
Signed-off-by: Denys Vlasenko <vda.linux at googlemail.com>
---
shell/ash_test/ash-getopts/getopt_nested.right | 14 +++++
shell/ash_test/ash-getopts/getopt_nested.tests | 21 +++++++
shell/hush.c | 78 +++++++++++++++++++-----
shell/hush_test/hush-getopts/getopt_nested.right | 14 +++++
shell/hush_test/hush-getopts/getopt_nested.tests | 21 +++++++
5 files changed, 133 insertions(+), 15 deletions(-)
diff --git a/shell/ash_test/ash-getopts/getopt_nested.right b/shell/ash_test/ash-getopts/getopt_nested.right
new file mode 100644
index 0000000..b0c339d
--- /dev/null
+++ b/shell/ash_test/ash-getopts/getopt_nested.right
@@ -0,0 +1,14 @@
+var:a
+var:b
+var:c
+var:a
+var:b
+var:c
+Illegal option -d
+var:?
+Illegal option -e
+var:?
+Illegal option -f
+var:?
+var:a
+End: var:? OPTIND:6
diff --git a/shell/ash_test/ash-getopts/getopt_nested.tests b/shell/ash_test/ash-getopts/getopt_nested.tests
new file mode 100755
index 0000000..1b48b40
--- /dev/null
+++ b/shell/ash_test/ash-getopts/getopt_nested.tests
@@ -0,0 +1,21 @@
+# Test that there is no interference of getopt()
+# in getopts and unset.
+# It's unclear what "correct" OPTIND values should be
+# for "b" and "c" results from "-bc": 2? 3?
+# What we focus on here is that all options are reported
+# correct number of times and in correct sequence.
+
+(
+
+loop=99
+while getopts "abc" var -a -bc -abc -def -a; do
+ echo "var:$var" #OPTIND:$OPTIND
+ # this may use getopt():
+ unset -ff func
+ test $((--loop)) = 0 && break # BUG if this triggers
+done
+echo "End: var:$var OPTIND:$OPTIND"
+
+) 2>&1 \
+| sed -e 's/ unrecognized option: / invalid option -- /' \
+ -e 's/ illegal option -- / invalid option -- /' \
diff --git a/shell/hush.c b/shell/hush.c
index cdc3a86..ceb8cbb 100644
--- a/shell/hush.c
+++ b/shell/hush.c
@@ -907,6 +907,9 @@ struct globals {
unsigned depth_break_continue;
unsigned depth_of_loop;
#endif
+#if ENABLE_HUSH_GETOPTS
+ unsigned getopt_count;
+#endif
const char *ifs;
const char *cwd;
struct variable *top_var;
@@ -2214,6 +2217,10 @@ static int set_local_var(char *str, unsigned flags)
cur->flg_export = 1;
if (name_len == 4 && cur->varstr[0] == 'P' && cur->varstr[1] == 'S')
cmdedit_update_prompt();
+#if ENABLE_HUSH_GETOPTS
+ if (strncmp(cur->varstr, "OPTIND=", 7) == 0)
+ G.getopt_count = 0;
+#endif
if (cur->flg_export) {
if (flags & SETFLAG_UNEXPORT) {
cur->flg_export = 0;
@@ -2244,6 +2251,10 @@ static int unset_local_var_len(const char *name, int name_len)
if (!name)
return EXIT_SUCCESS;
+#if ENABLE_HUSH_GETOPTS
+ if (name_len == 6 && strncmp(name, "OPTIND", 6) == 0)
+ G.getopt_count = 0;
+#endif
var_pp = &G.top_var;
while ((cur = *var_pp) != NULL) {
if (strncmp(cur->varstr, name, name_len) == 0 && cur->varstr[name_len] == '=') {
@@ -9889,7 +9900,8 @@ Test that VAR is a valid variable name?
*/
char cbuf[2];
const char *cp, *optstring, *var;
- int c, exitcode;
+ int c, n, exitcode, my_opterr;
+ unsigned count;
optstring = *++argv;
if (!optstring || !(var = *++argv)) {
@@ -9897,17 +9909,18 @@ Test that VAR is a valid variable name?
return EXIT_FAILURE;
}
- c = 0;
+ if (argv[1])
+ argv[0] = G.global_argv[0]; /* for error messages in getopt() */
+ else
+ argv = G.global_argv;
+ cbuf[1] = '\0';
+
+ my_opterr = 0;
if (optstring[0] != ':') {
cp = get_local_var_value("OPTERR");
/* 0 if "OPTERR=0", 1 otherwise */
- c = (!cp || NOT_LONE_CHAR(cp, '0'));
+ my_opterr = (!cp || NOT_LONE_CHAR(cp, '0'));
}
- opterr = c;
- cp = get_local_var_value("OPTIND");
- optind = cp ? atoi(cp) : 0;
- optarg = NULL;
- cbuf[1] = '\0';
/* getopts stops on first non-option. Add "+" to force that */
/*if (optstring[0] != '+')*/ {
@@ -9916,11 +9929,47 @@ Test that VAR is a valid variable name?
optstring = s;
}
- if (argv[1])
- argv[0] = G.global_argv[0]; /* for error messages */
- else
- argv = G.global_argv;
- c = getopt(string_array_len(argv), argv, optstring);
+ /* Naively, now we should just
+ * cp = get_local_var_value("OPTIND");
+ * optind = cp ? atoi(cp) : 0;
+ * optarg = NULL;
+ * opterr = my_opterr;
+ * c = getopt(string_array_len(argv), argv, optstring);
+ * and be done? Not so fast...
+ * Unlike normal getopt() usage in C programs, here
+ * each successive call will (usually) have the same argv[] CONTENTS,
+ * but not the ADDRESSES. Worse yet, it's possible that between
+ * invocations of "getopts", there will be calls to shell builtins
+ * which use getopt() internally. Example:
+ * while getopts "abc" RES -a -bc -abc de; do
+ * unset -ff func
+ * done
+ * This would not work correctly: getopt() call inside "unset"
+ * modifies internal libc state which is tracking position in
+ * multi-option strings ("-abc"). At best, it can skip options
+ * or return the same option infinitely. With glibc implementation
+ * of getopt(), it would use outright invalid pointers and return
+ * garbage even _without_ "unset" mangling internal state.
+ *
+ * We resort to resetting getopt() state and calling it N times,
+ * until we get Nth result (or failure).
+ * (N == G.getopt_count is reset to 0 whenever OPTIND is [un]set).
+ */
+ optind = 0; /* reset getopt() state */
+ count = 0;
+ n = string_array_len(argv);
+ do {
+ optarg = NULL;
+ opterr = (count < G.getopt_count) ? 0 : my_opterr;
+ c = getopt(n, argv, optstring);
+ if (c < 0)
+ break;
+ count++;
+ } while (count <= G.getopt_count);
+
+ /* Set OPTIND. Prevent resetting of the magic counter! */
+ set_local_var_from_halves("OPTIND", utoa(optind));
+ G.getopt_count = count; /* "next time, give me N+1'th result" */
/* Set OPTARG */
/* Always set or unset, never left as-is, even on exit/error:
@@ -9949,10 +9998,9 @@ Test that VAR is a valid variable name?
c = '?';
}
- /* Set VAR and OPTIND */
+ /* Set VAR */
cbuf[0] = c;
set_local_var_from_halves(var, cbuf);
- set_local_var_from_halves("OPTIND", utoa(optind));
return exitcode;
}
diff --git a/shell/hush_test/hush-getopts/getopt_nested.right b/shell/hush_test/hush-getopts/getopt_nested.right
new file mode 100644
index 0000000..0218dba
--- /dev/null
+++ b/shell/hush_test/hush-getopts/getopt_nested.right
@@ -0,0 +1,14 @@
+var:a
+var:b
+var:c
+var:a
+var:b
+var:c
+./getopt_nested.tests: invalid option -- d
+var:?
+./getopt_nested.tests: invalid option -- e
+var:?
+./getopt_nested.tests: invalid option -- f
+var:?
+var:a
+End: var:? OPTIND:6
diff --git a/shell/hush_test/hush-getopts/getopt_nested.tests b/shell/hush_test/hush-getopts/getopt_nested.tests
new file mode 100755
index 0000000..1b48b40
--- /dev/null
+++ b/shell/hush_test/hush-getopts/getopt_nested.tests
@@ -0,0 +1,21 @@
+# Test that there is no interference of getopt()
+# in getopts and unset.
+# It's unclear what "correct" OPTIND values should be
+# for "b" and "c" results from "-bc": 2? 3?
+# What we focus on here is that all options are reported
+# correct number of times and in correct sequence.
+
+(
+
+loop=99
+while getopts "abc" var -a -bc -abc -def -a; do
+ echo "var:$var" #OPTIND:$OPTIND
+ # this may use getopt():
+ unset -ff func
+ test $((--loop)) = 0 && break # BUG if this triggers
+done
+echo "End: var:$var OPTIND:$OPTIND"
+
+) 2>&1 \
+| sed -e 's/ unrecognized option: / invalid option -- /' \
+ -e 's/ illegal option -- / invalid option -- /' \
More information about the busybox-cvs
mailing list