Skip to content

Replace FF_USE_NEW_BASH_EVAL_STRATEGY with fixed strategy

Philipp Hahnrequested to merge
gitlab-community/gitlab-org/gitlab-runner:phahn/close-stdin into main

What does this MR do?

Close STDIN instead of piping from : and deprecate the then no longer needed FF_USE_NEW_BASH_EVAL_STRATEGY

This avoids triggering a 🐛 in bash (4.0-5.3)12, which !2818 (merged) tried to address. The MR introduced FF_USE_NEW_BASH_EVAL_STRATEGY, which still is not enabled by default. As such users will still encounter this problem and waste time finding the real issue, namely a poor decision by GitLab-runner on how it redirects STDIN.

Recap

The runner executes the script: like this, depending on FF_USE_NEW_EVAL_STRATEGY:

$ bash -e -c ": |              eval '(exit 2)' "; echo $?
1
$ bash -e -c ": |            ( eval '(exit 2)')"; echo $?
2
$ bash -e -c "exec </dev/null; eval '(exit 2)' "; echo $?
2
  1. is the old (broken) behavior if FF_USE_NEW_EVAL_STRATEGY is disabled
  2. is the new (fixed) behavior if FF_USE_NEW_EVAL_STRATEGY is enabled
  3. is the new (fixed) behavior bypassing the broken bash behavior

: | eval … creates a pipe between the parent shell running : and the sub-shell executing the eval, so that read(STDIN) returns EOF (instead of the following shell code or returning the errorEBADFD). It is more efficient to re-open STDIN to read from /dev/null, which also returns EOF but saves context switching between those two processes.

Not returning 2 is a bug in bash, which is fixed by 3. Using </dev/null instead of :| avoids the bug in bash as eval … is then always executed by the parent shell.

Change the code to not use a pipe, but open /dev/null for STDIN.

Signed-off-by: Philipp Hahn p.hahn@avm.de
Link: #27668 (closed)
Fixes: 71f1a22f ("Add new eval execution strategy for capturing exit code")
Fixes: 2f89c003 ("Clarify new bash eval strategy FF name")

Why was this MR needed?

FF_USE_NEW_BASH_EVAL_STRATEGY is still not the default and is just a work-around for a bug in bash, which has been fixed recently. As soon as that version becomes widely available, there will be a behavior change anyway. As such fix the real issue, with does work the the current (broken) and future (fixes) bash version.

What's the best way to test this MR?

  • There may by other reasons why the extra (…) around eval … might be needed, but as of today I'm not aware of any legit case.
  • https://docs.gitlab.com/runner/executors/instance/#troubleshooting documents a strange case, where FF_USE_NEW_BASH_EVAL_STRATEGY is advised to fix the problem. Investigating that case more may show, why FF_USE_NEW_BASH_EVAL_STRATEGY is still needed. Sadly 885a0372 does provide more background itself, but references !4475 (comment 1653245031). I cannot look into those tickets myself, but maybe @psureshbabu can provide more background on why that troubleshooting section was added.

What are the relevant issue numbers?

/cc @ayufan /cc @cat /cc @tmaczukin

  1. https://savannah.gnu.org/bugs/index.php?67473 ↩

  2. https://savannah.gnu.org/support/?109840 ↩

  3. https://cgit.git.savannah.gnu.org/cgit/bash.git/diff/execute_cmd.c?h=devel&id=e9053f2a3ae4ed638d269481626e0e5408ae8965 ↩

Edited by Philipp Hahn

Merge request reports