[Tarantool-patches] [PATCH luajit v3 2/2] Improve CLI signal handling on POSIX.

[Evgeniy Temirgaleev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

From: Mike Pall <mike>

(cherry picked from commit 423ac2144b0447c925596f4d93e073ce501ed4e1)

An obsolete signal() function in glibc is implemented as a wrapper for
POSIX sigaction() call with BSD semantic. In particular, it provides
an SA_RESTART flag for SIGINT handler [1-2]. The latter makes certain
system calls, e.g. read() or wait(), restartable across signals [3].

The patch switches from signal() to sigaction() for POSIX systems
to obtain an explicit control of the signal flag set, which must
not contain SA_RESTART flag. (Actually, the empty flag set is
used.)

1. https://man7.org/linux/man-pages/man2/signal.2.html
2. https://ftp.gnu.org/gnu/glibc/glibc-2.44.tar.xz (sysdeps/posix/signal.c, line 44)
3. https://man7.org/linux/man-pages/man2/sigaction.2.html

Temir Galeev:
* added the description and the test for the problem

Part of tarantool/tarantool#13442
---
 src/luajit.c                                  | 23 +++++--
 ...ket-fix-cli-posix-sigint-handling.test.lua | 64 +++++++++++++++++++
 .../script.lua                                | 26 ++++++++
 3 files changed, 109 insertions(+), 4 deletions(-)
 create mode 100644 test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling.test.lua
 create mode 100644 test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.lua

diff --git a/src/luajit.c b/src/luajit.c
index d9d85361..d35bea54 100644
--- a/src/luajit.c
+++ b/src/luajit.c
@@ -35,6 +35,21 @@
 
 #if !LJ_TARGET_CONSOLE
 #include <signal.h>
+
+#if LJ_TARGET_POSIX
+/* Improve signal handling on POSIX. Try CTRL-C on: luajit -e 'io.read()' */
+static void signal_set(int sig, void (*h)(int))
+{
+  struct sigaction sa;
+  memset(&sa, 0, sizeof(sa));
+  sa.sa_handler = h;
+  sigemptyset(&sa.sa_mask);
+  sigaction(sig, &sa, NULL);
+}
+#else
+#define signal_set	signal
+#endif
+
 #endif
 
 static lua_State *globalL = NULL;
@@ -54,8 +69,8 @@ static void lstop(lua_State *L, lua_Debug *ar)
 
 static void laction(int i)
 {
-  signal(i, SIG_DFL); /* if another SIGINT happens before lstop,
-			 terminate process (default action) */
+  /* Terminate process if another SIGINT happens (double CTRL-C). */
+  signal_set(i, SIG_DFL);
   lua_sethook(globalL, lstop, LUA_MASKCALL | LUA_MASKRET | LUA_MASKCOUNT, 1);
 }
 #endif
@@ -129,11 +144,11 @@ static int docall(lua_State *L, int narg, int clear)
   lua_pushcfunction(L, traceback);  /* push traceback function */
   lua_insert(L, base);  /* put it under chunk and args */
 #if !LJ_TARGET_CONSOLE
-  signal(SIGINT, laction);
+  signal_set(SIGINT, laction);
 #endif
   status = lua_pcall(L, narg, (clear ? 0 : LUA_MULTRET), base);
 #if !LJ_TARGET_CONSOLE
-  signal(SIGINT, SIG_DFL);
+  signal_set(SIGINT, SIG_DFL);
 #endif
   lua_remove(L, base);  /* remove traceback function */
   /* force a complete garbage collection in case of errors */
diff --git a/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling.test.lua b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling.test.lua
new file mode 100644
index 00000000..5b235660
--- /dev/null
+++ b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling.test.lua
@@ -0,0 +1,64 @@
+local tap = require('tap')
+
+-- The test checks that luajit CLI stop hook is executed on
+-- SIGINT on a blocking system call (read(), waitpid(), etc.).
+--
+-- The test runs luajit CLI with a script, which prints it's PID
+-- as a ready marker and blocks on a read() call. After the
+-- child's PID number is read from child's pipe, the test send
+-- SIGINT to the child.
+-- With a patch applied (no SA_RESTART flag for SIGINT handler)
+-- read() call is interrupted and the CLI's Lua stop hook
+-- executed.
+-- Without a patch (SA_RESTART flag is present for SIGINT
+-- handler) read() call is restarted, the test and the child is
+-- blocked on a pipe read until the child is terminated by
+-- SIGALRM. The test reads nothing from the child and fails.
+
+local test = tap.test('lj-noticket-fix-cli-posix-sigint-handling'):skipcond({
+  -- The patched code is not present in Tarantool binary:
+  ["Disabled due to Tarantool uses it's own signal handling"] = _TARANTOOL,
+  ['Disabled on POSIX nonconformant OS'] = jit.os ~= 'BSD' and
+                                           jit.os ~= 'Linux' and
+                                           jit.os ~= 'OSX' and
+                                           jit.os ~= 'POSIX',
+})
+
+local ffi = require('ffi')
+ffi.cdef[[
+  int kill(int pid, int sig);
+]]
+local SIGINT = 2
+
+test:plan(1)
+local utils = require('utils')
+local script = utils.exec.test_data(arg, 'script.lua')
+local cmd = ('exec %s %s 2>&1'):format(utils.exec.luacmd(arg), script)
+local proc = io.popen(cmd)
+
+local MARKER = 'CHILD_PID:'
+local RMARKER = '^'..MARKER..'(%d+)$'
+-- The child prints its PID after the CLI has installed the SIGINT
+-- handler, so it is a readiness marker. Skip possible noise
+-- lines, collect pre-marker output for error reporting.
+local lines_for_diag = {'child output:'}
+local pid, line
+repeat
+  line = proc:read('*l')
+  if line ~= nil then
+    table.insert(lines_for_diag, line)
+    local match = line:match(RMARKER)
+    if match ~= nil then pid = tonumber(match) end
+  end
+until line == nil or pid ~= nil
+if pid == nil then
+  test:diag(table.concat(lines_for_diag, '\n'))
+end
+assert(pid ~= nil, 'the child process must report its PID')
+
+local res = ffi.C.kill(pid, SIGINT)
+assert(res == 0, 'SIGINT delivery failed')
+local output = proc:read('*a')
+proc:close()
+test:like(output, 'interrupted!', 'SIGINT interrupts the blocking call')
+test:done(true)
diff --git a/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.lua b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.lua
new file mode 100644
index 00000000..d2430a52
--- /dev/null
+++ b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.lua
@@ -0,0 +1,26 @@
+local ffi = require('ffi')
+
+ffi.cdef[[
+  int pipe(int *fds);
+  int getpid(void);
+  long read(int fd, void *buf, size_t count);
+  unsigned int alarm(unsigned int seconds);
+]]
+
+local fds = ffi.new('int[2]')
+assert(ffi.C.pipe(fds) == 0)
+
+-- Fail instead of hanging forever if SIGINT does not break
+-- the call. (SIGALRM isn't handled by CLI.)
+ffi.C.alarm(10)
+
+-- The CLI installs the SIGINT handler before the script
+-- starts, so this line is the readiness marker for the test.
+local MARKER = 'CHILD_PID:'
+io.write(MARKER, ffi.C.getpid(), '\n')
+io.flush()
+
+-- Block in read(2) until SIGINT. Without SA_RESTART it returns
+-- EINTR and the CLI runs the Lua hook, which raises
+-- "interrupted!".
+ffi.C.read(fds[0], ffi.new('char[1]'), 1)
-- 
2.49.0