patch 8.0.0849: crash when job exit callback wipes the terminal

Problem:    Crash when job exit callback wipes the terminal.
Solution:   Check for b_term to be NULL.  (Yasuhiro Matsumoto, closes #1922)
            Implement options for term_start() to be able to test.
            Make term_wait() more reliable.

diff --git a/src/channel.c b/src/channel.c
index fc5e0f5fed23ea485f6e950e61af6af262970d18..c511f22ed1ee1affd9723d403e50acaf498a88d9 100644 (file)
--- a/src/channel.c
+++ b/src/channel.c
@@ -4160,7 +4160,6 @@ get_job_options(typval_T *tv, jobopt_T *opt, int supported)
     hashitem_T *hi;
     ch_part_T  part;
 
-    opt->jo_set = 0;
     if (tv->v_type == VAR_UNKNOWN)
        return OK;
     if (tv->v_type != VAR_DICT)
@@ -4616,6 +4615,7 @@ job_cleanup(job_T *job)
        int             dummy;
 
        /* Invoke the exit callback. Make sure the refcount is > 0. */
+       ch_log(job->jv_channel, "Invoking exit callback %s", job->jv_exit_cb);
        ++job->jv_refcount;
        argv[0].v_type = VAR_JOB;
        argv[0].vval.v_job = job;
@@ -4888,7 +4888,7 @@ job_start(typval_T *argvars, jobopt_T *opt_arg)
        if (get_job_options(&argvars[1], &opt,
            JO_MODE_ALL + JO_CB_ALL + JO_TIMEOUT_ALL + JO_STOPONEXIT
                           + JO_EXIT_CB + JO_OUT_IO + JO_BLOCK_WRITE) == FAIL)
-       goto theend;
+           goto theend;
     }
 
     /* Check that when io is "file" that there is a file name. */

diff --git a/src/terminal.c b/src/terminal.c
index 2c0879d560d974f3cb208c1d1971b3c0d22b9fbe..abdb50b30dfafecd03a681d6e45c0a308c522fd0 100644 (file)
--- a/src/terminal.c
+++ b/src/terminal.c
@@ -40,6 +40,7 @@
  * - MS-Windows: no redraw for 'updatetime'  #1915
  * - in bash mouse clicks are inserting characters.
  * - mouse scroll: when over other window, scroll that window.
+ * - add argument to term_wait() for waiting time.
  * - For the scrollback buffer store lines in the buffer, only attributes in
  *   tl_scrollback.
  * - When the job ends:
@@ -146,7 +147,7 @@ static term_T *first_term = NULL;
 /*
  * Functions with separate implementation for MS-Windows and Unix-like systems.
  */
-static int term_and_job_init(term_T *term, int rows, int cols, char_u *cmd);
+static int term_and_job_init(term_T *term, int rows, int cols, char_u *cmd, jobopt_T *opt);
 static void term_report_winsize(term_T *term, int rows, int cols);
 static void term_free_vterm(term_T *term);
 
@@ -185,15 +186,49 @@ set_term_and_win_size(term_T *term)
 }
 
 /*
- * ":terminal": open a terminal window and execute a job in it.
+ * Initialize job options for a terminal job.
+ * Caller may overrule some of them.
  */
-    void
-ex_terminal(exarg_T *eap)
+    static void
+init_job_options(jobopt_T *opt)
+{
+    clear_job_options(opt);
+
+    opt->jo_mode = MODE_RAW;
+    opt->jo_out_mode = MODE_RAW;
+    opt->jo_err_mode = MODE_RAW;
+    opt->jo_set = JO_MODE | JO_OUT_MODE | JO_ERR_MODE;
+
+    opt->jo_io[PART_OUT] = JIO_BUFFER;
+    opt->jo_io[PART_ERR] = JIO_BUFFER;
+    opt->jo_set |= JO_OUT_IO + JO_ERR_IO;
+
+    opt->jo_modifiable[PART_OUT] = 0;
+    opt->jo_modifiable[PART_ERR] = 0;
+    opt->jo_set |= JO_OUT_MODIFIABLE + JO_ERR_MODIFIABLE;
+
+    opt->jo_set |= JO_OUT_BUF + JO_ERR_BUF;
+}
+
+/*
+ * Set job options mandatory for a terminal job.
+ */
+    static void
+setup_job_options(jobopt_T *opt, int rows, int cols)
+{
+    opt->jo_io_buf[PART_OUT] = curbuf->b_fnum;
+    opt->jo_io_buf[PART_ERR] = curbuf->b_fnum;
+    opt->jo_pty = TRUE;
+    opt->jo_term_rows = rows;
+    opt->jo_term_cols = cols;
+}
+
+    static void
+term_start(char_u *cmd, jobopt_T *opt)
 {
     exarg_T    split_ea;
     win_T      *old_curwin = curwin;
     term_T     *term;
-    char_u     *cmd = eap->arg;
 
     if (check_restricted() || check_secure())
        return;
@@ -256,9 +291,10 @@ ex_terminal(exarg_T *eap)
                                  (char_u *)"terminal", OPT_FREE|OPT_LOCAL, 0);
 
     set_term_and_win_size(term);
+    setup_job_options(opt, term->tl_rows, term->tl_cols);
 
     /* System dependent: setup the vterm and start the job in it. */
-    if (term_and_job_init(term, term->tl_rows, term->tl_cols, cmd) == OK)
+    if (term_and_job_init(term, term->tl_rows, term->tl_cols, cmd, opt) == OK)
     {
        /* store the size we ended up with */
        vterm_get_size(term->tl_vterm, &term->tl_rows, &term->tl_cols);
@@ -273,6 +309,20 @@ ex_terminal(exarg_T *eap)
     }
 }
 
+/*
+ * ":terminal": open a terminal window and execute a job in it.
+ */
+    void
+ex_terminal(exarg_T *eap)
+{
+    jobopt_T opt;
+
+    init_job_options(&opt);
+    /* TODO: get options from before the command */
+
+    term_start(eap->arg, &opt);
+}
+
 /*
  * Free the scrollback buffer for "term".
  */
@@ -965,8 +1015,7 @@ terminal_loop(void)
        update_cursor(curbuf->b_term, FALSE);
 
        c = term_vgetc();
-       if (curbuf->b_term->tl_vterm == NULL
-                                         || !term_job_running(curbuf->b_term))
+       if (!term_use_loop())
            /* job finished while waiting for a character */
            break;
 
@@ -993,8 +1042,7 @@ terminal_loop(void)
 #ifdef FEAT_CMDL_INFO
            clear_showcmd();
 #endif
-           if (curbuf->b_term->tl_vterm == NULL
-                                         || !term_job_running(curbuf->b_term))
+           if (!term_use_loop())
                /* job finished while waiting for a character */
                break;
 
@@ -1613,35 +1661,6 @@ term_get_attr(buf_T *buf, linenr_T lnum, int col)
     return cell2attr(line->sb_cells + col);
 }
 
-/*
- * Set job options common for Unix and MS-Windows.
- */
-    static void
-setup_job_options(jobopt_T *opt, int rows, int cols)
-{
-    clear_job_options(opt);
-    opt->jo_mode = MODE_RAW;
-    opt->jo_out_mode = MODE_RAW;
-    opt->jo_err_mode = MODE_RAW;
-    opt->jo_set = JO_MODE | JO_OUT_MODE | JO_ERR_MODE;
-
-    opt->jo_io[PART_OUT] = JIO_BUFFER;
-    opt->jo_io[PART_ERR] = JIO_BUFFER;
-    opt->jo_set |= JO_OUT_IO + JO_ERR_IO;
-
-    opt->jo_modifiable[PART_OUT] = 0;
-    opt->jo_modifiable[PART_ERR] = 0;
-    opt->jo_set |= JO_OUT_MODIFIABLE + JO_ERR_MODIFIABLE;
-
-    opt->jo_io_buf[PART_OUT] = curbuf->b_fnum;
-    opt->jo_io_buf[PART_ERR] = curbuf->b_fnum;
-    opt->jo_pty = TRUE;
-    opt->jo_set |= JO_OUT_BUF + JO_ERR_BUF;
-
-    opt->jo_term_rows = rows;
-    opt->jo_term_cols = cols;
-}
-
 /*
  * Create a new vterm and initialize it.
  */
@@ -2089,12 +2108,19 @@ f_term_sendkeys(typval_T *argvars, typval_T *rettv)
 f_term_start(typval_T *argvars, typval_T *rettv)
 {
     char_u     *cmd = get_tv_string_chk(&argvars[0]);
-    exarg_T    ea;
+    jobopt_T   opt;
 
     if (cmd == NULL)
        return;
-    ea.arg = cmd;
-    ex_terminal(&ea);
+    init_job_options(&opt);
+    /* TODO: allow more job options */
+    if (argvars[1].v_type != VAR_UNKNOWN
+           && get_job_options(&argvars[1], &opt,
+               JO_TIMEOUT_ALL + JO_STOPONEXIT
+               + JO_EXIT_CB + JO_CLOSE_CALLBACK) == FAIL)
+       return;
+
+    term_start(cmd, &opt);
 
     if (curbuf->b_term != NULL)
        rettv->vval.v_number = curbuf->b_fnum;
@@ -2109,19 +2135,40 @@ f_term_wait(typval_T *argvars, typval_T *rettv UNUSED)
     buf_T      *buf = term_get_buf(argvars);
 
     if (buf == NULL)
+    {
+       ch_log(NULL, "term_wait(): invalid argument");
        return;
+    }
 
     /* Get the job status, this will detect a job that finished. */
-    if (buf->b_term->tl_job != NULL)
-       (void)job_status(buf->b_term->tl_job);
+    if (buf->b_term->tl_job == NULL
+           || STRCMP(job_status(buf->b_term->tl_job), "dead") == 0)
+    {
+       /* The job is dead, keep reading channel I/O until the channel is
+        * closed. */
+       while (buf->b_term != NULL && !buf->b_term->tl_channel_closed)
+       {
+           mch_char_avail();
+           parse_queued_messages();
+           ui_delay(10L, FALSE);
+       }
+       mch_char_avail();
+       parse_queued_messages();
+    }
+    else
+    {
+       mch_char_avail();
+       parse_queued_messages();
 
-    /* Check for any pending channel I/O. */
-    vpeekc_any();
-    ui_delay(10L, FALSE);
+       /* Wait for 10 msec for any channel I/O. */
+       /* TODO: use delay from optional argument */
+       ui_delay(10L, TRUE);
+       mch_char_avail();
 
-    /* Flushing messages on channels is hopefully sufficient.
-     * TODO: is there a better way? */
-    parse_queued_messages();
+       /* Flushing messages on channels is hopefully sufficient.
+        * TODO: is there a better way? */
+       parse_queued_messages();
+    }
 }
 
 # ifdef WIN3264
@@ -2209,12 +2256,11 @@ dyn_winpty_init(void)
  * Return OK or FAIL.
  */
     static int
-term_and_job_init(term_T *term, int rows, int cols, char_u *cmd)
+term_and_job_init(term_T *term, int rows, int cols, char_u *cmd, jobopt_T *opt)
 {
     WCHAR          *p;
     channel_T      *channel = NULL;
     job_T          *job = NULL;
-    jobopt_T       opt;
     DWORD          error;
     HANDLE         jo = NULL, child_process_handle, child_thread_handle;
     void           *winpty_err;
@@ -2298,8 +2344,7 @@ term_and_job_init(term_T *term, int rows, int cols, char_u *cmd)
 
     create_vterm(term, rows, cols);
 
-    setup_job_options(&opt, rows, cols);
-    channel_set_job(channel, job, &opt);
+    channel_set_job(channel, job, opt);
 
     job->jv_channel = channel;
     job->jv_proc_info.hProcess = child_process_handle;
@@ -2381,18 +2426,17 @@ term_report_winsize(term_T *term, int rows, int cols)
  * Return OK or FAIL.
  */
     static int
-term_and_job_init(term_T *term, int rows, int cols, char_u *cmd)
+term_and_job_init(term_T *term, int rows, int cols, char_u *cmd, jobopt_T *opt)
 {
     typval_T   argvars[2];
-    jobopt_T   opt;
 
     create_vterm(term, rows, cols);
 
     /* TODO: if the command is "NONE" only create a pty. */
     argvars[0].v_type = VAR_STRING;
     argvars[0].vval.v_string = cmd;
-    setup_job_options(&opt, rows, cols);
-    term->tl_job = job_start(argvars, &opt);
+
+    term->tl_job = job_start(argvars, opt);
     if (term->tl_job != NULL)
        ++term->tl_job->jv_refcount;
 

diff --git a/src/testdir/test_terminal.vim b/src/testdir/test_terminal.vim
index cc5afc3940f298b21bcce5bfac8c106e03e6044a..90bc58b8545b78f4c125fe786a30fece6756c214 100644 (file)
--- a/src/testdir/test_terminal.vim
+++ b/src/testdir/test_terminal.vim
@@ -86,6 +86,23 @@ func Test_terminal_hide_buffer()
   unlet g:job
 endfunc
 
+func! s:Nasty_exit_cb(job, st)
+  exe g:buf . 'bwipe!'
+  let g:buf = 0
+endfunc
+
+func Test_terminal_nasty_cb()
+  let cmd = Get_cat_cmd()
+  let g:buf = term_start(cmd, {'exit_cb': function('s:Nasty_exit_cb')})
+  let g:job = term_getjob(g:buf)
+
+  call WaitFor('job_status(g:job) == "dead"')
+  call WaitFor('g:buf == 0')
+  unlet g:buf
+  unlet g:job
+  call delete('Xtext')
+endfunc
+
 func Check_123(buf)
   let l = term_scrape(a:buf, 0)
   call assert_true(len(l) == 0)
@@ -113,13 +130,17 @@ func Check_123(buf)
   call assert_equal('123', l)
 endfunc
 
-func Test_terminal_scrape()
+func Get_cat_cmd()
   if has('win32')
-    let cmd = 'cmd /c "cls && color 2 && echo 123"'
+    return 'cmd /c "cls && color 2 && echo 123"'
   else
     call writefile(["\<Esc>[32m123"], 'Xtext')
-    let cmd = "cat Xtext"
+    return "cat Xtext"
   endif
+endfunc
+
+func Test_terminal_scrape()
+  let cmd = Get_cat_cmd()
   let buf = term_start(cmd)
 
   let termlist = term_list()

diff --git a/src/version.c b/src/version.c
index ae8d0b77d70ab798cff9d272374692dc06db6bbb..8a029184c376c5e4893af1dc0b0c1febb9642db2 100644 (file)
--- a/src/version.c
+++ b/src/version.c
@@ -769,6 +769,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    849,
 /**/
     848,
 /**/