Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong CWD value set when calling OCI poststop hook executable #18907

Closed
fangpenlin opened this issue Jun 16, 2023 · 4 comments · Fixed by #18921 · May be fixed by containers/buildah#4871
Closed

Wrong CWD value set when calling OCI poststop hook executable #18907

fangpenlin opened this issue Jun 16, 2023 · 4 comments · Fixed by #18921 · May be fixed by containers/buildah#4871
Labels
kind/bug Categorizes issue or PR as related to a bug. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Comments

@fangpenlin
Copy link
Contributor

fangpenlin commented Jun 16, 2023

Issue Description

I am figuring out how to use OCI hooks for some custom clean up implementations. But then I realized it's pretty odd that OCI poststop hook is called by podman with a wrong CWD value (I assume it's wrong, see below).

I wrote a simple C++ program for dumping the CWD value and environment variables like this

dump_env.cpp

#include <iostream>
#include <fstream>
#include <string>
#include <unistd.h>

using namespace std;

int main(int argc, char **argv, char **envp){
  ofstream output_file;
  output_file.open(string("/tmp/") + argv[0] + ".txt");
  output_file << "--- Environ Vars: ---" << endl;
  for (char **env = envp; *env != 0; env++) {
    char *thisEnv = *env;
    output_file << thisEnv << "\n";
  }
  char cwd[1024];
  output_file << "--- CWD: ---" << endl;
  output_file << getcwd(cwd, 1024) << endl;
  output_file.close();
  return 0;
}

Then I have two OCI hook json files created like this

/usr/share/containers/oci/hooks.d/start.json:

{
  "version": "1.0.0",
  "hook": {
    "path": "/tmp/dump_env",
    "args": ["start"]
  },
  "when": {
    "always": true
  },
  "stages": ["createRuntime"]
}

and this

/usr/share/containers/oci/hooks.d/stop.json:

{
  "version": "1.0.0",
  "hook": {
    "path": "/tmp/dump_env",
    "args": ["stop"]
  },
  "when": {
    "always": true
  },
  "stages": ["poststop"]
}

After run any pod run command, I got output files, for start hook it's like this:

--- Environ Vars: ---
_OCI_SYNCPIPE=3
_OCI_STARTPIPE=4
PATH=/usr/local/bin:/usr/local/sbin:/usr/bin:/usr/sbin
XDG_RUNTIME_DIR=/run/user/1000
_CONTAINERS_USERNS_CONFIGURED=init
_CONTAINERS_ROOTLESS_UID=1000
HOME=/home/user
_LIBCONTAINER_CLONED_BINARY=1
--- CWD: ---
/home/user/.local/share/containers/storage/overlay-containers/b7420ff30555da204e3dbca52edf2d4e5c09fa31d66997d95e7c432b876d4457/userdata

And the output of stop env dump looks like this

--- Environ Vars: ---
--- CWD: ---
/home/user

As you can see the CWD for start hook invoked by crun is set to the userdata folder of container. With that, I can easily read config.json and perform the task for my starting hook. But for poststop, somehow the CWD value was set to /home/user.
After digging a bit deeper, for the reasons I am not aware of, podman is making the poststop call instead of delegate it to crun here.

I haven't look at the code carefully yet, not sure why the cwd is set to home folder instead of container's userdata folder. I also tried my best to read the OCI container runtime spec see if they mention anything about setting the CWD value when calling the hook executable. Unfortunately, I didn't find anything about this part in the documents. So I assume this is an implementation details up to vendor like podman? The behavior is inconsistent between crun invoked calls and the podman invoked calls, plus it's harder to make a poststop hook works without knowing the container userdata folder, I wounder if this should be seen as a bug? I assume it is, but if it's not or this is even intentional, please feel free to close this issue.

Steps to reproduce the issue

Steps to reproduce the issue

  1. Build a simple hook program for dumping cwd value
  2. Add createRuntime and poststop hook files to use the dump cwd command to output the value to somewhere
  3. Run podman run with any image and exit the container
  4. Read the output cwd value for start and stop

Describe the results you received

Describe the results you received

Describe the results you expected

CWD value when poststop executable invoked:

/home/user

podman info output

host:
  arch: amd64
  buildahVersion: 1.30.0
  cgroupControllers: []
  cgroupManager: cgroupfs
  cgroupVersion: v1
  conmon:
    package: conmon-2.1.7-2.fc37.x86_64
    path: /usr/bin/conmon
    version: 'conmon version 2.1.7, commit: '
  cpuUtilization:
    idlePercent: 98.6
    systemPercent: 0.09
    userPercent: 1.31
  cpus: 32
  databaseBackend: boltdb
  distribution:
    distribution: fedora
    variant: container
    version: "37"
  eventLogger: journald
  hostname: MY-COMPUTER
  idMappings:
    gidmap:
    - container_id: 0
      host_id: 1000
      size: 1
    - container_id: 1
      host_id: 524288
      size: 65536
    uidmap:
    - container_id: 0
      host_id: 1000
      size: 1
    - container_id: 1
      host_id: 524288
      size: 65536
  kernel: 5.15.90.1-microsoft-standard-WSL2
  linkmode: dynamic
  logDriver: journald
  memFree: 522428416
  memTotal: 16720412672
  networkBackend: netavark
  ociRuntime:
    name: crun
    package: crun-1.8.5-1.fc37.x86_64
    path: /usr/bin/crun
    version: |-
      crun version 1.8.5
      commit: b6f80f766c9a89eb7b1440c0a70ab287434b17ed
      rundir: /run/user/1000/crun
      spec: 1.0.0
      +SYSTEMD +SELINUX +APPARMOR +CAP +SECCOMP +EBPF +CRIU +LIBKRUN +WASM:wasmedge +YAJL
  os: linux
  remoteSocket:
    exists: true
    path: /run/user/1000/podman/podman.sock
  security:
    apparmorEnabled: false
    capabilities: CAP_CHOWN,CAP_DAC_OVERRIDE,CAP_FOWNER,CAP_FSETID,CAP_KILL,CAP_NET_BIND_SERVICE,CAP_SETFCAP,CAP_SETGID,CAP_SETPCAP,CAP_SETUID,CAP_SYS_CHROOT
    rootless: true
    seccompEnabled: true
    seccompProfilePath: /usr/share/containers/seccomp.json
    selinuxEnabled: false
  serviceIsRemote: false
  slirp4netns:
    executable: /usr/bin/slirp4netns
    package: slirp4netns-1.2.0-8.fc37.x86_64
    version: |-
      slirp4netns version 1.2.0
      commit: 656041d45cfca7a4176f6b7eed9e4fe6c11e8383
      libslirp: 4.7.0
      SLIRP_CONFIG_VERSION_MAX: 4
      libseccomp: 2.5.3
  swapFree: 4117819392
  swapTotal: 4294967296
  uptime: 72h 50m 21.00s (Approximately 3.00 days)
plugins:
  authorization: null
  log:
  - k8s-file
  - none
  - passthrough
  - journald
  network:
  - bridge
  - macvlan
  - ipvlan
  volume:
  - local
registries:
  search:
  - docker.io
store:
  configFile: /home/user/.config/containers/storage.conf
  containerStore:
    number: 90
    paused: 0
    running: 0
    stopped: 90
  graphDriverName: overlay
  graphOptions: {}
  graphRoot: /home/user/.local/share/containers/storage
  graphRootAllocated: 1081101176832
  graphRootUsed: 3099852800
  graphStatus:
    Backing Filesystem: extfs
    Native Overlay Diff: "true"
    Supports d_type: "true"
    Using metacopy: "false"
  imageCopyTmpDir: /var/tmp
  imageStore:
    number: 9
  runRoot: /run/user/1000/containers
  transientStore: false
  volumePath: /home/user/.local/share/containers/storage/volumes
version:
  APIVersion: 4.5.0
  Built: 1681486976
  BuiltTime: Fri Apr 14 08:42:56 2023
  GitCommit: ""
  GoVersion: go1.19.7
  Os: linux
  OsArch: linux/amd64
  Version: 4.5.0


### Podman in a container

No

### Privileged Or Rootless

None

### Upstream Latest Release

No

### Additional environment details

Additional environment details

### Additional information

Additional information like issue happens only occasionally or issue happens with a particular architecture or on a particular setting
@fangpenlin fangpenlin added the kind/bug Categorizes issue or PR as related to a bug. label Jun 16, 2023
@fangpenlin
Copy link
Contributor Author

I read the code a bit more, the Run method is making the osexec.Cmd struct at

https://github.com/containers/podman/blob/189a74d345386c663b29df0dba19475efbb5b968/vendor/github.com/containers/common/pkg/hooks/exec/exec.go#L22C9-L29

And according to Go's doc for Cmd struct and its Dir value

https://pkg.go.dev/os/exec

// Dir specifies the working directory of the command.
// If Dir is the empty string, Run runs the command in the
// calling process's current directory.
Dir string

the Dir value is not provided, I guess that's why it inherits the cwd value from podman daemon process. I was running it remotely, maybe that's why I am seeing the wrong value. Not sure if running podman container locall would make any difference... hmmm 🤔

@fangpenlin
Copy link
Contributor Author

fangpenlin commented Jun 16, 2023

Nope, I just tried, if you run podman locally instead or remotely, it will use the current CWD where you kicked start the command. Like, when I run

sudo podman --log-level=debug run --entrypoint=/bin/sh -it alpine -c 'echo hi'

in /tmp and I will see this

--- Environ Vars: ---
--- CWD: ---
/tmp

Shouldn't be too hard to fix, I will probably create a PR tomorrow for this.

@rhatdan
Copy link
Member

rhatdan commented Jun 16, 2023

Please open a PR to fix.

@fangpenlin
Copy link
Contributor Author

@rhatdan I created a PR here

#18921

along with one for common

containers/common#1514

and one for buildah

containers/buildah#4871

As a first time contributor, I am not sure how the workflow looks like here. I didn't add change in vendor folder in my PR. Please let me know if this is not the correct way to open PR with across repo changes here, thanks! 🙏

openshift-merge-robot added a commit that referenced this issue Jun 28, 2023
…or-poststop-hook-exe

Fixes #18907, pass in correct cwd value for hooks exe
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/bug Categorizes issue or PR as related to a bug. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
2 participants