Aliasing "exec" and "builtin" causes various problems. Can you turn it off by default?

[akinomyoga]

I initially received a related issue at akinomyoga/ble.sh#93. This finally turned out to be related to the following settings in gitstatus.plugin.sh:

    alias exec=_gitstatus_exec_wrapper
    alias builtin=_gitstatus_builtin_wrapper

These cause every kind of problem. In particular, the builtin builtin is intended to protect the scripts from such modified builtin behavior so isn't expected to be replaced by an alias or a function.

• The persistent redirections, such as exec 3> file, don't work anymore because it is replaced by _gitstatus_exec_wrapper 3> file and the real exec is executed inside _gitstatus_exec_wrapper. In this case, the redirection 3> file will not be treated by the special rule for exec, so the effect of the redirection is not persistent after the execution of the command.
• builtin unset -v var becomes to remove the local variables instead of changing the variable states to unset. In Bash, the behavior of unset changes depending on the way how unset could find the variable: When unset finds the variable in local scope, unset sets the variable state to unset. When unset finds the variable by dynamic scoping, unset removes the variable placeholder in the function call chain. As a result, the upper level of the variable placeholder becomes visible. If one replaces builtin with a shell function, the variables become always found through the dynamic scoping, so the behavior becomes different.
• The meaning of builtin eval 'echo $1' will change. The positional parameters $1, $2, ... in eval arguments are supposed to reference the values in the context of builtin eval 'echo $1'. However, if one replaces builtin with a function, these parameters start to reference the arguments specified to _gitstatus_builtin_wrapper.
  • Single argument source i.e., builtin source file.sh is also affected. Inside the sourced file file.sh, the parameters $1, etc. of the caller context should be able to be referenced. If builtin is replaced, the parameters $1, etc. are changed to the arguments specified to the wrapper function.
  • the same problems occur with many other builtins: builtin printf -v 'a[$1]', builtin unset -v 'a[$1]', builtin test -v 'a[$1]', v='x=a[$1]'; builtin let v, declare 'a=([$1]=1)', read 'a[$1]', etc.

As real problems:

• My shell setting ble.sh was completely broken by the replaced builtin [gitstatus] stty: invalid argument on source ble.sh, patch 0.3.3 akinomyoga/ble.sh#93 (Now I have added a workaround to protect ble.sh from aliased builtin, so now it doesn't break)
• In the same thread, I received a report on a similar problem by gitstatus.plugin.sh: sudo docker exec fails.

To borrow @romkatv's words, "It's generally not a good idea to redefine builtins." In particular, redefining the builtin builtin is the most one that we want to avoid. Isn't there any other way to detect exec other_program?

• For example, we may constantly test if the parent shell is still alive or not from the daemon process by using kill -0 $$. In this way, one cannot detect if the parent process is replaced by another program using exec, but better than doing nothing.
• For example, if we have procfs, we could check /proc/$$/cmdline to see if the process is replaced or not.

Anyway, I believe we should by default turn off GITSTATUS_STOP_ON_EXEC.

[romkatv]

Thanks for sending the PR. I agree that redefining exec and builtin are horrible hacks.

If exec is not redefined, restarting bash via exec bash will cause a resource leak. That seems pretty bad. Is there a way to avoid it?

In zsh I solve this problem by applying O_CLOEXEC to the file descriptors, so when the current shell is replaced by something else via exec, the file descriptors are closed, which causes the background gitstatusd process to terminate.

[akinomyoga]

If exec is not redefined, restarting bash via exec bash will cause a resource leak. That seems pretty bad. Is there a way to avoid it?

Option 1. Detect exec bash using exported variables

restarting bash via exec bash will cause a resource leak.

In the case of exec bash, I feel like we can terminate the daemon (or reuse the daemon) in the reloaded gitstatus.plugin.sh because ~/.bashrc will be loaded again by the new startup of Bash process. If we mark the related variables with the export attribute, these variables will be inherited to the new session of Bash started by exec. For example, I haven't tested it, but something like

diff --git a/gitstatus.plugin.sh b/gitstatus.plugin.sh
index 4d7d4e1..dd7251f 100644
--- a/gitstatus.plugin.sh
+++ b/gitstatus.plugin.sh
@@ -1,5 +1,12 @@
 # Bash bindings for gitstatus.

+if [[ $_GITSTATUS_BASH_PID ]]; then
+  if [[ $_GITSTATUS_BASH_PID == $$ ]]; then
+    exec {_GITSTATUS_REQ_FD}>&- {_GITSTATUS_RESP_FD}<&-
+  fi
+  unset _GITSTATUS_REQ_FD _GITSTATUS_RESP_FD _GITSTATUS_BASH_PID
+fi
+
 [[ $- == *i* ]] || return  # non-interactive shell

 # Starts gitstatusd in the background. Does nothing and succeeds if gitstatusd
@@ -209,6 +216,7 @@ function gitstatus_start() {
     } 0"$GITSTATUS_DAEMON_LOG"

     exec {_GITSTATUS_REQ_FD}>>"$req_fifo" {_GITSTATUS_RESP_FD}<"$resp_fifo"   || return
+    export _GITSTATUS_REQ_FD _GITSTATUS_RESP_FD _GITSTATUS_BASH_PID=$$
     command rm -f -- "$req_fifo" "$resp_fifo"                                 || return
     [[ "$GITSTATUS_DAEMON_LOG" != /dev/null ]] || command rmdir -- "$tmpdir" 2>/dev/null

In zsh I solve this problem by applying O_CLOEXEC to the file descriptors,

Ah, that is a clever trick!

Option 2. Set FD_CLOEXEC using Bash loadable builtin fdflags

It seems Bash provides a loadable builtin named fdflags for that purpose, but unfortunately, it is usually not distributed with Bash itself. When one builds Bash by oneself, one would get the loadable builtin in e.g. /usr/local/lib/bash/fdflags. Ubuntu seems to provide a separate package named bash-builtins in which /usr/lib/bash/fdflags is contained. This approach is only available when the loadable builtin fdflags exists, but if it exists, we can do

enable -f /usr/lib/bash/fdflags fdflags
fdflags -s +cloexec "$_GITSTATUS_REQ_FD" "$_GITSTATUS_RESP_FD"

[romkatv]

In the case of exec bash, I feel like we can terminate the daemon (or reuse the daemon) in the reloaded gitstatus.plugin.sh because ~/.bashrc will be loaded again by the new startup of Bash process.

Good idea! This will still cause a leak in case of exec not-bash or exec sudo bash but it's probably good enough. I'll make this change and try it out for myself to verify that everything works as expected.

[akinomyoga]

Oh, I was actually thinking of another approach. I think currently the cleanest way is to use Option 4 below. (Edit: I think it is better to combine it with the above-mentioned startup check for exec bash).

Option 3. Set FD_CLOEXEC using gdb (requires gdb and a C compiler that supports the option -E)

Another way is to use gdb or some debugger API to attach the Bash process and set FD_CLOEXEC to the corresponding file descriptors. We need gcc and related headers unistd.h and fcntl.h to extract the value of F_SETFD and FD_CLOEXEC.

Edit: modified the code

read F_SETFD FD_CLOEXEC <<< $(
  printf '%s\n' '#include ' '#include ' 'F_SETFD FD_CLOEXEC' |
  gcc -E -P - | tail -1)
gdb -batch \
 -ex "attach $$" \
 -ex "p (int) fcntl($_GITSTATUS_REQ_FD, $F_SETFD, $FD_CLOEXEC)" \
 -ex "p (int) fcntl($_GITSTATUS_RESP_FD, $F_SETFD, $FD_CLOEXEC)" \
 -ex "detach"

Option 4. Set FD_CLOEXEC using our own loadable builtin (requires a C compiler capable of creating shared object):

I noticed that this project is licensed by GPL v3. Then, we can actually build our own loadable builtin that adds FD_CLOEXEC.to the specified file descriptors. One of the limitations of Bash loadable builtins is that they need to be licensed by GPL v3 because Bash loadable builtins are supposed to be dynamically linked with Bash image (which is GPL v3). But, in our case, gitstatus is already GPL v3 so we don't have to worry about it.

#include 
#include 
#include 

/* ---- Common definitions ------------------------------------------------- */

#define BUILTIN_ENABLED 0x01
typedef struct word_desc { char *word; int flags; } WORD_DESC;
typedef struct word_list { struct word_list *next; WORD_DESC *word; } WORD_LIST;
typedef int sh_builtin_func_t(WORD_LIST *);
struct builtin {
  const char *name;
  sh_builtin_func_t *function;
  int flags;
  const char **long_doc;
  const char *short_doc;
  char *handle;
};

/* ---- _gitstatus_set_cloexec builtin ------------------------------------- */

static int _gitstatus_set_cloexec_builtin(WORD_LIST *list) {
  int fd, error;
  error = 0;
  while (list) {
    fd = atoi(list->word->word);
    if (fd >= 0 && fcntl(fd, F_SETFD, FD_CLOEXEC) != 0)
      error = 1;
    list = list->next;
  }
  return error;
}
static const char* _gitstatus_set_cloexec_doc[] = {
  "set FD_CLOEXEC", " ",
  "This is a builtin used by gitstatus.",
  (const char*) NULL
};
struct builtin _gitstatus_set_cloexec_struct = {
  "_gitstatus_set_cloexec",
  _gitstatus_set_cloexec_builtin,
  BUILTIN_ENABLED,
  _gitstatus_set_cloexec_doc,
  "_gitstatus_set_cloexec FD",
  0,
};

Usage example,

$ gcc -shared -o _gitstatus_set_cloexec _gitstatus_set_cloexec.c
$ enable -f ./_gitstatus_set_cloexec _gitstatus_set_cloexec
$ _gitstatus_set_cloexec "$_GITSTATUS_REQ_FD" "$_GITSTATUS_RESP_FD"

[romkatv]

Option 3. Set FD_CLOEXEC using gdb (requires gdb and a C compiler that supports the option -E)

Huh! I've actually implemented this within gitstatusd at some point. It was using syscall injection via ptrace to invoke fcntl within bash. This worked only on linux-x64 and was too hacky for my taste.

I really don't want a dependency on gdb of a C compiler.

[romkatv]

In the case of exec bash, I feel like we can terminate the daemon (or reuse the daemon) in the reloaded gitstatus.plugin.sh because ~/.bashrc will be loaded again by the new startup of Bash process.

Good idea! This will still cause a leak in case of exec not-bash or exec sudo bash but it's probably good enough. I'll make this change and try it out for myself to verify that everything works as expected.

Implemented here: 815301f

[akinomyoga]

Huh! I've actually implemented this within gitstatusd at some point. It was using syscall injection via ptrace to invoke fcntl within bash. This worked only on linux-x64 and was too hacky for my taste.

Oh, OK. Yeah, I understand that's too hacky.

I really don't want a dependency on gdb of a C compiler.

Actually, I just thought about it as one option in case there is really no other way to solve the issue. In that case, maybe we could combine different ways and choose one available in that system if any. But now, it's fine to forget about this approach because it seems like you have found a satisfactory way for you.

Implemented here: 815301f

OK. Thank you for your quick work! If it works fine within your environment, it is fine for me to close the PR now. Thank you very much!

[romkatv]

OK. Thank you for your quick work!

Thanks for filing the PR and suggesting viable fixes.

If it works fine within your environment, it is fine for me to close the PR now.

Yeah, feel free to close the PR.

It works fine in my tests. I don't use bash myself, so there might be corner cases that I've missed. I'll wait for a week or two for possible fallout from bash users who follow master. If no bugs get reported I'll cut a new release.

[akinomyoga]

OK. I'll close the issue. Thank you again!

[romkatv]

FYI: This change has been released in v1.5.0.