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.

Author: brammool

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. */

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;
 

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()

src/version.c

@@ -769,6 +769,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    849,
 /**/
     848,
 /**/