opencontainers / runc

CLI tool for spawning and running containers according to the OCI specification
https://www.opencontainers.org/
Apache License 2.0
11.58k stars 2.06k forks source link

[carry 1738] Clear hook environ variables on empty Env #4323

Open lifubang opened 2 weeks ago

lifubang commented 2 weeks ago

Carry #1738


The runtime spec has:

  • env (array of strings, OPTIONAL) with the same semantics as IEEE Std 1003.1-2008's environ.

And running execle or similar with NULL env results in an empty environent:

$ cat test.c
#include <unistd.h>

int main()
{
  return execle("/usr/bin/env", "env", NULL, NULL);
}
$ cc -o test test.c
$ ./test
…no output…

Go's Cmd.Env, on the other hand, has:

If Env is nil, the new process uses the current process's environment.

This commit works around that by setting a single dummy environment variable []string{} in those cases to avoid leaking the runtime environment into the hooks.

lifubang commented 1 week ago

Even though this is a bug, but it has existed for many years, so if we merge this, it may cause a break change for docker and other tools which use runc for the container runtime. We should check a config.json file generated by containerd to see if there is a env config for hooks.

lifubang commented 1 week ago

Even though this is a bug, but it has existed for many years, so if we merge this, it may cause a break change for docker and other tools which use runc for the container runtime. We should check a config.json file generated by containerd to see if there is a env config for hooks.

kolyshkin commented 1 week ago

What I see is crun works the same way, ie the current environment is passed on to the hook. Which is obviously wrong, but the fix may bring in some compatibility issues. Not sure how to assess if it's going to be the case...

lifubang commented 6 days ago

We should check a config.json file generated by containerd to see if there is a env config for hooks.

I had some time to look some config.json files in my company's cluster:

  1. In k8s cluster: no hooks;
  2. In docker swarm cluster: only one prestart hook, and using a absolute path:
    cat config.json | jq .hooks
    {
    "prestart": [
    {
      "path": "/proc/1158/exe",
      "args": [
        "libnetwork-setkey",
        "-exec-root=/var/run/docker",
        "2123cdedbd057cd58fba1c9869d410fcd1e6588f705e0eda6deb345d9a9670c0",
        "406aba1a8aaa"
      ]
    }
    ]
    }

I also test the binary compiled from this PR with docker, it works fine for a normal use.

but the fix may bring in some compatibility issues.

It seems that there is no compatibility issues for a normal use, but I don't know whether there are issues for some other unknown complex scenarios.