From 46c7d448f27e8fc213c50cd241914b2d94e2e308 Mon Sep 17 00:00:00 2001 From: Karen Arutyunov Date: Mon, 10 Aug 2020 20:47:11 +0300 Subject: Fix bash coprocess usage races --- libbutl/manifest-parser.bash.in | 20 +++++++++++++++++++- libbutl/manifest-serializer.bash.in | 7 ++++++- libbutl/utility.bash.in | 25 +++++++++++++++++++++++++ 3 files changed, 50 insertions(+), 2 deletions(-) diff --git a/libbutl/manifest-parser.bash.in b/libbutl/manifest-parser.bash.in index 5b38eeb..df06138 100644 --- a/libbutl/manifest-parser.bash.in +++ b/libbutl/manifest-parser.bash.in @@ -67,9 +67,27 @@ function butl_manifest_parser_start () # [] # multiple coprocesses at a time (see the BUGS section of bash(1) man page # for details). # - coproc { butl_parse_manifest; } <&"$butl_manifest_parser_ifd" + # An update: it turns out that we still can end up with an unset COPROC if + # the process finishes too early. To avoid that we suspend the subshell + # before executing the parser process and resume it after querying the + # COPROC value. Note that we need to be careful not to attempt to resume the + # process that hasn't suspended itself (see butl_resume_process() for + # details). + # + # Note that the suspend builtin doesn't work in subshells by default since + # there is no job control enabled for them. Also when it is enabled (via + # `set -m`), the builtin stops subshells recursively up to the command being + # run from the interactive shell, which is not what we want. That's why we + # use kill which is also a builtin and thus presumably is not slower than + # suspend. + # + coproc { kill -SIGSTOP $BASHPID; exec "$(butl_path)/manifest" parse; } \ + <&"$butl_manifest_parser_ifd" + exec {butl_manifest_parser_ofd}<&"${COPROC[0]}" butl_manifest_parser_pid="$COPROC_PID" + + butl_resume_process "$butl_manifest_parser_pid" } # Finish the manifest parsing co-process. diff --git a/libbutl/manifest-serializer.bash.in b/libbutl/manifest-serializer.bash.in index fa6b94a..ce99779 100644 --- a/libbutl/manifest-serializer.bash.in +++ b/libbutl/manifest-serializer.bash.in @@ -64,9 +64,14 @@ function butl_manifest_serializer_start () # [--long-lines] [] # See notes in butl_manifest_parser_start() on bash co-process issues. # - coproc { butl_serialize_manifest "${ops[@]}"; } >&"$butl_manifest_serializer_ofd" + coproc { kill -SIGSTOP $BASHPID; \ + exec "$(butl_path)/manifest" "${ops[@]}" serialize; } \ + >&"$butl_manifest_serializer_ofd" + butl_manifest_serializer_ifd="${COPROC[1]}" butl_manifest_serializer_pid="$COPROC_PID" + + butl_resume_process "$butl_manifest_serializer_pid" } # Finish the manifest serialization co-process. diff --git a/libbutl/utility.bash.in b/libbutl/utility.bash.in index 56bd3ab..5f51a2c 100644 --- a/libbutl/utility.bash.in +++ b/libbutl/utility.bash.in @@ -23,3 +23,28 @@ function butl_path () # dirname "${BASH_SOURCE[0]}" } + +# Resume a stopped process execution by sending it the SIGCONT signal. +# +# Note that to avoid races the function waits until the process enters the +# stopped state. +# +function butl_resume_process () # +{ + local pid="$1" + + while true; do + + # Note that while the ps's -o option 'state' value is not specified by + # POSIX, it is supported by all the major implementations with the leading + # 'T' character indicating the 'stopped' process run state. + # + local s + s="$(ps -p "$pid" -o state=)" + + if [ "${s:0:1}" == "T" ]; then + kill -SIGCONT "$pid" + break + fi + done +} -- cgit v1.1