scripts+bwrap: Make script execution cancellable

Prep for implementing `rpm-ostree cancel`, but this works with the way we handle
`Ctrl-C` interactively on a client as well. Being able to cancel a script
execution is quite nice; some of them are expensive, and having one loop forever
has been known to happen.

Closes: #1025
Approved by: jlebon

src/libpriv/rpmostree-bwrap.c

@@ -294,44 +294,62 @@ rpmostree_bwrap_set_child_setup (RpmOstreeBwrap *bwrap,
   bwrap->child_setup_data = data;
 }
 
+/* Execute @bwrap - must have been configured. After executing this method, the
+ * @bwrap instance cannot be run again.
+ */
 gboolean
 rpmostree_bwrap_run (RpmOstreeBwrap *bwrap,
+                     GCancellable   *cancellable,
                      GError        **error)
 {
-  int estatus;
-  const char *current_lang = getenv ("LANG");
-
   g_assert (!bwrap->executed);
   bwrap->executed = TRUE;
 
+  const char *current_lang = getenv ("LANG");
   if (!current_lang)
     current_lang = "C";
 
-  {
+  const char *lang_var = glnx_strjoina ("LANG=", current_lang);
-    const char *lang_var = glnx_strjoina ("LANG=", current_lang);
+  /* This is similar to what systemd does, except:
-    /* This is similar to what systemd does, except:
+   *  - We drop /usr/local, since scripts shouldn't see it.
-     *  - We drop /usr/local, since scripts shouldn't see it.
+   *  - We pull in the current process' LANG, since that's what people
-     *  - We pull in the current process' LANG, since that's what people
+   *    have historically expected from RPM scripts.
-     *    have historically expected from RPM scripts.
+   */
-     */
+  const char *bwrap_env[] = {"PATH=/usr/sbin:/usr/bin", lang_var, NULL};
-    const char *bwrap_env[] = {"PATH=/usr/sbin:/usr/bin",
-                               lang_var,
-                               NULL};
 
-    /* Add the final NULL */
+  /* Set up our error message */
-    g_ptr_array_add (bwrap->argv, NULL);
+  const char *errmsg = glnx_strjoina ("Executing bwrap(", bwrap->child_argv0, ")");
+  GLNX_AUTO_PREFIX_ERROR (errmsg, error);
 
-    const char *errmsg = glnx_strjoina ("Executing bwrap(", bwrap->child_argv0, ")");
+  /* Add the final NULL */
-    GLNX_AUTO_PREFIX_ERROR (errmsg, error);
+  g_ptr_array_add (bwrap->argv, NULL);
 
-    if (!g_spawn_sync (NULL, (char**)bwrap->argv->pdata, (char**) bwrap_env,
+  g_autoptr(GSubprocessLauncher) launcher =
-                       G_SPAWN_SEARCH_PATH, bwrap_child_setup, bwrap, NULL, NULL,
+    g_subprocess_launcher_new (G_SUBPROCESS_FLAGS_NONE);
-                       &estatus, error))
+  g_subprocess_launcher_set_environ (launcher, (char**)bwrap_env);
+  g_subprocess_launcher_set_child_setup (launcher, bwrap_child_setup, bwrap, NULL);
+  g_autoptr(GSubprocess) subproc =
+    g_subprocess_launcher_spawnv (launcher, (const char *const*)bwrap->argv->pdata,
+                                  error);
+  if (!subproc)
+    return FALSE;
+  if (!g_subprocess_wait (subproc, cancellable, error))
+    {
+      /* Now, it's possible @cancellable has been set, which means the process
+       * hasn't terminated yet. AFAIK that should be the only cause for the
+       * process not having exited now, but we just kill the process regardless
+       * on error here.  The GSubprocess code ignores the request if we've
+       * already reaped it.
+       *
+       * Right now we run bwrap --die-with-parent, but until we do the whole txn
+       * as a subprocess, the script would leak until rpm-ostreed exited.
+       */
+      g_subprocess_force_exit (subproc);
       return FALSE;
-
+    }
-    if (!g_spawn_check_exit_status (estatus, error))
+  int estatus = g_subprocess_get_exit_status (subproc);
-      return FALSE;
+  if (!g_spawn_check_exit_status (estatus, error))
-  }
+    return FALSE;
 
   return TRUE;
 }
@@ -352,7 +370,7 @@ rpmostree_bwrap_selftest (GError **error)
 
   rpmostree_bwrap_append_child_argv (bwrap, "true", NULL);
 
-  if (!rpmostree_bwrap_run (bwrap, error))
+  if (!rpmostree_bwrap_run (bwrap, NULL, error))
     {
       g_prefix_error (error, "bwrap test failed, see <https://github.com/projectatomic/rpm-ostree/pull/429>: ");
       return FALSE;

src/libpriv/rpmostree-bwrap.h

@@ -51,6 +51,7 @@ void rpmostree_bwrap_set_child_setup (RpmOstreeBwrap *bwrap,
                                       gpointer             data);
 
 gboolean rpmostree_bwrap_run (RpmOstreeBwrap *bwrap,
+                              GCancellable   *cancellable,
                               GError        **error);
 
 gboolean rpmostree_bwrap_selftest (GError **error);

src/libpriv/rpmostree-kernel.c

@@ -456,7 +456,7 @@ rpmostree_run_dracut (int     rootfs_dfd,
 
   rpmostree_bwrap_set_child_setup (bwrap, dracut_child_setup, GINT_TO_POINTER (tmpf.fd));
 
-  if (!rpmostree_bwrap_run (bwrap, error))
+  if (!rpmostree_bwrap_run (bwrap, cancellable, error))
     goto out;
 
   if (rebuild_from_initramfs)

src/libpriv/rpmostree-postprocess.c

@@ -57,7 +57,8 @@ static gboolean
 run_bwrap_mutably (int           rootfs_fd,
                    const char   *binpath,
                    char        **child_argv,
-                   GError     **error)
+                   GCancellable *cancellable,
+                   GError      **error)
 {
   struct stat stbuf;
   const char *etc_bind;
@@ -93,7 +94,7 @@ run_bwrap_mutably (int           rootfs_fd,
       }
   }
 
-  if (!rpmostree_bwrap_run (bwrap, error))
+  if (!rpmostree_bwrap_run (bwrap, cancellable, error))
     return FALSE;
 
   return TRUE;
@@ -324,7 +325,7 @@ process_kernel_and_initramfs (int            rootfs_dfd,
    */
   {
     char *child_argv[] = { "depmod", (char*)kver, NULL };
-    if (!run_bwrap_mutably (rootfs_dfd, "depmod", child_argv, error))
+    if (!run_bwrap_mutably (rootfs_dfd, "depmod", child_argv, cancellable, error))
       return FALSE;
   }
 
@@ -1605,7 +1606,7 @@ rpmostree_treefile_postprocessing (int            rootfs_fd,
 
       {
         char *child_argv[] = { binpath, NULL };
-        if (!run_bwrap_mutably (rootfs_fd, binpath, child_argv, error))
+        if (!run_bwrap_mutably (rootfs_fd, binpath, child_argv, cancellable, error))
           return glnx_prefix_error (error, "While executing postprocessing script '%s'", bn);
       }
 

src/libpriv/rpmostree-scripts.c

@@ -313,7 +313,7 @@ run_script_in_bwrap_container (int rootfs_fd,
                                      script_arg,
                                      NULL);
 
-  if (!rpmostree_bwrap_run (bwrap, error))
+  if (!rpmostree_bwrap_run (bwrap, cancellable, error))
     {
       if (error)
         {
@@ -819,7 +819,7 @@ rpmostree_deployment_sanitycheck (int           rootfs_fd,
   if (!bwrap)
     return FALSE;
   rpmostree_bwrap_append_child_argv (bwrap, "/usr/bin/true", NULL);
-  if (!rpmostree_bwrap_run (bwrap, error))
+  if (!rpmostree_bwrap_run (bwrap, cancellable, error))
     return FALSE;
   sd_journal_print (LOG_INFO, "sanitycheck(/usr/bin/true) successful");
   return TRUE;

tests/common/libvm.sh

@@ -345,6 +345,30 @@ vm_get_journal_cursor() {
   vm_cmd journalctl -o json -n 1 | jq -r '.["__CURSOR"]'
 }
 
+# Wait for a message logged after $cursor matching a regexp to appear
+vm_wait_content_after_cursor() {
+    from_cursor=$1; shift
+    regex=$1; shift
+    cat > wait.sh <<EOF
+#!/usr/bin/bash
+set -xeuo pipefail
+tmpf=\$(mktemp /var/tmp/journal.XXXXXX)
+for x in \$(seq 60); do
+  journalctl -u rpm-ostreed --after-cursor "${from_cursor}" > \${tmpf}
+  if grep -q -e "${regex}" \${tmpf}; then
+    exit 0
+  else
+    cat \${tmpf}
+    sleep 1
+  fi
+done
+echo "timed out after 60s" 1>&2
+journalctl -u rpm-ostreed --after-cursor "${from_cursor}" | tail -100
+exit 1
+EOF
+    vm_cmdfile wait.sh
+}
+
 vm_assert_journal_has_content() {
   from_cursor=$1; shift
   # add an extra helping of quotes for hungry ssh

tests/vmcheck/test-layering-scripts.sh

@@ -159,6 +159,33 @@ if vm_rpmostree install rofiles-violation; then
     assert_not_reached "installed test-post-rofiles-violation!"
 fi
 
+# Test cancellation via having a script hang
+cursor=$(vm_get_journal_cursor)
+vm_build_rpm post-that-hangs \
+             post "echo entering post-that-hangs-infloop 1>&2; while true; do sleep 1h; done"
+# Enable job control so we can do a background job, then foreground.
+set -m
+nohup $SSH -t -t rpm-ostree install post-that-hangs &
+if ! vm_wait_content_after_cursor "${cursor}" "entering post-that-hangs-infloop"; then
+    kill -s INT %1
+    fg %1 || true
+    exit 1
+fi
+# Explicitly kill the client process in the VM; I originally tried to kill the
+# ssh binary and have it propagate the signal, on our side but couldn't figure
+# out how to make that work.
+vm_cmd pkill --signal INT -f "'rpm-ostree install post-that-hangs'"
+# But do also kill the ssh binary on our side
+kill -s INT %1
+# And wait for that to complete so it doesn't hang the shell
+fg %1 || true
+set +m
+# Wait for our expected result
+vm_wait_content_after_cursor "${cursor}" "Txn.*failed.*Running %post for post-that-hangs"
+# Forcibly restart now to avoid any races with the txn finally exiting
+vm_cmd systemctl restart rpm-ostreed
+
+# Test rm -rf /!
 vm_cmd 'useradd testuser || true'
 vm_cmd touch /home/testuser/somedata /tmp/sometmpfile /var/tmp/sometmpfile
 vm_build_rpm rmrf post "rm --no-preserve-root -rf / &>/dev/null || true"