fix execvp PATH handling

When the execvp syscall is invoked, the system PATH should be searched
 for the executable to invoke, with the first match in the PATH being
 invoked. However, the call being modified to inject qemu breaks this
 behaviour, as it's not till _after_ qemu is invoked that the presence or
 executability is checked, which is too late. ENOENT and EACCESS aren't
 returned from the execve call, which stops execvp looping over the PATH
 and aborts the entire process.

This is resolved by testing the target command prior to executing it via
 qemu, returning the appropriate error codes.

In the case of relative paths, the prepending of the workdir resulted in
 a corrupted string being returned such as `��@/work/./script.sh`. This
 is because with malloc the memory isn't zeroed, so the memory is
 'dirty'. As a result, the strncat doesn't insert the workdir at the
 start as desired, and an invalid path is returned.

Testing the affected calls is tricky, because cross-arch binaries for
 'env' and other commands are needed in the container, and need to be
 run via emulation, which then `execvp`/`execve` out to other things
 which must also be for that architecture, and run via the emulator.
 Including a static busybox binary for the cross-arch, and customising
 the PATH for emulated calls to prefer those gives the desired behaviour.

Signed-off-by: David Ackroyd <dackroyd@nine.com.au>

Dockerfile

@@ -114,13 +114,46 @@ COPY --from=build-archive /archive/* /
 
 FROM --platform=$BUILDPLATFORM tonistiigi/bats-assert AS assert
 
+FROM --platform=$BUILDPLATFORM ${ALPINE_BASE} AS alpine-crossarch
+
+RUN apk add --no-cache bash
+
+# Runs on the build platform without emulation, but we need to get hold of the cross arch busybox binary
+# for use with tests using emulation
+ARG BUILDARCH
+RUN <<eof
+  bash -euo pipefail -c '
+    if [ "$BUILDARCH" == "amd64" ]; then
+      echo "aarch64" > /etc/apk/arch
+    else
+      echo "x86_64" > /etc/apk/arch
+    fi
+    '
+eof
+RUN apk add --allow-untrusted --no-cache busybox-static
+
+# Recreate all the symlinks for commands handled by the busybox multi-call binary such that they will use
+# the cross-arch binary, and work under emulation
+RUN <<eof
+  bash -euo pipefail -c '
+    mkdir -p /crossarch/bin /crossarch/usr/bin
+    mv /bin/busybox.static /crossarch/bin/
+    for i in $(echo /bin/*; echo /usr/bin/*); do
+     if [[ $(readlink -f "$i") != *busybox* ]]; then
+       continue
+     fi
+     ln -s /crossarch/bin/busybox.static /crossarch$i
+    done'
+eof
+
 # buildkit-test runs test suite for buildkit embedded QEMU
 FROM golang:${GO_VERSION}-alpine AS buildkit-test
 RUN apk add --no-cache bash bats
 WORKDIR /work
 COPY --from=assert . .
 COPY test .
 COPY --from=binaries / /usr/bin
+COPY --from=alpine-crossarch /crossarch /crossarch/
 RUN ./run.sh
 
 # image builds binfmt installation image 

patches/buildkit-direct-execve-v7.1/0003-linux-user-path-in-execve-should-be-relative-to-work.patch

@@ -19,8 +19,8 @@ Signed-off-by: Tibor Vass <tibor@docker.com>
 ---
  include/qemu/path.h  |  1 +
  linux-user/syscall.c |  9 +++++++--
- util/path.c          | 30 ++++++++++++++++++++++++++++++
- 3 files changed, 38 insertions(+), 2 deletions(-)
+ util/path.c          | 32 ++++++++++++++++++++++++++++++++
+ 3 files changed, 40 insertions(+), 2 deletions(-)
 
 diff --git a/include/qemu/path.h b/include/qemu/path.h
 index c6292a9709..a81fb51e1f 100644
@@ -37,7 +37,7 @@ diff --git a/linux-user/syscall.c b/linux-user/syscall.c
 index 73054926a0..b92be0963e 100644
 --- a/linux-user/syscall.c
 +++ b/linux-user/syscall.c
-@@ -8798,12 +8798,17 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
+@@ -8828,12 +8828,17 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
               * 		execve(pathname, [argv0, argv1], envp)
               * on the host, becomes:
               * 		execve("/proc/self/exe", [qemu_progname, "-0", argv0, pathname, argv1], envp)
@@ -61,7 +61,7 @@ diff --git a/util/path.c b/util/path.c
 index 8e174eb436..f7907b8238 100644
 --- a/util/path.c
 +++ b/util/path.c
-@@ -68,3 +68,33 @@ const char *path(const char *name)
+@@ -68,3 +68,35 @@ const char *path(const char *name)
      qemu_mutex_unlock(&lock);
      return ret;
  }
@@ -90,6 +90,8 @@ index 8e174eb436..f7907b8238 100644
 +    }
 +    if (!(p = malloc(k * sizeof(char*)))) return NULL;
 +
++    p[0] = '\0';
++
 +    if (!strncat(p, buf, i)) return NULL;
 +    if (!strncat(p, "/", 1)) return NULL;
 +    if (!strncat(p, path, j)) return NULL;

patches/buildkit-direct-execve-v7.1/0007-fix-execvp-PATH-handling.patch

@@ -0,0 +1,50 @@
+From 511efc9c787b9d6ea00cc2c1f6a6ec781ca7bb0c Mon Sep 17 00:00:00 2001
+From: David Ackroyd <dackroyd@nine.com.au>
+Date: Tue, 22 Nov 2022 11:18:58 +1100
+Subject: [PATCH] fix execvp PATH handling
+MIME-Version: 1.0
+Content-Type: text/plain; charset=UTF-8
+Content-Transfer-Encoding: 8bit
+
+When the execvp syscall is invoked, the system PATH should be searched
+ for the executable to invoke, with the first match in the PATH being
+ invoked. However, the call being modified to inject qemu breaks this
+ behaviour, as it's not till _after_ qemu is invoked that the presence or
+ executability is checked, which is too late. ENOENT and EACCESS aren't
+ returned from the execve call, which stops execvp looping over the PATH
+ and aborts the entire process.
+
+This is resolved by testing the target command prior to executing it via
+ qemu, returning the appropriate error codes.
+
+Signed-off-by: David Ackroyd <dackroyd@nine.com.au>
+---
+ linux-user/syscall.c | 15 +++++++++++++++
+ 1 file changed, 15 insertions(+)
+
+diff --git a/linux-user/syscall.c b/linux-user/syscall.c
+index b3d913c851..213e17a4b8 100644
+--- a/linux-user/syscall.c
++++ b/linux-user/syscall.c
+@@ -8840,6 +8840,21 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
+                 goto execve_end;
+             }
+
++            /*
++             * Check whether executable up front, as running once the qemu process is started these failures
++             * will happen internally there, and only exposed as a non-zero exit code for qemu.
++             */
++            ret = get_errno(stat(argp[3], &st));
++            if (is_error(ret)) {
++                ret = -host_to_target_errno(errno);
++                goto execve_end;
++            }
++
++            if ((st.st_mode & S_IFDIR) || !(st.st_mode & (S_IXUSR|S_IXGRP|S_IXOTH))) {
++                ret = TARGET_EACCES;
++                goto execve_end;
++            }
++
+             /* copy guest argv1 onwards to host argv4 onwards */
+             for (gp = guest_argp + 1*sizeof(abi_ulong), q = argp + 4; gp;
+                   gp += sizeof(abi_ulong), q++) {

test/run.sh

@@ -1,5 +1,6 @@
 #!/usr/bin/env sh
 
+ln -s /usr/bin/env /work/env
 go build print/printargs.go
 go build exec/execargv0.go
 
@@ -14,6 +15,7 @@ if [ "$(uname -m)" = "aarch64" ]; then
   crossEmulator="x86_64"
 fi
 
+ln -sf /crossarch/usr/bin/env /work/env
 GOARCH=$crossArch go build print/printargs.go
 GOARCH=$crossArch go build exec/execargv0.go
 

test/shebang-path.sh

@@ -0,0 +1 @@
+#!/work/env ./printargs

test/shebang-path2.sh

@@ -0,0 +1,3 @@
+#!/work/env sh
+
+./printargs "$@"

test/test.bats

@@ -6,15 +6,15 @@ exec0() {
   if [ -z "$BINFMT_EMULATOR" ]; then
     run ./execargv0 "$@"
   else 
-    run buildkit-qemu-$BINFMT_EMULATOR ./execargv0 "$@"
+    PATH=/crossarch/usr/bin:/crossarch/bin:$PATH run buildkit-qemu-$BINFMT_EMULATOR ./execargv0 "$@"
   fi
 }
 
 execdirect() {
   if [ -z "$BINFMT_EMULATOR" ]; then
     run "$@"
   else 
-    run buildkit-qemu-$BINFMT_EMULATOR "$@"
+    PATH=/crossarch/usr/bin:/crossarch/bin:$PATH run buildkit-qemu-$BINFMT_EMULATOR "$@"
   fi
 }
 
@@ -85,3 +85,53 @@ execdirect() {
   assert_success
   assert_output "./printargs ./shebang.sh foo bar1"
 }
+
+@test "relative-exec" {
+  exec0 env env ./printargs foo bar1 bar2
+  assert_success
+  assert_output "./printargs foo bar1 bar2"
+}
+
+@test "path-based-exec" {
+  PATH="$PATH:/work" exec0 env env printargs foo bar1 bar2
+  assert_success
+  assert_output "printargs foo bar1 bar2"
+}
+
+@test "shebang-path" {
+  exec0 ./shebang-path.sh ./shebang-path.sh foo bar1
+  assert_success
+  assert_output "./printargs /work/shebang-path.sh foo bar1"
+}
+
+@test "shebang-path-shell" {
+  exec0 ./shebang-path2.sh ./shebang-path2.sh foo bar1
+  assert_success
+  assert_output "./printargs foo bar1"
+}
+
+@test "shell-command-relative" {
+  if [ -n "$BINFMT_EMULATOR" ]; then
+    skip "prepend_workdir_if_relative is altering the behaviour for args when run under emulation"
+  fi
+
+  exec0 sh sh -c './shebang-path.sh foo bar1 bar2'
+  assert_success
+  assert_output "./printargs ./shebang-path.sh foo bar1 bar2"
+}
+
+@test "shell-command-relative-direct" {
+  if [ -n "$BINFMT_EMULATOR" ]; then
+    skip "prepend_workdir_if_relative is altering the behaviour for args when run under emulation"
+  fi
+
+  execdirect sh -c './shebang-path.sh foo bar1 bar2'
+  assert_success
+  assert_output "./printargs ./shebang-path.sh foo bar1 bar2"
+}
+
+@test "shell-command-relative-nested" {
+  exec0 sh sh -c './shebang-path2.sh foo bar1 bar2'
+  assert_success
+  assert_output "./printargs foo bar1 bar2"
+}