From 7b9b9b6224d970257aeb7b8ac869705d807e7b77 Mon Sep 17 00:00:00 2001 From: agnostic-apollo Date: Sun, 24 Jul 2022 11:51:21 +0500 Subject: [PATCH] fix(run-docker.sh): Fix docker exec not passing kill signals (ctrl+c) to commands in some cases leaving processes still running If `--tty` is not passed to `docker exec` because stdout is not available (`[ ! -t 1 ]`), like due to redirection to file (`&> build.log`) or if stdin is not available (`< /dev/null`), then docker does not forward kill signals to the process started and they remain running. To fix the issue, the `DOCKER_EXEC_PID_FILE_PATH` env variable with the value `/tmp/docker-exec-pid-` is passed to the process called with `docke exec` and the process started stores its pid in the file path passed. Traps are set in `run-docker.sh` that runs the `docker exec` command to receive any kills signals, and if it does, it runs another `docker exec` command to read the pid of the process previously started from `DOCKER_EXEC_PID_FILE_PATH` and then kills it and all its children. See Also: https://github.com/docker/cli/issues/2607 https://github.com/moby/moby/issues/9098 https://github.com/moby/moby/pull/41548 https://stackoverflow.com/questions/41097652/how-to-fix-ctrlc-inside-a-docker-container Also passing `--init` to `docker run` to "Run an init inside the container that forwards signals and reaps processes", although it does not work for above cases, but may helpful in others. The `--init` flag changes will only engage on new container creation. https://docs.docker.com/engine/reference/run/#specify-an-init-process https://docs.docker.com/engine/reference/commandline/run/ ``` ./scripts/run-docker.sh ./build-package.sh -f libjpeg-turbo &> build.log ^C $ ./scripts/run-docker.sh ps -efww Running container 'termux-package-builder' from image 'termux/package-builder'... UID PID PPID C STIME TTY TIME CMD builder 1 0 0 05:48 pts/0 00:00:00 bash builder 9243 0 0 06:01 pts/1 00:00:00 bash builder 28127 0 0 06:12 ? 00:00:00 /bin/bash ./build-package.sh -f libjpeg-turbo builder 28141 28127 0 06:12 ? 00:00:00 /bin/bash ./build-package.sh -f libjpeg-turbo builder 28449 28141 1 06:12 ? 00:00:00 ninja -w dupbuild=warn -j 8 builder 28656 28449 0 06:12 ? 00:00:00 /bin/sh -c /home/builder/.termux-build/_cache/android-r23c-api-24-v0/bin/clang builder 28657 28656 79 06:12 ? 00:00:01 /home/builder/.termux-build/_cache/android-r23c-api-24-v0/bin/clang builder 28694 28449 0 06:12 ? 00:00:00 /bin/sh -c /home/builder/.termux-build/_cache/android-r23c-api-24-v0/bin/clang builder 28695 28694 89 06:12 ? 00:00:00 /home/builder/.termux-build/_cache/android-r23c-api-24-v0/bin/clang builder 28728 28449 0 06:12 ? 00:00:00 /bin/sh -c /home/builder/.termux-build/_cache/android-r23c-api-24-v0/bin/clang builder 28729 28728 0 06:12 ? 00:00:00 /home/builder/.termux-build/_cache/android-r23c-api-24-v0/bin/clang builder 28731 28449 0 06:12 ? 00:00:00 /bin/sh -c /home/builder/.termux-build/_cache/android-r23c-api-24-v0/bin/clang builder 28734 28731 0 06:12 ? 00:00:00 /home/builder/.termux-build/_cache/android-r23c-api-24-v0/bin/clang builder 28740 28449 0 06:12 ? 00:00:00 /bin/sh -c /home/builder/.termux-build/_cache/android-r23c-api-24-v0/bin/clang builder 28741 28740 0 06:12 ? 00:00:00 /home/builder/.termux-build/_cache/android-r23c-api-24-v0/bin/clang builder 28744 0 0 06:12 pts/2 00:00:00 ps -efww builder 28748 28449 0 06:12 ? 00:00:00 /bin/sh -c /home/builder/.termux-build/_cache/android-r23c-api-24-v0/bin/clang builder 28752 28748 0 06:12 ? 00:00:00 /home/builder/.termux-build/_cache/android-r23c-api-24-v0/bin/clang builder 28753 28449 0 06:12 ? 00:00:00 /bin/sh -c /home/builder/.termux-build/_cache/android-r23c-api-24-v0/bin/clang builder 28754 28753 0 06:12 ? 00:00:00 /home/builder/.termux-build/_cache/android-r23c-api-24-v0/bin/clang builder 28755 28449 0 06:12 ? 00:00:00 ninja -w dupbuild=warn -j 8 $ ./scripts/run-docker.sh ./build-package.sh -f libjpeg-turbo &> build.log $ ./scripts/run-docker.sh ./build-package.sh -f libjpeg-turbo Running container 'termux-package-builder' from image 'termux/package-builder'... ERROR: Another build is already running within same environment. ``` --- build-all.sh | 8 ++- build-package.sh | 12 ++-- clean.sh | 6 ++ scripts/run-docker.sh | 10 ++- scripts/utils/docker/docker.sh | 112 +++++++++++++++++++++++++++++++++ 5 files changed, 141 insertions(+), 7 deletions(-) create mode 100644 scripts/utils/docker/docker.sh diff --git a/build-all.sh b/build-all.sh index 78f0a8de18..08ce452353 100755 --- a/build-all.sh +++ b/build-all.sh @@ -3,6 +3,12 @@ set -e -u -o pipefail +TERMUX_SCRIPTDIR=$(cd "$(realpath "$(dirname "$0")")"; pwd) + +# Store pid of current process in a file for docker__run_docker_exec_trap +source "$TERMUX_SCRIPTDIR/scripts/utils/docker/docker.sh"; docker__create_docker_exec_pid_file + + if [ "$(uname -o)" = "Android" ] || [ -e "/system/bin/app_process" ]; then echo "On-device execution of this script is not supported." exit 1 @@ -53,7 +59,7 @@ if [ -e "$BUILDORDER_FILE" ]; then echo "Using existing buildorder file: $BUILDORDER_FILE" else mkdir -p "$BUILDALL_DIR" - ./scripts/buildorder.py > "$BUILDORDER_FILE" + "$TERMUX_SCRIPTDIR/scripts/buildorder.py" > "$BUILDORDER_FILE" fi if [ -e "$BUILDSTATUS_FILE" ]; then echo "Continuing build-all from: $BUILDSTATUS_FILE" diff --git a/build-package.sh b/build-package.sh index 1b6b5d0cac..8951cc3618 100755 --- a/build-package.sh +++ b/build-package.sh @@ -3,6 +3,14 @@ set -e -o pipefail -u +cd "$(realpath "$(dirname "$0")")" +TERMUX_SCRIPTDIR=$(pwd) +export TERMUX_SCRIPTDIR + +# Store pid of current process in a file for docker__run_docker_exec_trap +source "$TERMUX_SCRIPTDIR/scripts/utils/docker/docker.sh"; docker__create_docker_exec_pid_file + + SOURCE_DATE_EPOCH=$(git log -1 --pretty=%ct 2>/dev/null || date "+%s") export SOURCE_DATE_EPOCH @@ -22,10 +30,6 @@ else export TERMUX_ON_DEVICE_BUILD=false fi -cd "$(realpath "$(dirname "$0")")" -TERMUX_SCRIPTDIR=$(pwd) -export TERMUX_SCRIPTDIR - # Automatically enable offline set of sources and build tools. # Offline termux-packages bundle can be created by executing # script ./scripts/setup-offline-bundle.sh. diff --git a/clean.sh b/clean.sh index 0a6047a062..a1d03a2da5 100755 --- a/clean.sh +++ b/clean.sh @@ -2,6 +2,12 @@ # clean.sh - clean everything. set -e -u +TERMUX_SCRIPTDIR=$(cd "$(realpath "$(dirname "$0")")"; pwd) + +# Store pid of current process in a file for docker__run_docker_exec_trap +. "$TERMUX_SCRIPTDIR/scripts/utils/docker/docker.sh"; docker__create_docker_exec_pid_file + + # Checking if script is running on Android with 2 different methods. # Needed for safety to prevent execution of potentially dangerous # operations such as 'rm -rf /data/*' on Android device. diff --git a/scripts/run-docker.sh b/scripts/run-docker.sh index d85e0c999d..057e3bec99 100755 --- a/scripts/run-docker.sh +++ b/scripts/run-docker.sh @@ -1,6 +1,8 @@ #!/bin/sh set -e -u +TERMUX_SCRIPTDIR=$(cd "$(realpath "$(dirname "$0")")"; cd ..; pwd) + CONTAINER_HOME_DIR=/home/builder UNAME=$(uname) if [ "$UNAME" = Darwin ]; then @@ -45,6 +47,7 @@ $SUDO docker start $CONTAINER_NAME >/dev/null 2>&1 || { echo "Creating new container..." $SUDO docker run \ --detach \ + --init \ --name $CONTAINER_NAME \ --volume $VOLUME \ $SEC_OPT \ @@ -61,8 +64,11 @@ $SUDO docker start $CONTAINER_NAME >/dev/null 2>&1 || { fi } +# Set traps to ensure that the process started with docker exec and all its children are killed. +. "$TERMUX_SCRIPTDIR/scripts/utils/docker/docker.sh"; docker__setup_docker_exec_traps + if [ "$#" -eq "0" ]; then - $SUDO docker exec --interactive $DOCKER_TTY $CONTAINER_NAME bash + $SUDO docker exec --env "DOCKER_EXEC_PID_FILE_PATH=$DOCKER_EXEC_PID_FILE_PATH" --interactive $DOCKER_TTY $CONTAINER_NAME bash else - $SUDO docker exec --interactive $DOCKER_TTY $CONTAINER_NAME "$@" + $SUDO docker exec --env "DOCKER_EXEC_PID_FILE_PATH=$DOCKER_EXEC_PID_FILE_PATH" --interactive $DOCKER_TTY $CONTAINER_NAME "$@" fi diff --git a/scripts/utils/docker/docker.sh b/scripts/utils/docker/docker.sh new file mode 100644 index 0000000000..8fd0851b30 --- /dev/null +++ b/scripts/utils/docker/docker.sh @@ -0,0 +1,112 @@ +# shellcheck shell=sh +# shellcheck disable=SC2039,SC2059 + +# Title: docker +# Description: A library for docker. +# License-SPDX: MIT + +## +# Run trap for `docker exec`. +# . +# This will read the pid stored in DOCKER_EXEC_PID_FILE_PATH by the process +# called by `docker exec`, like via a call to `docker__create_docker_exec_pid_file` +# and then will send kill signals to the process for the pid and all +# its children. This is need like for cases when `--tty` is not passed +# or cannot be passed to `docker exec` like if stdin is not available +# or if output of command is redirected to file instead of terminal, +# like `docker exec cmd &> cmd.log /dev/null || : +} + +# Read the pid stored in file path and send kill signals to the process and all its children +DOCKER_PID="$(cat '"$DOCKER_EXEC_PID_FILE_PATH"')" && [[ "$DOCKER_PID" =~ ^[0-9]+$ ]] && \ +DOCKER_PROCESS="$(ps -efww --pid "$DOCKER_PID" --no-headers -o pid:1,cmd)" && \ +echo "Docker trap killing $DOCKER_PROCESS" && \ +docker_killtree "'"$signal"'" "$DOCKER_PID" + ' + # Exec docker_exec_trap_command inside docker context + $SUDO docker exec "$CONTAINER_NAME" bash -c "$docker_exec_trap_command" + + fi + + exit $exit_code # Exit with the original trap signal exit code + +} + +## +# Setup traps for `docker exec`. +# +# .This call sets `DOCKER_EXEC_PID_FILE_PATH`, which should be passed to +# processes started with `docker exec`, like with +# `--env "DOCKER_EXEC_PID_FILE_PATH=$DOCKER_EXEC_PID_FILE_PATH"` option, +# so that its available in `docker__create_docker_exec_pid_file` inside the +# called process and in `docker__run_docker_exec_trap` inside the process +# that calls `docker exec`. +# . +# . +# docker__setup_docker_exec_traps +## +docker__setup_docker_exec_traps() { + + DOCKER_EXEC_PID_FILE_PATH="/tmp/docker-exec-pid-$(date +"%Y-%m-%d-%H.%M.%S.")$((RANDOM%1000))" + + trap 'docker__run_docker_exec_trap' EXIT + trap 'docker__run_docker_exec_trap TERM' TERM + trap 'docker__run_docker_exec_trap INT' INT + trap 'docker__run_docker_exec_trap HUP' HUP + trap 'docker__run_docker_exec_trap QUIT' QUIT + +} + +## +# Store the pid of current process at DOCKER_EXEC_PID_FILE_PATH so that +# if `docker exec` is killed, like with `ctrl+c`, then all child +# process of current processes are killed by docker__run_docker_exec_trap. +# . +# . +# **Parameters:** +# `pid` - The optional pid to store in file instead of current process pid ($$). +# . +# . +# docker__create_docker_exec_pid_file [`pid`] +## +docker__create_docker_exec_pid_file() { + + local pid=${1-}; pid=${pid:=$$} + + if [ -n "${DOCKER_EXEC_PID_FILE_PATH-}" ] && [ ! -e "${DOCKER_EXEC_PID_FILE_PATH-}" ]; then + if ! echo "$$" > "$DOCKER_EXEC_PID_FILE_PATH"; then + echo "Failed to create docker exec pid file at \"$DOCKER_EXEC_PID_FILE_PATH\"" + fi + fi + +}