vim lets you (and plugins) start and stop processes using job_start and
job_stop. There is an issue: stopping does not work, see for example
https://github.com/lervag/vimtex/issues/1032
in the case of the vimtex plugin. This is because the function
mch_signal_job in vim's src/os_unix.c first checks whether the process
to kill is group leader:

#ifdef HAVE_GETPGID
    if (job_pid == getpgid(job_pid))
        job_pid = -job_pid;
#endif

    /* Never kill ourselves! */
    if (job_pid != 0)
        kill(job_pid, sig);

but on OpenBSD getpgid(2) will fail and return EPERM (-1) if the process
is not part of the same session. vim starts jobs with fork(2), setsid(2)
before execvp(3), so this is never the case. So only the process (not the
group) is sent a SIGTERM. But often this process is a 'sh -c command',
and when sh receives a SIGTERM it waits for the current running command
to terminate before it terminates (sh does not "forward" SIGTERM to the
subprocess).
I guess there are three ways to solve this:
1) in vim, replace setsid() by setpgid(0, 0) in the child after forking,
 so that a new group is created instead of a new session. This is
 implemented below (diff for the editors/vim port). It works for me, but
 there may be an unwanted side effect: after setsid(), a child has no
 controlling terminal, but if I understand correctly after setpgid(0, 0)
 the child is still on the same terminal, so it may "fight" vim for I/O.
 In fact vim's help for job_start does not seem to be aware of the fact
 that jobs start new sessions, because it warns about this "competing
 for terminal I/O" issue.
2) remove the check
    if (job_pid == getpgid(job_pid))
 from vim's mch_signal_job() in src/os_unix.c so that the whole group
 always gets a signal.
3) change getpgid in OpenBSD so that it returns the pgid even if the
 target process is not part of the same session as the calling process.
 I guess this option will not be popular, but I do not see what security
 improvement this achieves, when ps(1) (ultimately, kvm_getprocs(3))
 will give any user more information.

Side question: to debug this I added the following to VAR var[] in
/usr/src/bin/ps/keyword.c:
        PID("sid", "SID", pvar, POFF(p_sid)),
as well as "sid" in one of the format strings (for example jfmt which
already has "sess") in /usr/src/bin/ps/ps.c so that ps(1) also prints
session id's. Ultimately this was not necessary because vim's source is
clear, but maybe that could be useful for other purposes?

Index: patches/patch-src_gui_c
===================================================================
RCS file: patches/patch-src_gui_c
diff -N patches/patch-src_gui_c
--- /dev/null   1 Jan 1970 00:00:00 -0000
+++ patches/patch-src_gui_c     28 Nov 2018 22:43:12 -0000
@@ -0,0 +1,23 @@
+$OpenBSD$ replace setsid by setpgid
+
+Index: src/gui.c
+--- src/gui.c.orig
++++ src/gui.c
+@@ -278,16 +278,12 @@ gui_do_fork(void)
+       getout_preserve_modified(1);
+ #endif
+ 
+-# if defined(HAVE_SETSID) || defined(HAVE_SETPGID)
++# if defined(HAVE_SETPGID)
+     /*
+      * Change our process group.  On some systems/shells a CTRL-C in the
+      * shell where Vim was started would otherwise kill gvim!
+      */
+-#  if defined(HAVE_SETSID)
+-    (void)setsid();
+-#  else
+     (void)setpgid(0, 0);
+-#  endif
+ # endif
+     if (!pipe_error)
+       close(pipefd[0]);
Index: patches/patch-src_if_cscope_c
===================================================================
RCS file: patches/patch-src_if_cscope_c
diff -N patches/patch-src_if_cscope_c
--- /dev/null   1 Jan 1970 00:00:00 -0000
+++ patches/patch-src_if_cscope_c       28 Nov 2018 22:43:12 -0000
@@ -0,0 +1,21 @@
+$OpenBSD$ replace setsid by setpgid
+
+Index: src/if_cscope.c
+--- src/if_cscope.c.orig
++++ src/if_cscope.c
+@@ -948,14 +948,10 @@ err_closing:
+       vim_free(ppath);
+ 
+ #if defined(UNIX)
+-# if defined(HAVE_SETSID) || defined(HAVE_SETPGID)
++# if defined(HAVE_SETPGID)
+       /* Change our process group to avoid cscope receiving SIGWINCH. */
+-#  if defined(HAVE_SETSID)
+-      (void)setsid();
+-#  else
+       if (setpgid(0, 0) == -1)
+           PERROR(_("cs_create_connection setpgid failed"));
+-#  endif
+ # endif
+       if (execl("/bin/sh", "sh", "-c", cmd, (char *)NULL) == -1)
+           PERROR(_("cs_create_connection exec failed"));
Index: patches/patch-src_os_unix_c
===================================================================
RCS file: patches/patch-src_os_unix_c
diff -N patches/patch-src_os_unix_c
--- /dev/null   1 Jan 1970 00:00:00 -0000
+++ patches/patch-src_os_unix_c 28 Nov 2018 22:43:12 -0000
@@ -0,0 +1,23 @@
+$OpenBSD$ replace setsid by setpgid
+
+Index: src/os_unix.c
+--- src/os_unix.c.orig
++++ src/os_unix.c
+@@ -4714,7 +4714,7 @@ mch_call_shell_fork(
+                * because stdin is not a tty, we would lose /dev/tty. */
+               if (p_stmp)
+               {
+-                  (void)setsid();
++                  (void)setpgid(0, 0);
+ #  if defined(SIGHUP)
+                   /* When doing "!xterm&" and 'shell' is bash: the shell
+                    * will exit and send SIGHUP to all processes in its
+@@ -5544,7 +5544,7 @@ mch_job_start(char **argv, job_T *job, jobopt_T *optio
+       /* Create our own process group, so that the child and all its
+        * children can be kill()ed.  Don't do this when using pipes,
+        * because stdin is not a tty, we would lose /dev/tty. */
+-      (void)setsid();
++      (void)setpgid(0, 0);
+ # endif
+ 
+ # ifdef FEAT_TERMINAL

Reply via email to