Re: [PATCH 0/5] linux-user changes to run docker

[Alex Bennée]

YAMAMOTO Takashi <yamamoto@midokura.com> writes:

> These patches, along with a few more hacks [1] I didn't include
> in this patchset, allowed me to run arm64 and armv7 version of
> dind image on amd64.
>
> [1] https://github.com/yamt/qemu/tree/linux-user-for-docker

Might be worth posting those patches next time (even if they have a RFC
or !MERGE in the title for now). I had a little noodle around with
testing and quickly found a few holes. It would be nice if we could have
a unit test to cover these various bits as I fear it will easily break
again. Feel free to use the following as a basis if you want:

>From 5d331e84f3e8763921a619647a46bc8b4c9f3207 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <alex.bennee@linaro.org>
Date: Mon, 24 May 2021 10:49:55 +0100
Subject: [PATCH 1/2] tests/tcg: simple test for /proc/self behaviour
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/tcg/multiarch/self.c | 114 +++++++++++++++++++++++++++++++++++++
 1 file changed, 114 insertions(+)
 create mode 100644 tests/tcg/multiarch/self.c

diff --git a/tests/tcg/multiarch/self.c b/tests/tcg/multiarch/self.c
new file mode 100644
index 0000000000..f6ea145d16
--- /dev/null
+++ b/tests/tcg/multiarch/self.c
@@ -0,0 +1,114 @@
+/*
+ * /proc/self checks
+ *
+ * Copyright (c) 2021 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#define _GNU_SOURCE
+#include <stdarg.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <errno.h>
+#include <string.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
+#include <assert.h>
+#include <fcntl.h>
+
+static void error1(const char *filename, int line, const char *fmt, ...)
+{
+    va_list ap;
+    va_start(ap, fmt);
+    fprintf(stderr, "%s:%d: ", filename, line);
+    vfprintf(stderr, fmt, ap);
+    fprintf(stderr, "\n");
+    va_end(ap);
+    exit(1);
+}
+
+static int __chk_error(const char *filename, int line, int ret)
+{
+    if (ret < 0) {
+        error1(filename, line, "%m (ret=%d, errno=%d/%s)",
+               ret, errno, strerror(errno));
+    }
+    return ret;
+}
+
+#define error(fmt, ...) error1(__FILE__, __LINE__, fmt, ## __VA_ARGS__)
+
+#define chk_error(ret) __chk_error(__FILE__, __LINE__, (ret))
+
+#define PATH_MAX 1024
+
+static void check_self_exe(struct stat *self)
+{
+    struct stat statbuf;
+    struct stat linkbuf;
+    chk_error(stat("/proc/self/exe", &statbuf));
+    chk_error(lstat("/proc/self/exe", &linkbuf));
+    assert(statbuf.st_ino != linkbuf.st_ino);
+    assert(statbuf.st_ino == self->st_ino);
+}
+
+static void check_mypid(struct stat *self)
+{
+    pid_t me = getpid();
+    char path[PATH_MAX];
+    struct stat statbuf;
+    struct stat linkbuf;
+
+    snprintf(&path[0], PATH_MAX, "/proc/%d/exe", me);
+
+    chk_error(stat(path, &statbuf));
+    chk_error(lstat(path, &linkbuf));
+    assert(statbuf.st_ino != linkbuf.st_ino);
+    assert(statbuf.st_ino == self->st_ino);
+}
+
+static void check_noncanon(struct stat *self)
+{
+    struct stat statbuf;
+    int fd_slash, fd_dot;
+
+    fd_slash = openat(AT_FDCWD, "/proc///self/exe", O_PATH);
+    chk_error(fstat(fd_slash, &statbuf));
+    assert(statbuf.st_ino == self->st_ino);
+    close(fd_slash);
+
+    fd_dot = openat(AT_FDCWD, "/proc/./self/exe", O_PATH);
+    chk_error(fstat(fd_dot, &statbuf));
+    assert(statbuf.st_ino == self->st_ino);
+    close(fd_dot);
+}
+
+int main(int argc, char **argv)
+{
+    struct stat self;
+
+    chk_error(stat(argv[0], &self));
+    printf("I am %s (%d/%lu)\n", argv[0], argc,
+           (long unsigned int) self.st_ino);
+
+    check_self_exe(&self);
+    check_mypid(&self);
+    check_noncanon(&self);
+
+#if 0
+    if (argc == 2) {
+        printf("I think I execve'd myself\n");
+    } else {
+        char *new_argv[3] = { argv[0], "again", NULL };
+        chk_error(execve("/proc/self/exe", new_argv, NULL));
+        /* should never return */
+        return -1;
+    }
+#endif
+
+    return 0;
+}

base-commit: d36f3ecdc70af8941053cca8347daca757be2865
-- 
2.20.1


> You can find my test setup here:
> https://github.com/yamt/garbage/tree/master/binfmt-aarch64-install
>
> YAMAMOTO Takashi (5):
>   linux-user: handle /proc/self/exe for execve
>   linux-uesr: make exec_path realpath
>   linux-user: Fix the execfd case of /proc/self/exe open
>   linux-user: dup the execfd on start up
>   linux-user: Implement pivot_root
>
>  linux-user/main.c    | 14 +++++++++++++-
>  linux-user/qemu.h    |  2 ++
>  linux-user/syscall.c | 43 ++++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 55 insertions(+), 4 deletions(-)

I also had a go at cleaning up is_proc_self and Daniel greatly
simplified it.

>From fe342309661e3fe8b9e192e6df6ef84267207dac Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <alex.bennee@linaro.org>
Date: Mon, 24 May 2021 12:19:18 +0100
Subject: [PATCH 2/2] linux-user: glib-ify is_proc_myself
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

For the cost of a couple of heap allocations we can reduce the code
complexity to a couple of string compares. While we are at it make the
function a bool return and fixup the fake_open function prototypes.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - use danpb's suggestion
---
 linux-user/syscall.c | 31 +++++++------------------------
 1 file changed, 7 insertions(+), 24 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index e739921e86..8af48b5f1f 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7987,33 +7987,16 @@ static int open_self_auxv(void *cpu_env, int fd)
     return 0;
 }
 
-static int is_proc_myself(const char *filename, const char *entry)
-{
-    if (!strncmp(filename, "/proc/", strlen("/proc/"))) {
-        filename += strlen("/proc/");
-        if (!strncmp(filename, "self/", strlen("self/"))) {
-            filename += strlen("self/");
-        } else if (*filename >= '1' && *filename <= '9') {
-            char myself[80];
-            snprintf(myself, sizeof(myself), "%d/", getpid());
-            if (!strncmp(filename, myself, strlen(myself))) {
-                filename += strlen(myself);
-            } else {
-                return 0;
-            }
-        } else {
-            return 0;
-        }
-        if (!strcmp(filename, entry)) {
-            return 1;
-        }
-    }
-    return 0;
+static bool is_proc_myself(const char *filename, const char *entry)
+{
+    g_autofree char *procself = g_strdup_printf("/proc/self/%s", entry);
+    g_autofree char *procpid = g_strdup_printf("/proc/%d/%s", getpid(), entry);
+    return g_str_equal(filename, procself) || g_str_equal(filename, procpid);
 }
 
 #if defined(HOST_WORDS_BIGENDIAN) != defined(TARGET_WORDS_BIGENDIAN) || \
     defined(TARGET_SPARC) || defined(TARGET_M68K) || defined(TARGET_HPPA)
-static int is_proc(const char *filename, const char *entry)
+static bool is_proc(const char *filename, const char *entry)
 {
     return strcmp(filename, entry) == 0;
 }
@@ -8097,7 +8080,7 @@ static int do_openat(void *cpu_env, int dirfd, const char 
*pathname, int flags,
     struct fake_open {
         const char *filename;
         int (*fill)(void *cpu_env, int fd);
-        int (*cmp)(const char *s1, const char *s2);
+        bool (*cmp)(const char *s1, const char *s2);
     };
     const struct fake_open *fake_open;
     static const struct fake_open fakes[] = {
-- 
2.20.1


-- 
Alex Bennée