arg parsing: single-dash options expanded incorrectly

[edsantiago]

Sorry for the unhelpful title; I can't think of a better description. This one has been haunting me today:

# podman run centos find / -name sdf
find: unknown predicate `-n'
# podman run centos echo find / -name sdf
find / -n -a -m -e sdf

Somewhere in podman-land, -name is being rewritten and passed along as -n -a -m -e.

podman-0.4.4-2.git9924956.fc27 and 0.5.1-1525212894.git970eaf0.fc27

[mheon]

@baude This sounds like shortopts handling in urfave/cli

[rhatdan]

Does it work if you do

podman run centos -- find / -name sdf

[rhatdan]

Although the error looks like it is coming from the find command.

[edsantiago]

#  podman run centos -- find / -name sdf
container create failed: container_linux.go:348: starting container process caused "exec: \"--\": executable file not found in $PATH"
: internal libpod error

also tried:

# podman run -- centos echo find / -name sdf
find / -n -a -m -e sdf

(Translation: AFAICT -- is not telling podman to stop fiddling with args).

Although the error looks like it is coming from the find command

Yes, because find / -n is not valid. find is one of those very weird old-style pre-posix commands that uses a single dash with long-name options.

[mheon]

I know we had issues with shortopts handling early on (podman run -ti ... wouldn't work, you needed to explicitly specify podman run -t -i ...) that we fixed in urfave/cli. I suspect that fix has come back and bit us.

[TomSweeneyRedHat]

@mheon yep, this smells very much like urfave cli. I tried a number of twists without luck. I'd thought off the bat that the '--' would work like Ed mentions, but it's not appearing to.

[rhatdan]

podman run -- centos sh -c "find / -name sdf"
Works/

[rhatdan]

But this is definately caused by our patch to urvave after seeing the echo example. @baude You were involved in this patch, do you want to take a look. I think we need to stop processing after the argv0

[edsantiago]

Ping, I'm still seeing this, podman-0.6.3-1.gitcd39b62.fc28

[rhatdan]

Yup noone has worked with upstream on this issue.

[vrothberg]

I'll have a look. This bug is pretty annoying.

[vrothberg]

The following patch fixes the upper issue. What it does is to query if each "short flag" is actually in the flag set. If one or more is not in the set, the string will be left untouched and passed back to further parsing, so that something like podman run -itrxwerewr will yield a flag provided but not defined: -itrxwerewr.

Do you have feedback before proposing the fix upstream?

diff --git a/vendor/github.com/urfave/cli/command.go b/vendor/github.com/urfave/cli/command.go
index 66a58b5a8954..a21a7621153f 100644
--- a/vendor/github.com/urfave/cli/command.go
+++ b/vendor/github.com/urfave/cli/command.go
@@ -182,7 +182,7 @@ func (c *Command) parseFlags(args Args) (*flag.FlagSet, error) {
 	}
 
 	if c.UseShortOptionHandling {
-		args = translateShortOptions(args)
+		args = translateShortOptions(set, args)
 	}
 
 	if !c.SkipArgReorder {
@@ -231,11 +231,25 @@ func reorderArgs(args []string) []string {
 	return append(flags, nonflags...)
 }
 
-func translateShortOptions(flagArgs Args) []string {
+func translateShortOptions(set *flag.FlagSet, flagArgs Args) []string {
+	allCharsFlags := func (s string) bool {
+		for i := range s {
+			f := set.Lookup(string(s[i]))
+			if f == nil {
+				return false
+			}
+		}
+		return true
+	}
+
 	// separate combined flags
 	var flagArgsSeparated []string
 	for _, flagArg := range flagArgs {
 		if strings.HasPrefix(flagArg, "-") && strings.HasPrefix(flagArg, "--") == false && len(flagArg) > 2 {
+			if !allCharsFlags(flagArg[1:]) {
+				flagArgsSeparated = append(flagArgsSeparated, flagArg)
+				continue
+			}
 			for _, flagChar := range flagArg[1:] {
 				flagArgsSeparated = append(flagArgsSeparated, "-"+string(flagChar))
 			}
@@ -243,6 +257,7 @@ func translateShortOptions(flagArgs Args) []string {
 			flagArgsSeparated = append(flagArgsSeparated, flagArg)
 		}
 	}
+
 	return flagArgsSeparated
 }
 

[rhatdan]

@vrothberg That looks great to me. Nice work.

[vrothberg]

Damn, that's only a half fix as podman run alpine ls -it would still be turned into podman run alpine ls -i -t.

[TomSweeneyRedHat]

Yeah, I've tried dealing with urfave parsing in the past and you can get stuck in spidey webs real quick.

[edsantiago]

Indeed. I've just surfaced from a couple of hours of ratholing in there. Useful as a golang learning exercise, but I got nowhere -- only far enough to suspect that the problem isn't fixable with this set of tools.

[vrothberg]

I have a working solution by catching errors, but it's spaghetti goto. I'll draft a PR tomorrow and see what upstream thinks about it. The only other solution I see would be using another flag package.

[vrothberg]

Here's a PR for urfave/cli (hopefully) fixing the issue: urfave/cli#758

Smoke tests were successful and I think there are no side effects.

[rhatdan]

@vrothberg Won't this still fail if you do an command like

podman run fedora find / -printf ...

Will urvave still see this as a valid -i and -t?

Would a better fix to be just stop interpreting the options after the fedora object when we tell it don't sort the options.

[vrothberg]

Would a better fix to be just stop interpreting the options after the fedora object when we tell it don't sort the options.

That's what's happening actually. set.Parse(args) will stop interpreting input as flags/options when hitting fedora. Everything after that will be interpreted as arguments.

[vrothberg]

podman run fedora find / -printf ...

Will urvave still see this as a valid -i and -t?

No it won't. It will stop at fedora and use the remainder as arguments.

[rhatdan]

Awesome.

[vrothberg]

@rhatdan To be more specific. The parsing errors the patch is catching are always on the left side of the specified image. For instance, when running podman run -it fedora:latest ls -it:

• we'll catch the error that -it is not defined and transform that into -i -t
• then we reparse everything again with podman run -i -t fedora:latest ls -it

What the patch also does is to make sure that all short flags in -abc are actually defined in the FlagSet, so we get a -abc is not defined instead of a -c is not defined.

[rhatdan]

My fear looking through the PRs, there has not been anything merged since Feb 21st. I am not sure how alive this project is.

We might need to carry this patch some how.

[vrothberg]

We might need to carry this patch some how.

I begin to share your fear. Given the bug is blocking some common usage patterns, I'd like to push it a bit forward and carry the patch until urfave/cli is revived. Any objections against committing the change on vendor/github.com/urfave/cli?

[rhatdan]

If you modify the vendor.conf to point at your repo from vendor/github.com/urfave/cli then I think we can carry it.

Something like
github.com/urfave/cli UUID https://github.com/vrothberg/cli

[vrothberg]

Done. I opened a PR: #1046