Skip to content

Commit

Permalink
cmd: fix multiple args after --
Browse files Browse the repository at this point in the history
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 e46e2e1 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@oracle.com>
Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>
  • Loading branch information
nickalcock authored and kvanhees committed Jul 27, 2022
1 parent 3a1181f commit 63a1ab5
Show file tree
Hide file tree
Showing 2 changed files with 206 additions and 191 deletions.

0 comments on commit 63a1ab5

Please sign in to comment.