[DTrace-devel] [PATCH 20/20] cmd: fix multiple args after --
Kris Van Hees
kris.van.hees at oracle.com
Wed Jul 27 21:42:33 UTC 2022
On Wed, May 11, 2022 at 10:13:02PM +0100, Nick Alcock via DTrace-devel wrote:
> 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>
Reviewed-by: Kris Van Hees <kris.van.hees at oracle.com>
... and queued for dev
> ---
> 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
>
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list