From ed5a9d661248a2160368f1b0ab3a1bf74831db04 Mon Sep 17 00:00:00 2001 From: Bram Moolenaar Date: Thu, 6 Sep 2018 13:14:43 +0200 Subject: [PATCH 1/3] patch 8.1.0349: crash when wiping buffer in a callback Problem: Crash when wiping buffer in a callback. Solution: Do not handle messages when only peeking for a character. (closes #2107) Add "redraw_flag" to test_override(). --- runtime/doc/eval.txt | 1 + src/evalfunc.c | 3 +++ src/globals.h | 7 ++++--- src/os_unix.c | 12 ++++++++---- src/os_win32.c | 10 +++++++--- src/screen.c | 7 +++++-- src/version.c | 2 ++ 7 files changed, 30 insertions(+), 12 deletions(-) diff --git a/runtime/doc/eval.txt b/runtime/doc/eval.txt index 6408165214..d51b92ce5f 100644 --- a/runtime/doc/eval.txt +++ b/runtime/doc/eval.txt @@ -8737,6 +8737,7 @@ test_override({name}, {val}) *test_override()* name effect when {val} is non-zero ~ redraw disable the redrawing() function + redraw_flag ignore the RedrawingDisabled flag char_avail disable the char_avail() function starting reset the "starting" variable, see below nfa_fail makes the NFA regexp engine fail to force a diff --git a/src/evalfunc.c b/src/evalfunc.c index 8a1fcef14b..90fb8881b4 100644 --- a/src/evalfunc.c +++ b/src/evalfunc.c @@ -13073,6 +13073,8 @@ f_test_override(typval_T *argvars, typval_T *rettv UNUSED) if (STRCMP(name, (char_u *)"redraw") == 0) disable_redraw_for_testing = val; + else if (STRCMP(name, (char_u *)"redraw_flag") == 0) + ignore_redraw_flag_for_testing = val; else if (STRCMP(name, (char_u *)"char_avail") == 0) disable_char_avail_for_testing = val; else if (STRCMP(name, (char_u *)"starting") == 0) @@ -13095,6 +13097,7 @@ f_test_override(typval_T *argvars, typval_T *rettv UNUSED) { disable_char_avail_for_testing = FALSE; disable_redraw_for_testing = FALSE; + ignore_redraw_flag_for_testing = FALSE; nfa_fail_for_testing = FALSE; if (save_starting >= 0) { diff --git a/src/globals.h b/src/globals.h index fa5b493e8a..3446cba751 100644 --- a/src/globals.h +++ b/src/globals.h @@ -1633,9 +1633,10 @@ EXTERN int alloc_fail_countdown INIT(= -1); EXTERN int alloc_fail_repeat INIT(= 0); /* flags set by test_override() */ -EXTERN int disable_char_avail_for_testing INIT(= 0); -EXTERN int disable_redraw_for_testing INIT(= 0); -EXTERN int nfa_fail_for_testing INIT(= 0); +EXTERN int disable_char_avail_for_testing INIT(= FALSE); +EXTERN int disable_redraw_for_testing INIT(= FALSE); +EXTERN int ignore_redraw_flag_for_testing INIT(= FALSE); +EXTERN int nfa_fail_for_testing INIT(= FALSE); EXTERN int in_free_unref_items INIT(= FALSE); #endif diff --git a/src/os_unix.c b/src/os_unix.c index 99662218b6..f33042675d 100644 --- a/src/os_unix.c +++ b/src/os_unix.c @@ -417,10 +417,14 @@ mch_inchar( handle_resize(); #ifdef MESSAGE_QUEUE - parse_queued_messages(); - /* If input was put directly in typeahead buffer bail out here. */ - if (typebuf_changed(tb_change_cnt)) - return 0; + // Only process messages when waiting. + if (wtime != 0) + { + parse_queued_messages(); + // If input was put directly in typeahead buffer bail out here. + if (typebuf_changed(tb_change_cnt)) + return 0; + } #endif if (wtime < 0 && did_start_blocking) /* blocking and already waited for p_ut */ diff --git a/src/os_win32.c b/src/os_win32.c index 02bcaaede9..9d36dec4a5 100644 --- a/src/os_win32.c +++ b/src/os_win32.c @@ -1529,15 +1529,19 @@ WaitForChar(long msec, int ignore_input) */ for (;;) { + // Only process messages when waiting. + if (msec != 0) + { #ifdef MESSAGE_QUEUE - parse_queued_messages(); + parse_queued_messages(); #endif #ifdef FEAT_MZSCHEME - mzvim_check_threads(); + mzvim_check_threads(); #endif #ifdef FEAT_CLIENTSERVER - serverProcessPendingMessages(); + serverProcessPendingMessages(); #endif + } if (0 #ifdef FEAT_MOUSE diff --git a/src/screen.c b/src/screen.c index 743c321c47..c9f9410b6a 100644 --- a/src/screen.c +++ b/src/screen.c @@ -10819,8 +10819,11 @@ redrawing(void) return 0; else #endif - return (!RedrawingDisabled - && !(p_lz && char_avail() && !KeyTyped && !do_redraw)); + return ((!RedrawingDisabled +#ifdef FEAT_EVAL + || ignore_redraw_flag_for_testing +#endif + ) && !(p_lz && char_avail() && !KeyTyped && !do_redraw)); } /* diff --git a/src/version.c b/src/version.c index ab77c13653..9e399a656d 100644 --- a/src/version.c +++ b/src/version.c @@ -794,6 +794,8 @@ static char *(features[]) = static int included_patches[] = { /* Add new patch number below this line */ +/**/ + 349, /**/ 348, /**/ From 0b1468884a2a1c5d3442cbb7119330e307f0aa3d Mon Sep 17 00:00:00 2001 From: Bram Moolenaar Date: Thu, 6 Sep 2018 16:27:24 +0200 Subject: [PATCH 2/3] patch 8.1.0350: Vim may block on ch_sendraw() Problem: Vim may block on ch_sendraw() when the job is sending data back to Vim, which isn't read yet. (Nate Bosch) Solution: Add the "noblock" option to job_start(). (closes #2548) --- runtime/doc/channel.txt | 14 ++++++++++++++ src/channel.c | 12 +++++++++++- src/structs.h | 2 ++ src/testdir/test_channel.vim | 34 +++++++++++++++++++++++++++++++++- src/version.c | 2 ++ 5 files changed, 62 insertions(+), 2 deletions(-) diff --git a/runtime/doc/channel.txt b/runtime/doc/channel.txt index 4cc36258c0..fd08cd4974 100644 --- a/runtime/doc/channel.txt +++ b/runtime/doc/channel.txt @@ -163,6 +163,9 @@ Use |ch_status()| to see if the channel could be opened. The "close_cb" is also considered for this. "never" All messages will be kept. + *channel-noblock* +"noblock" Same effect as |job-noblock|. Only matters for writing. + *waittime* "waittime" The time to wait for the connection to be made in milliseconds. A negative number waits forever. @@ -594,6 +597,17 @@ See |job_setoptions()| and |ch_setoptions()|. Note: when writing to a file or buffer and when reading from a buffer NL mode is used by default. + *job-noblock* +"noblock": 1 When writing use a non-blocking write call. This + avoids getting stuck if Vim should handle other + messages in between, e.g. when a job sends back data + to Vim. It implies that when `ch_sendraw()` returns + not all data may have been written yet. + This option was added in patch 8.1.0350, test with: > + if has("patch-8.1.350") + let options['noblock'] = 1 + endif +< *job-callback* "callback": handler Callback for something to read on any part of the channel. diff --git a/src/channel.c b/src/channel.c index 282340eefc..793dbaa833 100644 --- a/src/channel.c +++ b/src/channel.c @@ -1180,6 +1180,7 @@ channel_set_options(channel_T *channel, jobopt_T *opt) channel->ch_part[PART_OUT].ch_mode = opt->jo_out_mode; if (opt->jo_set & JO_ERR_MODE) channel->ch_part[PART_ERR].ch_mode = opt->jo_err_mode; + channel->ch_nonblock = opt->jo_noblock; if (opt->jo_set & JO_TIMEOUT) for (part = PART_SOCK; part < PART_COUNT; ++part) @@ -3677,7 +3678,7 @@ channel_any_keep_open() channel_set_nonblock(channel_T *channel, ch_part_T part) { chanpart_T *ch_part = &channel->ch_part[part]; - int fd = ch_part->ch_fd; + int fd = ch_part->ch_fd; if (fd != INVALID_FD) { @@ -3722,6 +3723,9 @@ channel_send( return FAIL; } + if (channel->ch_nonblock && !ch_part->ch_nonblocking) + channel_set_nonblock(channel, part); + if (ch_log_active()) { ch_log_lead("SEND ", channel, part); @@ -4553,6 +4557,12 @@ get_job_options(typval_T *tv, jobopt_T *opt, int supported, int supported2) == FAIL) return FAIL; } + else if (STRCMP(hi->hi_key, "noblock") == 0) + { + if (!(supported & JO_MODE)) + break; + opt->jo_noblock = get_tv_number(item); + } else if (STRCMP(hi->hi_key, "in_io") == 0 || STRCMP(hi->hi_key, "out_io") == 0 || STRCMP(hi->hi_key, "err_io") == 0) diff --git a/src/structs.h b/src/structs.h index ec109eb893..253f774abc 100644 --- a/src/structs.h +++ b/src/structs.h @@ -1651,6 +1651,7 @@ struct channel_S { partial_T *ch_close_partial; int ch_drop_never; int ch_keep_open; /* do not close on read error */ + int ch_nonblock; job_T *ch_job; /* Job that uses this channel; this does not * count as a reference to avoid a circular @@ -1729,6 +1730,7 @@ typedef struct ch_mode_T jo_in_mode; ch_mode_T jo_out_mode; ch_mode_T jo_err_mode; + int jo_noblock; job_io_T jo_io[4]; /* PART_OUT, PART_ERR, PART_IN */ char_u jo_io_name_buf[4][NUMBUFLEN]; diff --git a/src/testdir/test_channel.vim b/src/testdir/test_channel.vim index 371afc7997..5d071473a5 100644 --- a/src/testdir/test_channel.vim +++ b/src/testdir/test_channel.vim @@ -47,8 +47,11 @@ endfunc func Ch_communicate(port) " Avoid dropping messages, since we don't use a callback here. let s:chopt.drop = 'never' + " Also add the noblock flag to try it out. + let s:chopt.noblock = 1 let handle = ch_open('localhost:' . a:port, s:chopt) unlet s:chopt.drop + unlet s:chopt.noblock if ch_status(handle) == "fail" call assert_report("Can't open channel") return @@ -451,8 +454,9 @@ func Test_raw_pipe() call ch_log('Test_raw_pipe()') " Add a dummy close callback to avoid that messages are dropped when calling " ch_canread(). + " Also test the non-blocking option. let job = job_start(s:python . " test_channel_pipe.py", - \ {'mode': 'raw', 'drop': 'never'}) + \ {'mode': 'raw', 'drop': 'never', 'noblock': 1}) call assert_equal(v:t_job, type(job)) call assert_equal("run", job_status(job)) @@ -1350,6 +1354,34 @@ endfunc """""""""" +function ExitCbWipe(job, status) + exe g:wipe_buf 'bw!' +endfunction + +" This caused a crash, because messages were handled while peeking for a +" character. +func Test_exit_cb_wipes_buf() + if !has('timers') + return + endif + set cursorline lazyredraw + call test_override('redraw_flag', 1) + new + let g:wipe_buf = bufnr('') + + let job = job_start(['true'], {'exit_cb': 'ExitCbWipe'}) + let timer = timer_start(300, {-> feedkeys("\", 'nt')}, {'repeat': 5}) + call feedkeys(repeat('g', 1000) . 'o', 'ntx!') + call WaitForAssert({-> assert_equal("dead", job_status(job))}) + call timer_stop(timer) + + set nocursorline nolazyredraw + unlet g:wipe_buf + call test_override('ALL', 0) +endfunc + +"""""""""" + let g:Ch_unletResponse = '' func s:UnletHandler(handle, msg) let g:Ch_unletResponse = a:msg diff --git a/src/version.c b/src/version.c index 9e399a656d..292af60f53 100644 --- a/src/version.c +++ b/src/version.c @@ -794,6 +794,8 @@ static char *(features[]) = static int included_patches[] = { /* Add new patch number below this line */ +/**/ + 350, /**/ 349, /**/ From 198cb66d652d3d8ac16226dcc929a11b0b720151 Mon Sep 17 00:00:00 2001 From: Bram Moolenaar Date: Thu, 6 Sep 2018 21:44:17 +0200 Subject: [PATCH 3/3] patch 8.1.0351: 'incsearch' for :/foo/s// changes last search pattern Problem: 'incsearch' for :/foo/s// changes last search pattern. Solution: Save the last search pattern earlier. --- src/ex_docmd.c | 4 +++- src/ex_getln.c | 23 +++++++++++++++++++++-- src/testdir/test_search.vim | 17 +++++++++++++++++ src/version.c | 2 ++ 4 files changed, 43 insertions(+), 3 deletions(-) diff --git a/src/ex_docmd.c b/src/ex_docmd.c index 5b53785cc5..767791776e 100644 --- a/src/ex_docmd.c +++ b/src/ex_docmd.c @@ -2911,6 +2911,7 @@ free_cmdmod(void) /* * Parse the address range, if any, in "eap". + * May set the last search pattern. * Return FAIL and set "errormsg" or return OK. */ int @@ -4436,10 +4437,11 @@ skip_range( } /* - * get a single EX address + * Get a single EX address. * * Set ptr to the next character after the part that was interpreted. * Set ptr to NULL when an error is encountered. + * This may set the last used search pattern. * * Return MAXLNUM when no Ex address was found. */ diff --git a/src/ex_getln.c b/src/ex_getln.c index c316e192ad..80f210a5b0 100644 --- a/src/ex_getln.c +++ b/src/ex_getln.c @@ -271,6 +271,7 @@ set_search_match(pos_T *t) /* * Return TRUE when 'incsearch' highlighting is to be done. * Sets search_first_line and search_last_line to the address range. + * May change the last search pattern. */ static int do_incsearch_highlighting(int firstc, incsearch_state_T *is_state, @@ -470,8 +471,12 @@ may_do_incsearch_highlighting( int next_char; int use_last_pat; + // Parsing range may already set the last search pattern. + save_last_search_pattern(); + if (!do_incsearch_highlighting(firstc, is_state, &skiplen, &patlen)) { + restore_last_search_pattern(); finish_incsearch_highlighting(FALSE, is_state, TRUE); return; } @@ -479,6 +484,7 @@ may_do_incsearch_highlighting( // If there is a character waiting, search and redraw later. if (char_avail()) { + restore_last_search_pattern(); is_state->incsearch_postponed = TRUE; return; } @@ -493,7 +499,6 @@ may_do_incsearch_highlighting( curwin->w_cursor.lnum = search_first_line; curwin->w_cursor.col = 0; } - save_last_search_pattern(); // Use the previous pattern for ":s//". next_char = ccline.cmdbuff[skiplen + patlen]; @@ -627,10 +632,19 @@ may_adjust_incsearch_highlighting( int i; int save; + // Parsing range may already set the last search pattern. + save_last_search_pattern(); + if (!do_incsearch_highlighting(firstc, is_state, &skiplen, &patlen)) + { + restore_last_search_pattern(); return OK; + } if (patlen == 0 && ccline.cmdbuff[skiplen] == NUL) + { + restore_last_search_pattern(); return FAIL; + } if (firstc == ccline.cmdbuff[skiplen]) { @@ -641,7 +655,6 @@ may_adjust_incsearch_highlighting( else pat = ccline.cmdbuff + skiplen; - save_last_search_pattern(); cursor_off(); out_flush(); if (c == Ctrl_G) @@ -721,8 +734,14 @@ may_add_char_to_search(int firstc, int *c, incsearch_state_T *is_state) { int skiplen, patlen; + // Parsing range may already set the last search pattern. + save_last_search_pattern(); + if (!do_incsearch_highlighting(firstc, is_state, &skiplen, &patlen)) + { + restore_last_search_pattern(); return FAIL; + } // Add a character from under the cursor for 'incsearch'. if (is_state->did_incsearch) diff --git a/src/testdir/test_search.vim b/src/testdir/test_search.vim index 68dc3ec8e8..f96e54abe2 100644 --- a/src/testdir/test_search.vim +++ b/src/testdir/test_search.vim @@ -1043,6 +1043,23 @@ func Test_incsearch_vimgrep_dump() call delete('Xis_vimgrep_script') endfunc +func Test_keep_last_search_pattern() + if !exists('+incsearch') + return + endif + new + call setline(1, ['foo', 'foo', 'foo']) + set incsearch + call test_override("char_avail", 1) + let @/ = 'bar' + call feedkeys(":/foo/s//\", 'ntx') + call assert_equal('bar', @/) + + bwipe! + call test_override("ALL", 0) + set noincsearch +endfunc + func Test_search_undefined_behaviour() if !has("terminal") return diff --git a/src/version.c b/src/version.c index 292af60f53..ba1ac1a19c 100644 --- a/src/version.c +++ b/src/version.c @@ -794,6 +794,8 @@ static char *(features[]) = static int included_patches[] = { /* Add new patch number below this line */ +/**/ + 351, /**/ 350, /**/