Fix regression in handling flag-values starting with hyphen

[thaJeztah]

fixes #1092

Fix for a regression between v1.22.1 and v1.22.2, where
flag values starting with a hyphen would be parsed as a flag:

runc update test_update --memory-swap -1
Incorrect Usage: flag provided but not defined: -1

This problem was caused by reorderArgs() not properly checking
if the next arg in the list was a value for the flag (and not
the next flag);

Before this patch:

args before reordering: --opt,--opt-value,arg1
args after reordering:  --opt,arg1,--opt-value
args before reordering: --opt,--opt-value,arg1,arg2
args after reordering:  --opt,arg1,--opt-value,arg2
args before reordering: arg1,--opt,--opt-value,arg2
args after reordering:  --opt,arg2,arg1,--opt-value

After this patch is applied:

args before reordering: --opt,--opt-value,arg1
args after reordering:  --opt,--opt-value,arg1
args before reordering: --opt,--opt-value,arg1,arg2
args after reordering:  --opt,--opt-value,arg1,arg2
args before reordering: arg1,--opt,--opt-value,arg2
args after reordering:  --opt,--opt-value,arg1,arg2

What type of PR is this?

(REQUIRED)

• bug
• cleanup
• documentation
• feature

What this PR does / why we need it:

see description above

Which issue(s) this PR fixes:

fixes #1092

Special notes for your reviewer:

(fill-in or delete this section)

Testing

Test was added

Release Notes

Fix a regression where flag values starting with a hyphen would be parsed as a flag instead of a value for a flag

[thaJeztah]

wrote the test to a separate file (regression_test.go) and ran git bisect (don't do this often, so 😅)

git bisect start
git bisect good v1.22.0
git bisect bad v1.22.3
Bisecting: 62 revisions left to test after this (roughly 6 steps)
[41eb645fa27833f6d76257a58e62ab9b09366c82] Merge branch 'master' into pass-through-regression


git bisect run go test -run TestParseAndRunHyphenValues
...
Bisecting: 0 revisions left to test after this (roughly 0 steps)
[bfeb4c88ec4344b456abce2b587d7bc0b957e879] Merge branch 'master' into pass-through-regression
running go test -run TestParseAndRunHyphenValues
PASS
ok  	github.com/urfave/cli	0.006s
7d0751fa1309a01b405cde327cc43275d13016a2 is the first bad commit
commit 7d0751fa1309a01b405cde327cc43275d13016a2
Author: Lynn Cyrin <[email protected]>
Date:   Wed Sep 11 19:25:51 2019 -0700

    add argIsFlag check

:100644 100644 d33435e6dd71cca15fab4a44d8ee3289f333232c 6471f1005b3e9c33be475d6dc1f322eddabfd606 M	command.go
bisect run success

git bisect log

git bisect start
# good: [bfe2e925cfb6d44b40ad3a779165ea7e8aff9212] Merge pull request #882 from urfave/lynncyrin-patch-1
git bisect good bfe2e925cfb6d44b40ad3a779165ea7e8aff9212
# bad: [c354ceca3e45cda773c3846bcb0c67cec4046b03] Merge pull request #981 from urfave/string-slice-issue-v1
git bisect bad c354ceca3e45cda773c3846bcb0c67cec4046b03
# bad: [41eb645fa27833f6d76257a58e62ab9b09366c82] Merge branch 'master' into pass-through-regression
git bisect bad 41eb645fa27833f6d76257a58e62ab9b09366c82
# good: [ef47250cda5ff52a313118c01ad6b0c5b4877a70] Merge branch 'master' into asahasrabuddhe-patch-1
git bisect good ef47250cda5ff52a313118c01ad6b0c5b4877a70
# good: [2071bcfb8328cb13e5575a586f06a33563b04054] checkpoint
git bisect good 2071bcfb8328cb13e5575a586f06a33563b04054
# bad: [09cdbbfe281fd79e92616a28f1fdf701e9007c22] more docs
git bisect bad 09cdbbfe281fd79e92616a28f1fdf701e9007c22
# bad: [fcfc69e54ff0e870bae1eaf599b7d15b46abb86b] Merge remote-tracking branch 'origin/master' into pass-through-regression
git bisect bad fcfc69e54ff0e870bae1eaf599b7d15b46abb86b
# bad: [7d0751fa1309a01b405cde327cc43275d13016a2] add argIsFlag check
git bisect bad 7d0751fa1309a01b405cde327cc43275d13016a2
# good: [bfeb4c88ec4344b456abce2b587d7bc0b957e879] Merge branch 'master' into pass-through-regression
git bisect good bfeb4c88ec4344b456abce2b587d7bc0b957e879
# first bad commit: [7d0751fa1309a01b405cde327cc43275d13016a2] add argIsFlag check

Looks like the regression was in 7d0751f (#872)

[thaJeztah]

Found the issue; ready for review @AkihiroSuda @lynncyrin PTAL

[AkihiroSuda]

Thanks

[coilysiren]

@thaJeztah I have a requested change since this issue is particularly thorny!

Tangentially, I really appreciate how simple the fix is 🙏

[thaJeztah]

Thanks for your review 🤗

Tangentially, I really appreciate how simple the fix is 🙏

Yes! It was hiding in plain sight, yet so easy to miss.

[thaJeztah]

OK, that was a lot of fun; adding tests for -- revealed a corner-case situation that looks like a bug. I pushed a second commit to address that problem, but (see my comment inline) could use some extra eyes; thanks!

[thaJeztah]

Could use extra eyes on this change; it's a result of the latest commit / fix, but could potentially be a breaking change for users (so at least should be called out in the release notes); I think I interpreted the POSIX spec correctly with my patch, but specs can sometimes be ambiguous, so could use the extra eyes.

I also haven't checked the v2 branch/version yet (possibly the fix is needed there as well, or in another form)

[thaJeztah]

So far, the twitter-verse seems to consolidate to my interpretation being correct 😂 https://twitter.com/thaJeztah/status/1260921988102656002?s=20

[coilysiren]

I'm having a hard time imagining if this is [ a mundane change, or if it'll cause mass chaos for somebody somewhere ]. Is there a way to keep this behavior as-is???

[thaJeztah]

(I'm ignoring codecov for now, as I think it's just a rounding error 😅)

[thaJeztah]

@lynncyrin anything needed to move this forward?

[coilysiren]

@thaJeztah I have (hopefully only) 1 more question!

[coilysiren]

(I'm formally requesting changes for my comment on the -- change! re-request my code review when you've answered it)

[cyphar]

This is still not quite correct if you wanted to do something like:

% runc run --bundle --some-real-runc-flag ctr

Where it would still re-order things. The least-ugly way I can think of solving this is that we change nextIndexMayContainValue to make use of TakesValue() (with some interface casting) and removing this "skip reordering flags that start with -" logic if it doesn't break anything.

I also hasten to point out that urfave/cli has actually had this behaviour for a very long time and we (speaking collectively in the containers community) have misunderstood how urfave/cli handles this case. I discussed this in #1152 (which I've marked as a duplicate of this) and I've had to work around this in umoci (see opencontainers/umoci#328 for how I ended up hotfixing this -- you just disable argument reordering). Since we want to avoid breaking other users of urfave/cli we might have to migrate runc, umoci, and containerd away from urfave/cli (maybe to spf13/cobra which is what Docker uses)...

[cyphar]

A possible patch to do the above might look like the following. This change does pass all of the existing tests...

diff --git a/command.go b/command.go
index f02d3589ff3a..3c8e971a1c53 100644
--- a/command.go
+++ b/command.go
@@ -223,7 +223,7 @@ func (c *Command) useShortOptionHandling() bool {
 func reorderArgs(commandFlags []Flag, args []string) []string {
        var remainingArgs, reorderedArgs []string
 
-       nextIndexMayContainValue := false
+       nextIndexIsValue := false
        for i, arg := range args {
 
                // dont reorder any args after a --
@@ -233,17 +233,29 @@ func reorderArgs(commandFlags []Flag, args []string) []string {
                        remainingArgs = append(remainingArgs, args[i:]...)
                        break
 
-                       // checks if this arg is a value that should be re-ordered next to its associated flag
-               } else if nextIndexMayContainValue && !strings.HasPrefix(arg, "-") {
-                       nextIndexMayContainValue = false
+                       // if this argument is a value then it isn't a flag and should be
+                       // kept with its associated flag (which was the last flag
+                       // reordered)
+               } else if nextIndexIsValue {
+                       nextIndexIsValue = false
                        reorderedArgs = append(reorderedArgs, arg)
 
                        // checks if this is an arg that should be re-ordered
-               } else if argIsFlag(commandFlags, arg) {
+               } else if flag := findFlagForArg(commandFlags, arg); flag != nil {
                        // we have determined that this is a flag that we should re-order
                        reorderedArgs = append(reorderedArgs, arg)
-                       // if this arg does not contain a "=", then the next index may contain the value for this flag
-                       nextIndexMayContainValue = !strings.Contains(arg, "=")
+
+                       // if the flag takes an argument and doesn't contain a "=" then the
+                       // next index contains a value for this flag -- unfortunately we
+                       // have to depend on the Flag implementing DocGenerationFlag for
+                       // TakesValue().
+                       if docFlag, ok := (*flag).(DocGenerationFlag); ok {
+                               nextIndexIsValue = docFlag.TakesValue() && !strings.Contains(arg, "=")
+                       } else {
+                               // assume that flags take arguments
+                               // XXX: This is quite ugly and I'm not sure what we should do here.
+                               nextIndexIsValue = !strings.Contains(arg, "=")
+                       }
 
                        // simply append any remaining args
                } else {
@@ -254,15 +266,16 @@ func reorderArgs(commandFlags []Flag, args []string) []string {
        return append(reorderedArgs, remainingArgs...)
 }
 
-// argIsFlag checks if an arg is one of our command flags
-func argIsFlag(commandFlags []Flag, arg string) bool {
+// findFlagForArg checks if an arg is one of our command flags and returns the
+// Flag if the arg is a flag (nil otherwise).
+func findFlagForArg(commandFlags []Flag, arg string) *Flag {
        // checks if this is just a `-`, and so definitely not a flag
        if arg == "-" {
-               return false
+               return nil
        }
        // flags always start with a -
        if !strings.HasPrefix(arg, "-") {
-               return false
+               return nil
        }
        // this line turns `--flag` into `flag`
        if strings.HasPrefix(arg, "--") {
@@ -279,12 +292,12 @@ func argIsFlag(commandFlags []Flag, arg string) bool {
                for _, key := range strings.Split(flag.GetName(), ",") {
                        key := strings.TrimSpace(key)
                        if key == arg {
-                               return true
+                               return &flag
                        }
                }
        }
-       // return false if this arg was not one of our flags
-       return false
+       // couldn't find the flag
+       return nil
 }
 
 // Names returns the names including short names and aliases.

[thaJeztah]

@cyphar I see you commented on the first commit; did you check with the second commit as well?

[thaJeztah]

(I was looking at the requested changes to preserve some of the existing behaviour)

[cyphar]

No, this was just talking about the behaviour of the first change (and not the fun -- semantics that you quizzed everyone on in May). I /think/ that it should be fairly straight-forward (move the -- check after the "is this an option-value check" as in your current patch) but I agree with both of you that that particular change looks quite scary.

I think the --opt -a case is far more clear-cut.

[thaJeztah]

So, in your example case (runc run --bundle --some-real-runc-flag) both --bundle and --some-real-runc-flag are "known" flags to runc, but the second one should be treated as value for --bundle (so --bundle="--some-real-runc-flag") or as flag? In the former case, wouldn't it have to be used as runc run --bundle -- --some-real-runc-flag? 🤔)

[cyphar]

The former (--some-real-runc-flag treated as a avalue for --bundle). The motivating use-case is (aside from POSIX-correctness and compatibility with getopt(3)) similar to --:

% runc run --bundle --foo ctr # where ./--foo is a directory containing an OCI bundle

(Well, for umoci it's more like umoci config --image foo:bar --config.cmd sh --config.cmd -c but that's a slightly more complicated example. But the root cause is the same as your original --memory-limit -1 issue.)

Should mean that --foo is a value for --bundle regardless of whether --foo is a known command-line flag. Currently (and with your patches), urfave/cli will not reorder --bundle --foo (where --foo is not a valid flag for runc) but it will reorder the flags if you do --bundle --systemd-cgroup (where --systemd-cgroup is a supported flag for runc). Even more strangely, --bundle=--systemd-cgroup works without issue because the arguments don't get reordered.

The patch I posted above should make it so that --bundle=--foo acts exactly like --bundle --foo regardless of whether --foo is a valid runc flag.

In the former case, wouldn't it have to be used as runc run --bundle -- --some-real-runc-flag?

I disagree (especially if we take the interpretation that -- should be an option-value in that case). If you use getopt:

% getopt -o '' -l bundle:,systemd-cgroup: -- --bundle --systemd-cgroup container-name
 --bundle '--systemd-cgroup' -- 'container-name'

But also it's the obvious behaviour if you just inject =s:

 getopt -o '' -l bundle:,systemd-cgroup: -- --bundle=--systemd-cgroup container-name
 --bundle '--systemd-cgroup' -- 'container-name'

[AkihiroSuda]

@thaJeztah PTAL?

[AkihiroSuda]

ping @thaJeztah 🙏

[coilysiren]

👀

[cyphar]

I think my earlier comment (#1135 (comment)) still stands. I can cook up a proper PR based on the patch I posted above which handles --flagA --flagB as "--flagB" is the value of --flagA regardless of whether --flagB is a known flag, if you'd still like that change.

[brandond]

@cyphar is this still on your radar?

[stale]

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

[stale]

Closing this as it has become stale.

[cyphar]

I think on paper this is still something we'd like to have fixed for runc and umoci, but disabling argument reordering fixed the issue well enough that it isn't bothering us at the moment (though it does mean that some of the other argument reordering features now don't work because we've disabled them). I can take a crack at implementing the behaviour I mentioned above (I think I might still have the code on my laptop) but I'd like to know whether there is interest in taking it -- you've mentioned before that touching this stuff can be quite hairy and it's not clear whether fixing a bug for one user will go on to cause bugs for another five (and as a fellow maintainer I get where you're coming from!).

[meatballhat]

Superseded by #1356