Merge pull request #597 from leocardao/topic/fix-rlimit-ctrl-c

Fix CTRL-C on a program launched with rlimit

tests/tests_e3/os/process/main_test.py

@@ -3,6 +3,7 @@
 import subprocess
 import textwrap
 import time
+import signal
 
 import e3.fs
 import e3.os.fs
@@ -115,6 +116,123 @@ def run_test():
         e.restore()
 
 
+@pytest.mark.skipif(sys.platform == "win32", reason="A linux test")
+def test_rlimit_ctrl_c():
+    """Test rlimit CTRL-C.
+
+    When a parent process launched two or more child processes using rlimit, the CTRL-C
+    command no longer worked.
+
+    This was because when an rlimit process was launched, it became the foreground
+    process.
+
+    However, when the foreground process was killed, it left the parent process without
+    a foreground, so CTRL-C was ignored.
+
+    Examples:
+       Example 1: Spawn 1 rlimit child process:
+            python (Parent process)
+            |
+            -> rlimit (Child process / foreground process)
+
+            A CTRL-C appear:
+                The child process in the foreground was killed, leaving the parent
+                process alone with no foreground process but no child. Due to our
+                usage, this posed no known problems. However, leaving the parent
+                process without a foreground can cause unexpected results.
+
+       Example 2: Spawn 2 rlimit child process:
+            python (Parent process)
+            |
+            -> rlimit (Child process) ==> This process was the foreground process as
+            |                             long as no other rlimit process was running.
+            |                             In this example, we have 2 rlimit processes,
+            |                             so this process is not in the foreground.
+            |
+            -> rlimit (Child process / foreground process)
+
+            A CTRL-C appear:
+                The foreground process has been killed, leaving no foreground process.
+                Signals were no longer propagated, so CTRL-C did nothing.
+    """
+    try:
+        from ptyprocess import PtyProcess
+    except ImportError:
+        raise ImportError("ptyprocess is needed to run this tests") from None
+
+    # Only a linux test
+    assert sys.platform.startswith("linux"), "This test make sens only on linux"
+
+    script_to_run = """
+from __future__ import annotations
+
+import sys
+from e3.os.process import Run
+
+p2 = Run(["sleep", "100"], timeout=30, bg=True)
+p1 = Run(["sleep", "10"], timeout=1)
+# CTRL-C is now blocking
+p2.wait()
+"""
+
+    with open("tmp-test_rlimic_ctrl_c.py", "w") as f:
+        f.write(script_to_run)
+
+    start = time.perf_counter()
+    p = PtyProcess.spawn([sys.executable, "tmp-test_rlimic_ctrl_c.py"])
+    time.sleep(5)
+    p.sendintr()
+    # !!! Warning:
+    #   if the script_to_run write something on stdout, this will wait forever.
+    p.wait()
+    end = time.perf_counter()
+    assert int(end - start) < 30, f"CTRL-C failed: take {int(end - start)} seconds"
+
+
+@pytest.mark.skipif(sys.platform == "win32", reason="A linux test")
+def test_rlimit_foreground_option():
+    """Test rlimit --foreground.
+
+    Test if we can read/write from an interactive terminal using rlimit --foreground.
+    """
+    try:
+        from ptyprocess import PtyProcess
+    except ImportError:
+        raise ImportError("ptyprocess is needed to run this tests") from None
+
+    # Only a linux test
+    assert sys.platform.startswith("linux"), "This test make sens only on linux"
+
+    # Test with --foreground
+    os.environ["PS1"] = "$ "
+    p = PtyProcess.spawn(
+        [e3.os.process.get_rlimit(), "--foreground", "30", "bash", "--norc", "-i"],
+        env=os.environ,
+        echo=False,
+    )
+    p.write(b"echo 'test rlimit --foreground'\n", flush=True)
+    time.sleep(2)
+    assert p.read() == b"$ test rlimit --foreground\r\n$ "
+    # Ensure that the process is killed
+    p.kill(signal.SIGKILL)
+
+    # Test without foreground (Should failed)
+    p = PtyProcess.spawn(
+        [e3.os.process.get_rlimit(), "30", "bash", "--norc", "-i"],
+        env=os.environ,
+        echo=False,
+    )
+    p.write(b"echo 'test rlimit (no --foreground)'\n", flush=True)
+    time.sleep(2)
+    with pytest.raises(EOFError):
+        # The echo command should not be executed by bash. So p.read() will raise an
+        # EOFError. And if not, we will raise an Exception because this is not
+        # what we want.
+        p.read()
+        # Ensure that the process is killed
+    p.kill(signal.SIGKILL)
+
+
 def test_not_found():
     with pytest.raises(OSError) as err:
         e3.os.process.Run(["e3-bin-not-found"])

tools/rlimit/rlimit.c

@@ -24,11 +24,13 @@
    Usage:
       rlimit seconds command [args]   */
 
+#define _XOPEN_SOURCE 500 /* For setpgid */
 #include <sys/types.h>
 #include <unistd.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <signal.h>
+#include <string.h>
 #include <sys/types.h>
 #include <sys/wait.h>
 #include <errno.h>
@@ -37,11 +39,11 @@ void
 usage (void)
 {
   printf ("Usage:\n");
-  printf ("   rlimit seconds command [args]\n");
+  printf ("   rlimit [--foreground] seconds command [args]\n");
   exit (1);
 }
 
-
+static int foreground = 0;
 static int pid    = 0;
 /* pid of child (controlled) process */
 
@@ -72,6 +74,8 @@ reapchild (int nsig)
 {
   int delay;
 
+  (void)nsig;
+
   if (pid > 0) {
     int rc;
 
@@ -125,16 +129,26 @@ reapchild (int nsig)
 int
 main (int argc, char **argv)
 {
+  int begin;
   sigset_t block_cld;
 
   /* we need at least 3 args */
   if (argc < 3)
     usage ();
 
+  begin = 2;
+
+  if (strcmp(argv[1], "--foreground") == 0) {
+    ++foreground;
+    ++begin;
+  }
+  else if (*argv[1] == '-')
+      usage();
+
   /* argv[0] = .../rlimit
-     argv[1] = seconds
-     argv[2] = command
-     argv[3] = args */
+     argv[begin - 1] = seconds
+     argv[begin] = command
+     argv[begin + 1] = args */
 
   signal (SIGTERM, terminate_group);
   signal (SIGINT, terminate_group);
@@ -179,9 +193,9 @@ main (int argc, char **argv)
       sigprocmask(SIG_UNBLOCK, &block_cld, NULL);
 
       /* child exec the command in a new process group */
-      if (setpgid (0, 0)) {
-        perror ("setpgid");
-        exit (4);
+      if (setpgid (0, 0) == -1) {
+        perror("setpgid");
+        exit(4);
       }
 
       #if defined (__APPLE__)
@@ -202,30 +216,37 @@ main (int argc, char **argv)
       /* ignore SIGTTOU so that tcsetpgrp call does not block. */
       signal (SIGTTOU, SIG_IGN);
 
-      /* Set child process as foreground process group so that
-       * children can read from the terminal if needed. Note that
-       * we ignore any error here because stdin might not be a terminal.
+      /* If run with --foreground process group, rlimit must be able to
+       * read and write from a tty (not only read => STDIN is a tty).
+       *
        * If only stdout is a terminal there are no issues with not
        * being the foreground process as most terminals are configured
        * with TOSTOP off (so SIGTTOU is only emited in case of terminal
        * settings change.
        */
-      tcsetpgrp(0, getpgid(getpid()));
+      if (foreground && tcsetpgrp(0, getpgid(getpid())) == -1) {
+        perror("tcsetpgrp");
+	    exit(4);
+      }
 
       /* Restore SIGTTIN and SIGTTOU signal to their original value
        * in order not to impact children processes behaviour. */
       signal (SIGTTIN, SIG_DFL);
-      signal (SIGTTOU, SIG_DFL);
-      execvp ((const char *) argv[2], (char *const *) &argv[2]);
-      fprintf (stderr, "rlimit: could not run \"%s\": ", argv[2]);
+
+      /* When we are not in the foreground, we ignore SIGTTOU so that child process can
+       * write. */
+      if (foreground)
+        signal (SIGTTOU, SIG_DFL);
+      execvp ((const char *) argv[begin], (char *const *) &argv[begin]);
+      fprintf (stderr, "rlimit: could not run \"%s\": ", argv[begin]);
       perror ("execvp");
       exit (5);
 
     default: {
       /* parent sleeps
          wake up when the sleep call returns or when
          SIGCHLD is received */
-      int timeout = atoi (argv[1]);
+      int timeout = atoi (argv[begin - 1]);
       int seconds = timeout;
 
       /* At this stage rlimit might not be anymore the foreground process.

tox.ini

@@ -6,6 +6,7 @@ deps =
      xdist: pytest-xdist[psutil]
      # ??? needs to be added as a dep of e3-core
      requests-cache
+     ptyprocess;platform_system == "Linux"
      cov: pytest-cov
      cov: coverage