Use PS0 to start observing a command instead of the strcpy-hack.

Previously we checked bash's current_command_number (loaded via dlsym)
after every strcpy library-call in order to know when bash started
a new command execution. This has proven unreliable for bash 5.1.8 on
openSUSE Tumbleweed. When a subshell (echo forked; echo hi > foo)
was executed, the first strcpy did not fire yet.
Therefore we switched to PS0 calling «fork» where we start the
observation. Doing so we got rid of the dlsym-call altogether.
Further we make extra-sure, to not start tracking from a subshell,
which occurred in the above mentioned bash-OS-combo.

shell-integration-scripts/integration_fan.sh

@@ -285,12 +285,14 @@ _shournal_trigger_update(){
     local ret
 
     export _LIBSHOURNAL_TRIGGER="$desired_state";
+    export _SHOURNAL_SHELL_PID=$$
 
     # Note: our .so detects this special (non-)filename and
     # writes its response to an unnamed tmp file.
     ret=0
     read -d '' trigger_response < '_///shournal_trigger_response///_' || ret=$?
     unset _LIBSHOURNAL_TRIGGER
+    unset _SHOURNAL_SHELL_PID
 
     if [ $ret -ne 0 ]; then
         _shournal_debug "_shournal_trigger_update: failed to read " \
@@ -356,15 +358,25 @@ _libshournal_is_loaded(){
 # _shournal_verbose_reexec_allowed
 case "$_SHOURNAL_SHELL_NAME" in
   'bash')
+export _LIBSHOURNAL_SEQ_COUNTER=1
+_shournal_ps0='${_SHOURNAL_SHELL_NAME:((_LIBSHOURNAL_SEQ_COUNTER++)):0}$(:)'
 
 _shournal_add_prompts(){
+    [ -z "${PS0+x}" ] && PS0=''
     [ -z "${PROMPT_COMMAND+x}" ] && PROMPT_COMMAND=''
+
+    # Allright, what happens here? We use _SHOURNAL_SHELL_NAME as a dummy
+    # variable in order to increment _LIBSHOURNAL_SEQ_COUNTER without printing
+    # anything. Then we fork to notify libshournal-shellwatch.so that
+    # we're about to execute a command.
+    PS0="$PS0""$_shournal_ps0"
     PROMPT_COMMAND=$'_shournal_postexec\n'"$PROMPT_COMMAND"
     # no _shournal_preexec for bash, see below ...
     return 0
 }
 
 _shournal_remove_prompts(){
+    [ -n "${PS0+x}" ] && PS0=${PS0//"$_shournal_ps0"/}
     [ -n "${PROMPT_COMMAND+x}" ] &&
         PROMPT_COMMAND=${PROMPT_COMMAND//_shournal_postexec$'\n'/}
     return 0
@@ -388,9 +400,7 @@ _shournal_verbose_reexec_allowed(){
 
 # _shournal_preexec(){
 #   For bash preexec is not implemented here but in
-#   libshournal-shellwatch.so, because the DEBUG trap is
-#   somewhat unreliable and PS0 can only run commands in
-#   a subshell which would require additional care (more IPC).
+#   in an interplay of above PS0 and libshournal-shellwatch.so.
 # }
 
 _shournal_postexec(){

src/shell-integration-fanotify/CMakeLists.txt

@@ -16,7 +16,6 @@ add_library(libshournal-shellwatch SHARED
     attached_bash
     attached_shell
     event_open
-    event_other
     event_process
     shell_globals
     shell_logger

src/shell-integration-fanotify/attached_bash.cpp

@@ -2,21 +2,39 @@
 #include <QDebug>
 #include <dlfcn.h>
 
+#include "logger.h"
+
 
 #include "attached_bash.h"
 #include "os.h"
 
+static int read_seq(){
+    const char* _LIBSHOURNAL_SEQ_COUNTER = "_LIBSHOURNAL_SEQ_COUNTER";
+    const char* seq_val = getenv(_LIBSHOURNAL_SEQ_COUNTER);
+    if(seq_val == nullptr){
+        logWarning << qtr("Required environment variable '%1' "
+                          "is unset.").arg(_LIBSHOURNAL_SEQ_COUNTER);
+        return -1;
+    }
+    int seq;
+    try {
+        qVariantTo_throw(seq_val, &seq);
+    } catch (const ExcQVariantConvert& ex) {
+        logWarning << "Failed to convert sequnce:"
+                   << ex.descrip();
+        return -1;
+    }
+    return seq;
+}
+
 /// @throws ExcOs
 AttachedBash::AttachedBash() :
-    m_pExternCurrentCmdNumber(
-        reinterpret_cast<int*>(os::dlsym(RTLD_DEFAULT, "current_command_number"))
-        ),
-    m_lastCmdNumber(1) // see bash sourcecode shell.c, current_command_number is initialized to 1
+    m_lastSeq(1)
 {}
 
 void AttachedBash::handleEnable()
 {
-     m_lastCmdNumber = *m_pExternCurrentCmdNumber;
+    m_lastSeq = read_seq();
 }
 
 /// The command is considered valid, if the command-counter
@@ -25,9 +43,13 @@ void AttachedBash::handleEnable()
 /// per command sequence.
 bool AttachedBash::cmdCounterJustIncremented()
 {
-    if(*m_pExternCurrentCmdNumber == m_lastCmdNumber){
+    int current_seq = read_seq();
+    if(current_seq == -1){
+        return false; // error
+    }
+    if(current_seq == m_lastSeq){
         return false;
     }
-    m_lastCmdNumber = *m_pExternCurrentCmdNumber;
+    m_lastSeq = current_seq;
     return true;
 }

src/shell-integration-fanotify/attached_bash.h

@@ -2,13 +2,20 @@
 
 #include "attached_shell.h"
 
-/// I was unable to find a reliable way to find out, whether the
-/// last entered command was empty (invalid) from within the bash_integration.sh.
-/// Especially, when IgnoreDups is on, or the history get reloaded from file
-/// (at each PROMPT_COMMAND). Therefor we fetch the bash-internal counter
-/// 'current_command_number' at runtime, which is only incremented in case of
-/// non-empty commands and is independent of history-reloads.
-/// The symbol-name has been stable for at least a decade as of March 2019.
+/// We read the env-variable set in bash's PS0
+/// in order to prepare the observation of the next command
+/// sequence. We cannot do this easily from within a function
+/// called in PS0, because that is run in a subshell.
+/// Another possibility is to run a signal handler but there we must
+/// again be careful not to interfere with custom handlers of the
+/// user.
+/// counter=0
+/// trap_handler(){
+///     counter=$((counter+1))
+///     echo "hi from trap_handler: $counter: $(history 1)" >&2
+/// }
+/// trap trap_handler SIGRTMIN
+/// PS0='$(echo "sending signal... "; kill -SIGRTMIN  $$; )'
 class AttachedBash : public AttachedShell
 {
 public:
@@ -19,7 +26,6 @@ class AttachedBash : public AttachedShell
     bool cmdCounterJustIncremented() override;
 
 private:
-    int *m_pExternCurrentCmdNumber;
-    int m_lastCmdNumber;
+    int m_lastSeq;
 };
 

src/shell-integration-fanotify/event_open.cpp

@@ -83,11 +83,10 @@ int event_open::handleOpen(const char *pathname, int flags, mode_t mode, bool la
 
     // Note: we only process shell-request if the trigger env-variable is set AND the current
     // pathname is _///shournal_trigger_response///_
-    // So check for the pathname before calling handling the request in
+    // So check for the pathname before handling the request in
     // checkForTriggerAndHandle (this is for cases where the trigger variable is set
     // and other redirections occurr in between).
-    if(! g_shell.inSubshell &&
-       strcmp(pathname, "_///shournal_trigger_response///_") == 0){
+    if(strcmp(pathname, "_///shournal_trigger_response///_") == 0){
         bool shellRequestSuccess = false;
         auto shellRequest = checkForTriggerAndHandle(&shellRequestSuccess);
         switch (shellRequest) {

src/shell-integration-fanotify/event_process.cpp

@@ -5,7 +5,7 @@
 #include <sys/socket.h>
 #include <csignal>
 
-
+#include "attached_bash.h"
 #include "cleanupresource.h"
 #include "logger.h"
 
@@ -57,18 +57,21 @@ pid_t event_process::handleFork()
     }
     auto clearIgnEvents = finally([&g_shell] {g_shell.ignoreEvents.clear(); });
 
-    switch (g_shell.watchState) {
-    case E_WatchState::WITHIN_CMD: {
-        pid_t ret = g_shell.orig_fork();
-        if(ret == 0){
-            g_shell.inSubshell = true;
+    if( g_shell.inParentShell &&
+         g_shell.watchState == E_WatchState::INTERMEDIATE &&
+          dynamic_cast<const AttachedBash*>(g_shell.pAttchedShell) != nullptr &&
+         g_shell.pAttchedShell->cmdCounterJustIncremented()){
+        shell_request_handler::handlePrepareCmd();
+    }
+
+    pid_t ret = g_shell.orig_fork();
+    if(ret == 0){
+        if(g_shell.shellParentPid != 0){
+            // our parent shell is initialized, so we can't be it
+            g_shell.inParentShell = false;
         }
-        return ret;
-    }    
-    default:
-        break;
     }
-    return g_shell.orig_fork();
+    return ret;
 }
 
 
@@ -81,7 +84,7 @@ int event_process::handleExecve(const char *filename, char * const argv[], char
     }
     auto clearIgnEvents = finally([&g_shell] {g_shell.ignoreEvents.clear(); });
 
-    if(! g_shell.inSubshell ||
+    if( g_shell.inParentShell ||
          g_shell.watchState != E_WatchState::WITHIN_CMD){
         shell_earlydbg("ignore execve of %s", filename);
         return g_shell.orig_execve(filename, argv, envp);

src/shell-integration-fanotify/libshournal-shellwatch.cpp

@@ -10,7 +10,6 @@
 
 #include "cleanupresource.h"
 #include "event_open.h"
-#include "event_other.h"
 #include "event_process.h"
 #include "staticinitializer.h"
 #include "shell_globals.h"
@@ -155,17 +154,6 @@ int execve(const char *filename, char *const argv[],
 }
 
 
-LIBSHOURNAL_SHELLWATCH_EXPORT
-char *strcpy(char *dest, const char *src){
-    initSymIfNeeded();
-    try {
-        return event_other::handleStrcpy(dest, src);
-    } catch (const std::exception& ex ) {
-        std::cerr << __func__ << " fatal: " << ex.what() << "\n";
-    }
-    return ShellGlobals::instance().orig_strcpy(dest, src);
-}
-
 #ifdef __cplusplus
 }
 #endif

src/shell-integration-fanotify/shell_globals.h

@@ -45,7 +45,7 @@ class ShellGlobals
     std::atomic_flag ignoreEvents{};
 
     E_WatchState watchState {E_WatchState::DISABLED};
-    bool inSubshell {false};
+    bool inParentShell {false};
     fdcommunication::SocketCommunication shournalSocket;
     pid_t lastMountNamespacePid {-1};
 
@@ -60,6 +60,8 @@ class ShellGlobals
     SessionInfo sessionInfo;
     int shournalRootDirFd {-1};
 
+    pid_t shellParentPid {0};
+
 public:
     ~ShellGlobals() = default;
     Q_DISABLE_COPY(ShellGlobals)

src/shell-integration-fanotify/shell_request_handler.cpp

@@ -179,6 +179,36 @@ static void verboseCloseRootDirFd(){
     g_shell.shournalRootDirFd = -1;
 }
 
+static bool updateShellPID(){
+    const char* _SHOURNAL_SHELL_PID = "_SHOURNAL_SHELL_PID";
+    const char* pidValue = getenv(_SHOURNAL_SHELL_PID);
+    if(pidValue == nullptr){
+        logWarning << qtr("Required environment variable '%1' "
+                          "is unset.").arg(_SHOURNAL_SHELL_PID);
+        return false;
+    }
+    pid_t pid;
+    try {
+        qVariantTo_throw(pidValue, &pid);
+    } catch (const ExcQVariantConvert& ex) {
+        logWarning << "Failed to convert pid:"
+                   << ex.descrip();
+        return false;
+    }
+    auto realPid = getpid();
+    if(pid != realPid){
+        logWarning << qtr("Apparently we were enabled from a subshell, which "
+                          "is not supported.");
+        return false;
+    }
+
+    auto& g_shell = ShellGlobals::instance();
+    g_shell.shellParentPid = pid;
+    g_shell.inParentShell = true;
+
+    return true;
+}
+
 
 static bool handleDisableRequest(){
     auto& g_shell = ShellGlobals::instance();
@@ -493,6 +523,10 @@ ShellRequest shell_request_handler::checkForTriggerAndHandle(bool *success){
             return request;
         }
     }
+    if(! updateShellPID()){
+        return ShellRequest::TRIGGER_MALFORMED;
+    }
+
     switch (request) {
     case ShellRequest::ENABLE:
         *success = handleEnableRequest();