From b67deb567a73ead815cb84635c84806a910390a2 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 17 Mar 2021 12:56:24 -0700 Subject: [PATCH 1/4] nsexec.c: rm a block This block apparently does nothing except for creating a need for additional indentation. Remove it. While at it, break a long line in this code. No functional change. Signed-off-by: Kir Kolyshkin --- libcontainer/nsenter/nsexec.c | 50 +++++++++++++++++------------------ 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/libcontainer/nsenter/nsexec.c b/libcontainer/nsenter/nsexec.c index a33f2fcc3..65af7401f 100644 --- a/libcontainer/nsenter/nsexec.c +++ b/libcontainer/nsenter/nsexec.c @@ -749,34 +749,34 @@ void nsexec(void) bail("failed to sync with child: write(SYNC_USERMAP_ACK)"); } break; - case SYNC_RECVPID_PLS:{ - first_child = child; + case SYNC_RECVPID_PLS: + first_child = child; - /* Get the init_func pid. */ - if (read(syncfd, &child, sizeof(child)) != sizeof(child)) { - kill(first_child, SIGKILL); - bail("failed to sync with child: read(childpid)"); - } + /* Get the init_func pid. */ + if (read(syncfd, &child, sizeof(child)) != sizeof(child)) { + kill(first_child, SIGKILL); + bail("failed to sync with child: read(childpid)"); + } - /* Send ACK. */ - s = SYNC_RECVPID_ACK; - if (write(syncfd, &s, sizeof(s)) != sizeof(s)) { - kill(first_child, SIGKILL); - kill(child, SIGKILL); - bail("failed to sync with child: write(SYNC_RECVPID_ACK)"); - } + /* Send ACK. */ + s = SYNC_RECVPID_ACK; + if (write(syncfd, &s, sizeof(s)) != sizeof(s)) { + kill(first_child, SIGKILL); + kill(child, SIGKILL); + bail("failed to sync with child: write(SYNC_RECVPID_ACK)"); + } - /* Send the init_func pid back to our parent. - * - * Send the init_func pid and the pid of the first child back to our parent. - * We need to send both back because we can't reap the first child we created (CLONE_PARENT). - * It becomes the responsibility of our parent to reap the first child. - */ - len = dprintf(pipenum, "{\"pid\": %d, \"pid_first\": %d}\n", child, first_child); - if (len < 0) { - kill(child, SIGKILL); - bail("unable to generate JSON for child pid"); - } + /* Send the init_func pid back to our parent. + * + * Send the init_func pid and the pid of the first child back to our parent. + * We need to send both back because we can't reap the first child we created (CLONE_PARENT). + * It becomes the responsibility of our parent to reap the first child. + */ + len = dprintf(pipenum, "{\"pid\": %d, \"pid_first\": %d}\n", + child, first_child); + if (len < 0) { + kill(child, SIGKILL); + bail("unable to generate JSON for child pid"); } break; case SYNC_CHILD_READY: From 1948b4cee82a6284d20130b4dc21a8f937c96cbd Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 18 Mar 2021 16:53:44 -0700 Subject: [PATCH 2/4] cloned_binary.c: rm redundant comments Remove comments with architectures when defining SYS_memfd_create, as they are redundant, and indent has a funny way of indenting them. Signed-off-by: Kir Kolyshkin --- libcontainer/nsenter/cloned_binary.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/libcontainer/nsenter/cloned_binary.c b/libcontainer/nsenter/cloned_binary.c index 2667cd65c..a5fa15382 100644 --- a/libcontainer/nsenter/cloned_binary.c +++ b/libcontainer/nsenter/cloned_binary.c @@ -65,19 +65,19 @@ # else /* These values come from . */ # warning "libc is outdated -- using hard-coded SYS_memfd_create" -# if defined(__x86_64__) // x86_64 +# if defined(__x86_64__) # define SYS_memfd_create 319 -# elif defined(__i386__) // i386 +# elif defined(__i386__) # define SYS_memfd_create 356 -# elif defined(__ia64__) // ia64 +# elif defined(__ia64__) # define SYS_memfd_create 1340 -# elif defined(__arm__) // arm +# elif defined(__arm__) # define SYS_memfd_create 385 -# elif defined(__aarch64__) // arm64 +# elif defined(__aarch64__) # define SYS_memfd_create 279 -# elif defined(__ppc__) || defined(__ppc64__) // ppc + ppc64 +# elif defined(__ppc__) || defined(__ppc64__) # define SYS_memfd_create 360 -# elif defined(__s390__) || defined(__s390x__) // s390(x) +# elif defined(__s390__) || defined(__s390x__) # define SYS_memfd_create 350 # else # error "unknown architecture -- cannot hard-code SYS_memfd_create" From 522bd6418727ad046e2128018315189afe0ed65d Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 17 Mar 2021 13:36:31 -0700 Subject: [PATCH 3/4] Fix checking C code formatting Apparently, scripts/validate-c is not working in CI (or maybe maintainers ignored the failures from it) -- current C code gets some changes if we run indent on it. This commit fixes this, simplifying things along the way. In particular: 1. Remove "validate" make target, add "cfmt" target that just runs indent on all *.c files in the repository (NOTE that *.h files are not included, as before). This may help a contributor to fix their code -- they just need to run "make cfmt" now instead of running "make validate" and copy-pasting the indent command and options from the hint. 2. Split GHA validate/misc into validate/release and validate/cfmt. The latter checks that the sources are not changed after "make cfmt". 3. Adds a few more options to indent. This was mostly motivated by trying to save the existing formatting, minimizing the amount of changes indent produces. The new options are: * -il0: sets the offset for goto labels to 0 (currently all labels but one are not indented -- let's keep it that way); * -ppi2: sets the indentation for nested preprocessor directives to 2 spaces (same as it is done in "SYS_memfd_create" defines); * -cp1: sets the indentation between #else / #endif and the following comment to 1 space. 4. Reformat the code using the new indent options. 5. Remove the now-unused script/{.validate,validate-c}. Signed-off-by: Kir Kolyshkin --- .github/workflows/validate.yml | 23 +++++++++--- Makefile | 7 ++-- libcontainer/nsenter/cloned_binary.c | 52 +++++++++++++--------------- libcontainer/nsenter/nsexec.c | 26 +++++++------- script/.validate | 33 ------------------ script/validate-c | 42 ---------------------- 6 files changed, 61 insertions(+), 122 deletions(-) delete mode 100644 script/.validate delete mode 100755 script/validate-c diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index 02c59d384..5ae223289 100644 --- a/.github/workflows/validate.yml +++ b/.github/workflows/validate.yml @@ -126,7 +126,7 @@ jobs: run: make cross - misc: + cfmt: runs-on: ubuntu-20.04 steps: - name: checkout @@ -136,8 +136,23 @@ jobs: - name: install deps run: | sudo apt -qq update - sudo apt -qq install libseccomp-dev indent - - name: make validate - run: make validate + sudo apt -qq install indent + - name: cfmt + run: | + make cfmt + git diff --exit-code + + + release: + runs-on: ubuntu-20.04 + steps: + - name: checkout + uses: actions/checkout@v2 + with: + fetch-depth: 0 + - name: install deps + run: | + sudo apt -qq update + sudo apt -qq install libseccomp-dev - name: make release run: make release diff --git a/Makefile b/Makefile index 6f65d0dcd..448fde65e 100644 --- a/Makefile +++ b/Makefile @@ -114,8 +114,9 @@ clean: rm -rf release rm -rf man/man8 -validate: - script/validate-c +cfmt: C_SRC=$(shell git ls-files '*.c' | grep -v '^vendor/') +cfmt: + indent -linux -l120 -il0 -ppi2 -cp1 -T size_t -T jmp_buf $(C_SRC) shellcheck: shellcheck tests/integration/*.bats tests/integration/*.sh tests/*.sh @@ -150,5 +151,5 @@ localcross: .PHONY: runc all recvtty static release dbuild lint man runcimage \ test localtest unittest localunittest integration localintegration \ rootlessintegration localrootlessintegration shell install install-bash \ - install-man clean validate shfmt shellcheck \ + install-man clean cfmt shfmt shellcheck \ vendor verify-dependencies cross localcross diff --git a/libcontainer/nsenter/cloned_binary.c b/libcontainer/nsenter/cloned_binary.c index a5fa15382..b9a4a14ca 100644 --- a/libcontainer/nsenter/cloned_binary.c +++ b/libcontainer/nsenter/cloned_binary.c @@ -101,7 +101,6 @@ int memfd_create(const char *name, unsigned int flags) #endif } - /* This comes directly from . */ #ifndef F_LINUX_SPECIFIC_BASE # define F_LINUX_SPECIFIC_BASE 1024 @@ -127,7 +126,7 @@ static void *must_realloc(void *ptr, size_t size) void *old = ptr; do { ptr = realloc(old, size); - } while(!ptr); + } while (!ptr); return ptr; } @@ -139,10 +138,10 @@ static void *must_realloc(void *ptr, size_t size) static int is_self_cloned(void) { int fd, ret, is_cloned = 0; - struct stat statbuf = {}; - struct statfs fsbuf = {}; + struct stat statbuf = { }; + struct statfs fsbuf = { }; - fd = open("/proc/self/exe", O_RDONLY|O_CLOEXEC); + fd = open("/proc/self/exe", O_RDONLY | O_CLOEXEC); if (fd < 0) { fprintf(stderr, "you have no read access to runc binary file\n"); return -ENOTRECOVERABLE; @@ -298,7 +297,7 @@ enum { static int make_execfd(int *fdtype) { int fd = -1; - char template[PATH_MAX] = {0}; + char template[PATH_MAX] = { 0 }; char *prefix = getenv("_LIBCONTAINER_STATEDIR"); if (!prefix || *prefix != '/') @@ -327,7 +326,7 @@ static int make_execfd(int *fdtype) *fdtype = EFD_FILE; fd = open(prefix, O_TMPFILE | O_EXCL | O_RDWR | O_CLOEXEC, 0700); if (fd >= 0) { - struct stat statbuf = {}; + struct stat statbuf = { }; bool working_otmpfile = false; /* @@ -372,27 +371,27 @@ static int seal_execfd(int *fd, int fdtype) switch (fdtype) { case EFD_MEMFD: return fcntl(*fd, F_ADD_SEALS, RUNC_MEMFD_SEALS); - case EFD_FILE: { - /* Need to re-open our pseudo-memfd as an O_PATH to avoid execve(2) giving -ETXTBSY. */ - int newfd; - char fdpath[PATH_MAX] = {0}; + case EFD_FILE:{ + /* Need to re-open our pseudo-memfd as an O_PATH to avoid execve(2) giving -ETXTBSY. */ + int newfd; + char fdpath[PATH_MAX] = { 0 }; - if (fchmod(*fd, 0100) < 0) - return -1; + if (fchmod(*fd, 0100) < 0) + return -1; - if (snprintf(fdpath, sizeof(fdpath), "/proc/self/fd/%d", *fd) < 0) - return -1; + if (snprintf(fdpath, sizeof(fdpath), "/proc/self/fd/%d", *fd) < 0) + return -1; - newfd = open(fdpath, O_PATH | O_CLOEXEC); - if (newfd < 0) - return -1; + newfd = open(fdpath, O_PATH | O_CLOEXEC); + if (newfd < 0) + return -1; - close(*fd); - *fd = newfd; - return 0; - } + close(*fd); + *fd = newfd; + return 0; + } default: - break; + break; } return -1; } @@ -400,7 +399,7 @@ static int seal_execfd(int *fd, int fdtype) static int try_bindfd(void) { int fd, ret = -1; - char template[PATH_MAX] = {0}; + char template[PATH_MAX] = { 0 }; char *prefix = getenv("_LIBCONTAINER_STATEDIR"); if (!prefix || *prefix != '/') @@ -428,7 +427,6 @@ static int try_bindfd(void) if (mount("", template, "", MS_REMOUNT | MS_BIND | MS_RDONLY, "") < 0) goto out_umount; - /* Get read-only handle that we're sure can't be made read-write. */ ret = open(template, O_PATH | O_CLOEXEC); @@ -472,7 +470,7 @@ static ssize_t fd_to_fd(int outfd, int infd) if (n < 0) return -1; nwritten += n; - } while(nwritten < nread); + } while (nwritten < nread); total += nwritten; } @@ -483,7 +481,7 @@ static ssize_t fd_to_fd(int outfd, int infd) static int clone_binary(void) { int binfd, execfd; - struct stat statbuf = {}; + struct stat statbuf = { }; size_t sent = 0; int fdtype = EFD_NONE; diff --git a/libcontainer/nsenter/nsexec.c b/libcontainer/nsenter/nsexec.c index 65af7401f..c0a059209 100644 --- a/libcontainer/nsenter/nsexec.c +++ b/libcontainer/nsenter/nsexec.c @@ -56,7 +56,7 @@ struct clone_t { * Reserve some space for clone() to locate arguments * and retcode in this place */ - char stack[4096] __attribute__ ((aligned(16))); + char stack[4096] __attribute__((aligned(16))); char stack_ptr[0]; /* There's two children. This is used to execute the different code. */ @@ -118,15 +118,15 @@ static int logfd = -1; * it, namely (glibc 2.12). */ #if __GLIBC__ == 2 && __GLIBC_MINOR__ < 14 -# define _GNU_SOURCE -# include "syscall.h" -# if !defined(SYS_setns) && defined(__NR_setns) -# define SYS_setns __NR_setns -# endif +# define _GNU_SOURCE +# include "syscall.h" +# if !defined(SYS_setns) && defined(__NR_setns) +# define SYS_setns __NR_setns +# endif -#ifndef SYS_setns -# error "setns(2) syscall not supported by glibc version" -#endif +# ifndef SYS_setns +# error "setns(2) syscall not supported by glibc version" +# endif int setns(int fd, int nstype) { @@ -136,7 +136,7 @@ int setns(int fd, int nstype) static void write_log_with_info(const char *level, const char *function, int line, const char *format, ...) { - char message[1024] = {}; + char message[1024] = { }; va_list args; @@ -187,7 +187,7 @@ static int write_file(char *data, size_t data_len, char *pathfmt, ...) goto out; } - out: +out: close(fd); return ret; } @@ -328,14 +328,14 @@ static void update_oom_score_adj(char *data, size_t len) } /* A dummy function that just jumps to the given jumpval. */ -static int child_func(void *arg) __attribute__ ((noinline)); +static int child_func(void *arg) __attribute__((noinline)); static int child_func(void *arg) { struct clone_t *ca = (struct clone_t *)arg; longjmp(*ca->env, ca->jmpval); } -static int clone_parent(jmp_buf *env, int jmpval) __attribute__ ((noinline)); +static int clone_parent(jmp_buf *env, int jmpval) __attribute__((noinline)); static int clone_parent(jmp_buf *env, int jmpval) { struct clone_t ca = { diff --git a/script/.validate b/script/.validate deleted file mode 100644 index 170d67472..000000000 --- a/script/.validate +++ /dev/null @@ -1,33 +0,0 @@ -#!/bin/bash - -if [ -z "$VALIDATE_UPSTREAM" ]; then - # this is kind of an expensive check, so let's not do this twice if we - # are running more than one validate bundlescript - - VALIDATE_REPO='https://github.com/opencontainers/runc.git' - VALIDATE_BRANCH='master' - - if [ "$TRAVIS" = 'true' -a "$TRAVIS_PULL_REQUEST" != 'false' ]; then - VALIDATE_REPO="https://github.com/${TRAVIS_REPO_SLUG}.git" - VALIDATE_BRANCH="${TRAVIS_BRANCH}" - fi - - VALIDATE_HEAD="$(git rev-parse --verify HEAD)" - - git fetch -q "$VALIDATE_REPO" "refs/heads/$VALIDATE_BRANCH" - VALIDATE_UPSTREAM="$(git rev-parse --verify FETCH_HEAD)" - - VALIDATE_COMMIT_LOG="$VALIDATE_UPSTREAM..$VALIDATE_HEAD" - VALIDATE_COMMIT_DIFF="$VALIDATE_UPSTREAM...$VALIDATE_HEAD" - - validate_diff() { - if [ "$VALIDATE_UPSTREAM" != "$VALIDATE_HEAD" ]; then - git diff "$VALIDATE_COMMIT_DIFF" "$@" - fi - } - validate_log() { - if [ "$VALIDATE_UPSTREAM" != "$VALIDATE_HEAD" ]; then - git log "$VALIDATE_COMMIT_LOG" "$@" - fi - } -fi diff --git a/script/validate-c b/script/validate-c deleted file mode 100755 index c5333a8fd..000000000 --- a/script/validate-c +++ /dev/null @@ -1,42 +0,0 @@ -#!/bin/bash - -source "$(dirname "$BASH_SOURCE")/.validate" - -IFS=$'\n' -files=($(validate_diff --diff-filter=ACMR --name-only -- '*.c' | grep -v '^vendor/' || true)) -unset IFS - -# indent(1): "You must use the ‘-T’ option to tell indent the name of all the typenames in your program that are defined by typedef." -INDENT="indent -linux -l120 -T size_t -T jmp_buf" -if [ -z "$(indent --version 2>&1 | grep GNU)" ]; then - echo "Skipping C indentation checks, as GNU indent is not installed." - exit 0 -fi - -badFiles=() -for f in "${files[@]}"; do - orig=$(mktemp) - formatted=$(mktemp) - # we use "git show" here to validate that what's committed is formatted - git show "$VALIDATE_HEAD:$f" >${orig} - ${INDENT} ${orig} -o ${formatted} - if [ "$(diff -u ${orig} ${formatted})" ]; then - badFiles+=("$f") - fi - rm -f ${orig} ${formatted} -done - -if [ ${#badFiles[@]} -eq 0 ]; then - echo 'Congratulations! All C source files are properly formatted.' -else - { - echo "These files are not properly formatted:" - for f in "${badFiles[@]}"; do - echo " - $f" - done - echo - echo "Please reformat the above files using \"${INDENT}\" and commit the result." - echo - } >&2 - false -fi From 41f466d89d4c55cfd243143fd3e7545ed935ea77 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 17 Mar 2021 14:09:25 -0700 Subject: [PATCH 4/4] nsexec.c: fix formatting for netlink defines They were not aligned, and the last two had spaces not tabs. Signed-off-by: Kir Kolyshkin --- libcontainer/nsenter/nsexec.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/libcontainer/nsenter/nsexec.c b/libcontainer/nsenter/nsexec.c index c0a059209..e8660e7c2 100644 --- a/libcontainer/nsenter/nsexec.c +++ b/libcontainer/nsenter/nsexec.c @@ -102,16 +102,16 @@ static int logfd = -1; * List of netlink message types sent to us as part of bootstrapping the init. * These constants are defined in libcontainer/message_linux.go. */ -#define INIT_MSG 62000 +#define INIT_MSG 62000 #define CLONE_FLAGS_ATTR 27281 #define NS_PATHS_ATTR 27282 -#define UIDMAP_ATTR 27283 -#define GIDMAP_ATTR 27284 +#define UIDMAP_ATTR 27283 +#define GIDMAP_ATTR 27284 #define SETGROUP_ATTR 27285 #define OOM_SCORE_ADJ_ATTR 27286 #define ROOTLESS_EUID_ATTR 27287 -#define UIDMAPPATH_ATTR 27288 -#define GIDMAPPATH_ATTR 27289 +#define UIDMAPPATH_ATTR 27288 +#define GIDMAPPATH_ATTR 27289 /* * Use the raw syscall for versions of glibc which don't include a function for