[DTrace-devel] [PATCH 20/20] cmd: fix multiple args after --
Nick Alcock
nick.alcock at oracle.com
Wed May 11 21:13:02 UTC 2022
The existing options-parsing machinery in cmd/dtrace.c uses getopt
inside a for loop, with the loop governed by optind. The purpose of
this is opaque to me: the getopt is *already* in a while loop which
won't terminate until the job is done... but if more than one arg is
found after that (say, if argv contains a -- so getopt terminates
early), the next iteration of the loop will call getopt again, which
will *reset* optind and restart parsing. Because extra arguments are
pasted into g_argv, which is sized suitably for argc to start with, this
leads to an infloop, a rapid buffer overflow, and a segfault.
(This is probably non-exploitable because a) it's dying in main()
itself, which is rather special and b) it never returns.)
The handling of getopt has changed repeatedly in the course of
development. In the Solaris days, the getopt string did not start with
+, so the argument string was permuted and when getopt terminated all
remaining entries were non-options -- but the loop still consumes only
one of them before restarting getopt, so the crash is still present. in
commit e46e2e15d270aba3f78a904be7a76c8e9fc66c17 in 2010, we switched to
POSIXLY_CORRECT for unclear reasons; this had no obviouspositive
effects, but prevented things like -i 10 20 -n ... from working; we then
switched to using + in the optstring instead, which fixed that but left
this bug still present.
DTrace is not sensitive to the relative order of options and non-options
(only to the relative order of non-options with other non-options), so
we don't need + (or -), and indeed this is harmful because if the inner
loop spots a non-option it emits a usage message (so it relies on getopt
doing permutation of non-options to appear after options, which +
prevents).
Fix by doing what almost everything else that uses getopt does, dropping
the overrun-inducing outer for loop, and doing the storage of trailing
non-option arguments in a subsequent loop after the getopt, relying on
getopt's permutation of argc and setting of optind.
(Redo the other two getopt loops similarly, even though they don't
overrun. Nearly all the diff here is indentation changes. There is no
code change: they both process only options, so the permutation carried
out by the first getopt has no effect on them.)
Add a test for this. No other tests are affected.
Signed-off-by: Nick Alcock <nick.alcock at oracle.com>
---
cmd/dtrace.c | 378 +++++++++---------
.../dtrace-util/err.MultipleNonOptFlags.d | 17 +
2 files changed, 205 insertions(+), 190 deletions(-)
create mode 100644 test/unittest/dtrace-util/err.MultipleNonOptFlags.d
diff --git a/cmd/dtrace.c b/cmd/dtrace.c
index 7e43e0f16c68..11b7e2957b52 100644
--- a/cmd/dtrace.c
+++ b/cmd/dtrace.c
@@ -49,7 +49,7 @@ typedef struct dtrace_cmd {
#define E_USAGE 2
static const char DTRACE_OPTSTR[] =
- "+3:6:aAb:Bc:CD:ef:FGhHi:I:lL:m:n:o:p:P:qs:SU:vVwx:X:Z";
+ "3:6:aAb:Bc:CD:ef:FGhHi:I:lL:m:n:o:p:P:qs:SU:vVwx:X:Z";
static char **g_argv;
static int g_argc;
@@ -945,75 +945,75 @@ main(int argc, char *argv[])
* We also accumulate arguments that are not affiliated with getopt
* options into g_argv[], and abort if any invalid options are found.
*/
- for (optind = 1; optind < argc; optind++) {
- while ((c = getopt(argc, argv, DTRACE_OPTSTR)) != EOF) {
- switch (c) {
- case '3':
- if (strcmp(optarg, "2") != 0) {
- fprintf(stderr,
- "%s: illegal option -- 3%s\n",
- argv[0], optarg);
- return usage(stderr);
- }
- g_oflags &= ~DTRACE_O_LP64;
- g_oflags |= DTRACE_O_ILP32;
- break;
+ optind = 1;
+
+ while ((c = getopt(argc, argv, DTRACE_OPTSTR)) != EOF) {
+ switch (c) {
+ case '3':
+ if (strcmp(optarg, "2") != 0) {
+ fprintf(stderr,
+ "%s: illegal option -- 3%s\n",
+ argv[0], optarg);
+ return usage(stderr);
+ }
+ g_oflags &= ~DTRACE_O_LP64;
+ g_oflags |= DTRACE_O_ILP32;
+ break;
- case '6':
- if (strcmp(optarg, "4") != 0) {
- fprintf(stderr,
- "%s: illegal option -- 6%s\n",
- argv[0], optarg);
- return usage(stderr);
- }
- g_oflags &= ~DTRACE_O_ILP32;
- g_oflags |= DTRACE_O_LP64;
- break;
+ case '6':
+ if (strcmp(optarg, "4") != 0) {
+ fprintf(stderr,
+ "%s: illegal option -- 6%s\n",
+ argv[0], optarg);
+ return usage(stderr);
+ }
+ g_oflags &= ~DTRACE_O_ILP32;
+ g_oflags |= DTRACE_O_LP64;
+ break;
- case 'e':
- g_exec = 0;
- done = 1;
- break;
+ case 'e':
+ g_exec = 0;
+ done = 1;
+ break;
- case 'h':
- g_mode = DMODE_HEADER;
- g_cflags |= DTRACE_C_ZDEFS; /* -h implies -Z */
- g_exec = 0;
- mode++;
- break;
+ case 'h':
+ g_mode = DMODE_HEADER;
+ g_cflags |= DTRACE_C_ZDEFS; /* -h implies -Z */
+ g_exec = 0;
+ mode++;
+ break;
- case 'G':
- g_mode = DMODE_LINK;
- g_cflags |= DTRACE_C_ZDEFS; /* -G implies -Z */
- g_exec = 0;
- mode++;
- break;
+ case 'G':
+ g_mode = DMODE_LINK;
+ g_cflags |= DTRACE_C_ZDEFS; /* -G implies -Z */
+ g_exec = 0;
+ mode++;
+ break;
- case 'l':
- g_mode = DMODE_LIST;
- g_cflags |= DTRACE_C_ZDEFS; /* -l implies -Z */
- mode++;
- break;
+ case 'l':
+ g_mode = DMODE_LIST;
+ g_cflags |= DTRACE_C_ZDEFS; /* -l implies -Z */
+ mode++;
+ break;
- case 'v':
- g_verbose++;
- break;
+ case 'v':
+ g_verbose++;
+ break;
- case 'V':
- g_mode = DMODE_VERS;
- mode++;
- break;
+ case 'V':
+ g_mode = DMODE_VERS;
+ mode++;
+ break;
- default:
- if (strchr(DTRACE_OPTSTR, c) == NULL)
- return usage(stderr);
- }
+ default:
+ if (strchr(DTRACE_OPTSTR, c) == NULL)
+ return usage(stderr);
}
-
- if (optind < argc)
- g_argv[g_argc++] = argv[optind];
}
+ for (; optind < argc; optind++)
+ g_argv[g_argc++] = argv[optind];
+
if (mode > 1) {
fprintf(stderr, "%s: only one of the [-GhlV] options "
"can be specified at a time\n", g_pname);
@@ -1130,134 +1130,133 @@ main(int argc, char *argv[])
* We also accumulate any program specifications into our g_cmdv[] at
* this time; these will compiled as part of the fourth processing pass.
*/
- for (optind = 1; optind < argc; optind++) {
- while ((c = getopt(argc, argv, DTRACE_OPTSTR)) != EOF) {
- switch (c) {
- case 'b':
- if (dtrace_setopt(g_dtp,
- "bufsize", optarg) != 0)
- dfatal("failed to set -b %s", optarg);
- break;
+ optind = 1;
+ while ((c = getopt(argc, argv, DTRACE_OPTSTR)) != EOF) {
+ switch (c) {
+ case 'b':
+ if (dtrace_setopt(g_dtp,
+ "bufsize", optarg) != 0)
+ dfatal("failed to set -b %s", optarg);
+ break;
- case 'B':
- g_ofp = NULL;
- break;
+ case 'B':
+ g_ofp = NULL;
+ break;
- case 'C':
- g_cflags |= DTRACE_C_CPP;
- break;
+ case 'C':
+ g_cflags |= DTRACE_C_CPP;
+ break;
- case 'D':
- if (dtrace_setopt(g_dtp, "define", optarg) != 0)
- dfatal("failed to set -D %s", optarg);
- break;
+ case 'D':
+ if (dtrace_setopt(g_dtp, "define", optarg) != 0)
+ dfatal("failed to set -D %s", optarg);
+ break;
- case 'f':
- dcp = &g_cmdv[g_cmdc++];
- dcp->dc_func = compile_str;
- dcp->dc_spec = DTRACE_PROBESPEC_FUNC;
- dcp->dc_arg = optarg;
- break;
+ case 'f':
+ dcp = &g_cmdv[g_cmdc++];
+ dcp->dc_func = compile_str;
+ dcp->dc_spec = DTRACE_PROBESPEC_FUNC;
+ dcp->dc_arg = optarg;
+ break;
- case 'F':
- if (dtrace_setopt(g_dtp, "flowindent", 0) != 0)
- dfatal("failed to set -F");
- break;
+ case 'F':
+ if (dtrace_setopt(g_dtp, "flowindent", 0) != 0)
+ dfatal("failed to set -F");
+ break;
- case 'H':
- if (dtrace_setopt(g_dtp, "cpphdrs", 0) != 0)
- dfatal("failed to set -H");
- break;
+ case 'H':
+ if (dtrace_setopt(g_dtp, "cpphdrs", 0) != 0)
+ dfatal("failed to set -H");
+ break;
- case 'i':
- dcp = &g_cmdv[g_cmdc++];
- dcp->dc_func = compile_str;
- dcp->dc_spec = DTRACE_PROBESPEC_NAME;
- dcp->dc_arg = optarg;
- break;
+ case 'i':
+ dcp = &g_cmdv[g_cmdc++];
+ dcp->dc_func = compile_str;
+ dcp->dc_spec = DTRACE_PROBESPEC_NAME;
+ dcp->dc_arg = optarg;
+ break;
- case 'I':
- if (dtrace_setopt(g_dtp, "incdir", optarg) != 0)
- dfatal("failed to set -I %s", optarg);
- break;
+ case 'I':
+ if (dtrace_setopt(g_dtp, "incdir", optarg) != 0)
+ dfatal("failed to set -I %s", optarg);
+ break;
- case 'L':
- if (dtrace_setopt(g_dtp, "libdir", optarg) != 0)
- dfatal("failed to set -L %s", optarg);
- break;
+ case 'L':
+ if (dtrace_setopt(g_dtp, "libdir", optarg) != 0)
+ dfatal("failed to set -L %s", optarg);
+ break;
- case 'm':
- dcp = &g_cmdv[g_cmdc++];
- dcp->dc_func = compile_str;
- dcp->dc_spec = DTRACE_PROBESPEC_MOD;
- dcp->dc_arg = optarg;
- break;
+ case 'm':
+ dcp = &g_cmdv[g_cmdc++];
+ dcp->dc_func = compile_str;
+ dcp->dc_spec = DTRACE_PROBESPEC_MOD;
+ dcp->dc_arg = optarg;
+ break;
- case 'n':
- dcp = &g_cmdv[g_cmdc++];
- dcp->dc_func = compile_str;
- dcp->dc_spec = DTRACE_PROBESPEC_NAME;
- dcp->dc_arg = optarg;
- break;
+ case 'n':
+ dcp = &g_cmdv[g_cmdc++];
+ dcp->dc_func = compile_str;
+ dcp->dc_spec = DTRACE_PROBESPEC_NAME;
+ dcp->dc_arg = optarg;
+ break;
- case 'P':
- dcp = &g_cmdv[g_cmdc++];
- dcp->dc_func = compile_str;
- dcp->dc_spec = DTRACE_PROBESPEC_PROVIDER;
- dcp->dc_arg = optarg;
- break;
+ case 'P':
+ dcp = &g_cmdv[g_cmdc++];
+ dcp->dc_func = compile_str;
+ dcp->dc_spec = DTRACE_PROBESPEC_PROVIDER;
+ dcp->dc_arg = optarg;
+ break;
- case 'q':
- if (dtrace_setopt(g_dtp, "quiet", 0) != 0)
- dfatal("failed to set -q");
- break;
+ case 'q':
+ if (dtrace_setopt(g_dtp, "quiet", 0) != 0)
+ dfatal("failed to set -q");
+ break;
- case 'o':
- g_ofile = optarg;
- break;
+ case 'o':
+ g_ofile = optarg;
+ break;
- case 's':
- dcp = &g_cmdv[g_cmdc++];
- dcp->dc_func = compile_file;
- dcp->dc_spec = DTRACE_PROBESPEC_NONE;
- dcp->dc_arg = optarg;
- break;
+ case 's':
+ dcp = &g_cmdv[g_cmdc++];
+ dcp->dc_func = compile_file;
+ dcp->dc_spec = DTRACE_PROBESPEC_NONE;
+ dcp->dc_arg = optarg;
+ break;
- case 'S':
- g_cflags |= DTRACE_C_DIFV;
- break;
+ case 'S':
+ g_cflags |= DTRACE_C_DIFV;
+ break;
- case 'U':
- if (dtrace_setopt(g_dtp, "undef", optarg) != 0)
- dfatal("failed to set -U %s", optarg);
- break;
+ case 'U':
+ if (dtrace_setopt(g_dtp, "undef", optarg) != 0)
+ dfatal("failed to set -U %s", optarg);
+ break;
- case 'w':
- if (dtrace_setopt(g_dtp, "destructive", 0) != 0)
- dfatal("failed to set -w");
- break;
+ case 'w':
+ if (dtrace_setopt(g_dtp, "destructive", 0) != 0)
+ dfatal("failed to set -w");
+ break;
- case 'x':
- if ((p = strchr(optarg, '=')) != NULL)
- *p++ = '\0';
+ case 'x':
+ if ((p = strchr(optarg, '=')) != NULL)
+ *p++ = '\0';
- if (dtrace_setopt(g_dtp, optarg, p) != 0)
- dfatal("failed to set -x %s", optarg);
- break;
+ if (dtrace_setopt(g_dtp, optarg, p) != 0)
+ dfatal("failed to set -x %s", optarg);
+ break;
- case 'X':
- if (dtrace_setopt(g_dtp, "stdc", optarg) != 0)
- dfatal("failed to set -X %s", optarg);
- break;
+ case 'X':
+ if (dtrace_setopt(g_dtp, "stdc", optarg) != 0)
+ dfatal("failed to set -X %s", optarg);
+ break;
- case 'Z':
- g_cflags |= DTRACE_C_ZDEFS;
- break;
+ case 'Z':
+ g_cflags |= DTRACE_C_ZDEFS;
+ break;
- default:
- if (strchr(DTRACE_OPTSTR, c) == NULL)
- return usage(stderr);
- }
+ default:
+ if (strchr(DTRACE_OPTSTR, c) == NULL)
+ return usage(stderr);
}
}
@@ -1290,37 +1289,36 @@ main(int argc, char *argv[])
* grabbing or creating victim processes. The behavior of these calls
* may been affected by any library options set by the second pass.
*/
- for (optind = 1; optind < argc; optind++) {
- while ((c = getopt(argc, argv, DTRACE_OPTSTR)) != EOF) {
- switch (c) {
- case 'c':
- if ((v = make_argv(optarg)) == NULL)
- fatal("failed to allocate memory");
-
- proc = dtrace_proc_create(g_dtp, v[0], v, 0);
- if (proc == NULL) {
- free(v);
- dfatal(NULL); /* dtrace_errmsg() only */
- }
-
- g_psv[g_psc++] = proc;
+ optind = 1;
+ while ((c = getopt(argc, argv, DTRACE_OPTSTR)) != EOF) {
+ switch (c) {
+ case 'c':
+ if ((v = make_argv(optarg)) == NULL)
+ fatal("failed to allocate memory");
+
+ proc = dtrace_proc_create(g_dtp, v[0], v, 0);
+ if (proc == NULL) {
free(v);
- break;
+ dfatal(NULL); /* dtrace_errmsg() only */
+ }
+
+ g_psv[g_psc++] = proc;
+ free(v);
+ break;
- case 'p':
- errno = 0;
- pid = strtol(optarg, &p, 10);
+ case 'p':
+ errno = 0;
+ pid = strtol(optarg, &p, 10);
- if (errno != 0 || p == optarg || p[0] != '\0')
- fatal("invalid pid: %s\n", optarg);
+ if (errno != 0 || p == optarg || p[0] != '\0')
+ fatal("invalid pid: %s\n", optarg);
- proc = dtrace_proc_grab_pid(g_dtp, pid, 0);
- if (proc == NULL)
- dfatal(NULL); /* dtrace_errmsg() only */
+ proc = dtrace_proc_grab_pid(g_dtp, pid, 0);
+ if (proc == NULL)
+ dfatal(NULL); /* dtrace_errmsg() only */
- g_psv[g_psc++] = proc;
- break;
- }
+ g_psv[g_psc++] = proc;
+ break;
}
}
diff --git a/test/unittest/dtrace-util/err.MultipleNonOptFlags.d b/test/unittest/dtrace-util/err.MultipleNonOptFlags.d
new file mode 100644
index 000000000000..8ab959c855d6
--- /dev/null
+++ b/test/unittest/dtrace-util/err.MultipleNonOptFlags.d
@@ -0,0 +1,17 @@
+/*
+ * Oracle Linux DTrace.
+ * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
+ * Licensed under the Universal Permissive License v 1.0 as shown at
+ * http://oss.oracle.com/licenses/upl.
+ */
+
+/*
+ *
+ * ASSERTION:
+ * Multiple flags after a -- should not crash.
+ *
+ */
+
+/* @@runtest-opts: -- -e -e */
+
+BEGIN { exit(0); }
--
2.36.1.263.g194b774378.dirty
More information about the DTrace-devel
mailing list