First hook arg is overwritten with cmd

[nakato]

Setup

oci-runtime-tool: 0.3.0
runc: 1.0.0-rc4 and fb6ec65

Problem

When attempting to run a hook command with 1 or more arguments the first arg argument is discarded and replaced with the contents of path.

Given the following:

{
    ...
    "hooks": {
        "prestart": [
            "path": "/test.py",
            "args": [
                "pre"
            ]
        ],
    ...
}

the hook recives argv of ["/test.py"]

Given:

{
    ...
    "hooks": {
        "prestart": [
            "path": "/test.py",
            "args": [
                "anything",
                "pre"
            ]
        ],
    ...
}

The hook recives an argv of ["/test.py", "pre"]

Expectation

To me, I would expect that given the first example above, that the hook's argv looks like ["/test.py", "pre"], and the second one ["/test.py", "anything", "pre"]

Using q pip install q for lazy logging, the following will dump the argv to /tmp/q, and then promptly exit.

#!/usr/bin/env python3

import sys

import q

q(len(sys.argv))
q(sys.argv)

[cyphar]

This is expected. The specification requires that hook args acts in the same way as execve(2). This is useful for binaries like busybox which check the value of argv[0] when deciding what to do.

[lynnju]

@cyphar If this behaviour is expected, it is clearly confusing to some and the swallowing of arguments can lead to some rather difficult to debug issues (state/model mismatch for the developer). Perhaps an optional, but on by default, warning is warranted if usages such as these are detected?

[cyphar]

Maybe, though I disagree it's confusing (since it matches how C acts) and generally I don't like adding warnings for valid usecases. Here's the section of the spec if you're interested -- as you can see the spec matches POSIX's execv semantics. I'd recommend reading the spec as it acts as far more thorough documentation of config.json than any docs we could write independently.

(As an aside -- holy shit! Fellow Sydney-siders! 😸)

[lynnju]

@cyphar Well, yes, it may be how C acts, but this isn't C. Invalid invocations shouldn't be representable -- but if they are and the system can detect that, I think it should complain. Obviously, I think the system should have some sort of a "yes, I meant to do that" so you don't spam people taking advantage of the execv semantics but is that really the usual use case or is it likely to be a mistake? If it's more likely to be a mistake, then I would argue for a foot-gun guard.

[lynnju]

Actually, instead of a warning, perhaps I should argue that we're not actually following the specification -- the argv value is passed through unchanged there. Here we're overwriting argv[0] with our own value, discarding what is set by the developer. Perhaps this might work: Instead, of argv[0] = ... push our value to the argv array at index 0 shifting the others to the right. That way, our behaviour actually matches what we're doing and it gets rid of the mismatch. If you're worried about "there's no way to set argv[0]" ... well, that's how it is now. Either that or we should stop setting argv[0] as a convenience for the developer -- technically we're breaking the execv argv spec as it is now.

[cyphar]

@lynnju

Well, yes, it may be how C acts, but this isn't C.

Well, yes, but the OCI specification refers to POSIX -- so while it isn't C, the POSIX C semantics are how args is meant to act. Not to mention these semantics are not unique to C. In Go, os/exec.Cmd has the same semantics for Cmd.Path and Cmd.Args. Rust has the same semantics. To be fair, Python doesn't have those semantics with its subprocess module, so this isn't a universal thing. But it all comes down to that the spec defines that this is how the interface should look -- if you'd like to change that you're more than welcome to propose the change to https://github.com/opencontainers/runtime-spec -- but changing it would break busybox-like users.

Here we're overwriting argv[0] with our own value, discarding what is set by the developer.

Ah, you're right. I agree this is a bug, but in my mind the fix would be to pass the args provided in the config.json (not the shifting idea that you propose). I can work up a patch for this in a bit.

[nakato]

If runc is modified to match the spec and not modify args in any way, then existing configs where the hooks config resembles the following will break. This format currently works and is how oci-runtime-tool currently generates the config file.

"hooks": {
    "prestart" [
        {
            "path": "/path/to/hook"
        }
    ]
}

What benifits does having control over the first arg bring?

To me this feels like an implmentation detail of the programming language that probably shouldn't be exposed at this layer. I am not required to use /usr/bin/ls /usr/bin/ls /path/to/list in a shell, in cron, or anywhere else that command execution is exposed outside of a programming language.

I am however fine with it matching spec, I would not have been so confused with what was happening if it matched spec.

Busybox supports
busybox $function $functionargs usage.
so:
busybox ls /path works.
The links to /bin/ls to busybox and argv[0] setting the function to be called is for user-convience.

[kolyshkin]

I don't see that this is broken. Yes, ability to specify argv[0] in addition to command is somewhat weird, but it is what it is in the spec, and currently runc passes argv[0] as it should be (checking it now in PR #4593).

Also, the busybox argument is kinda moot, as it also accepts the form where argv[0] is "/some/path/to/busybox" and argv[1] is an applet name. So, it could be used just fine without an ability to set argv[0].

Anyway, I guess this can be closed.