From cf7ff70ca73218d618e7c00ab785bcf5f9120a94 Mon Sep 17 00:00:00 2001 From: Bram Moolenaar Date: Mon, 9 May 2016 17:20:14 +0200 Subject: [PATCH 1/4] patch 7.4.1826 Problem: Callbacks are invoked when it's not safe. (Andrew Stewart) Solution: When a channel is to be closed don't invoke callbacks right away, wait for a safe moment. --- src/channel.c | 21 +++++++++++++++++++-- src/structs.h | 3 +++ src/version.c | 2 ++ 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/channel.c b/src/channel.c index 8433236227..159a3a0368 100644 --- a/src/channel.c +++ b/src/channel.c @@ -2782,7 +2782,8 @@ channel_wait(channel_T *channel, sock_T fd, int timeout) channel_close_on_error(channel_T *channel, char *func) { /* Do not call emsg(), most likely the other end just exited. */ - ch_errors(channel, "%s(): Cannot read from channel", func); + ch_errors(channel, "%s(): Cannot read from channel, will close it soon", + func); /* Queue a "DETACH" netbeans message in the command queue in order to * terminate the netbeans session later. Do not end the session here @@ -2800,7 +2801,15 @@ channel_close_on_error(channel_T *channel, char *func) (int)STRLEN(DETACH_MSG_RAW), FALSE, "PUT "); /* When reading from stdout is not possible, assume the other side has - * died. */ + * died. Don't close the channel right away, it may be the wrong moment + * to invoke callbacks. */ + channel->ch_to_be_closed = TRUE; +} + + static void +channel_close_now(channel_T *channel) +{ + ch_log(channel, "Closing channel because of previous read error"); channel_close(channel, TRUE); if (channel->ch_nb_close_cb != NULL) (*channel->ch_nb_close_cb)(); @@ -3515,6 +3524,14 @@ channel_parse_messages(void) } while (channel != NULL) { + if (channel->ch_to_be_closed) + { + channel->ch_to_be_closed = FALSE; + channel_close_now(channel); + /* channel may have been freed, start over */ + channel = first_channel; + continue; + } if (channel->ch_refcount == 0 && !channel_still_useful(channel)) { /* channel is no longer useful, free it */ diff --git a/src/structs.h b/src/structs.h index 50901acd57..a2b38bffd8 100644 --- a/src/structs.h +++ b/src/structs.h @@ -1416,6 +1416,9 @@ struct channel_S { char *ch_hostname; /* only for socket, allocated */ int ch_port; /* only for socket */ + int ch_to_be_closed; /* When TRUE reading or writing failed and + * the channel must be closed when it's safe + * to invoke callbacks. */ int ch_error; /* When TRUE an error was reported. Avoids * giving pages full of error messages when * the other side has exited, only mention the diff --git a/src/version.c b/src/version.c index a6f85c3e32..373aff2dd0 100644 --- a/src/version.c +++ b/src/version.c @@ -753,6 +753,8 @@ static char *(features[]) = static int included_patches[] = { /* Add new patch number below this line */ +/**/ + 1826, /**/ 1825, /**/ From fb6ffc732e65dbc459c89247ff78134402f1a18b Mon Sep 17 00:00:00 2001 From: Bram Moolenaar Date: Mon, 9 May 2016 17:58:04 +0200 Subject: [PATCH 2/4] patch 7.4.1827 Problem: No error when invoking a callback when it's not safe. Solution: Add an error message. Avoid the error when freeing a channel. --- src/channel.c | 34 ++++++++++++++++++++++++++++++++-- src/structs.h | 2 ++ src/version.c | 2 ++ 3 files changed, 36 insertions(+), 2 deletions(-) diff --git a/src/channel.c b/src/channel.c index 159a3a0368..a7f4c4b8cb 100644 --- a/src/channel.c +++ b/src/channel.c @@ -59,6 +59,9 @@ static void channel_read(channel_T *channel, int part, char *func); /* Whether a redraw is needed for appending a line to a buffer. */ static int channel_need_redraw = FALSE; +/* Whether we are inside channel_parse_messages() or another situation where it + * is safe to invoke callbacks. */ +static int safe_to_invoke_callback = 0; #ifdef WIN32 static int @@ -403,8 +406,15 @@ channel_free(channel_T *channel) { if (!in_free_unref_items) { - channel_free_contents(channel); - channel_free_channel(channel); + if (safe_to_invoke_callback == 0) + { + channel->ch_to_be_freed = TRUE; + } + else + { + channel_free_contents(channel); + channel_free_channel(channel); + } } } @@ -444,6 +454,10 @@ free_unused_channels_contents(int copyID, int mask) int did_free = FALSE; channel_T *ch; + /* This is invoked from the garbage collector, which only runs at a safe + * point. */ + ++safe_to_invoke_callback; + for (ch = first_channel; ch != NULL; ch = ch->ch_next) if (!channel_still_useful(ch) && (ch->ch_copyID & mask) != (copyID & mask)) @@ -453,6 +467,8 @@ free_unused_channels_contents(int copyID, int mask) channel_free_contents(ch); did_free = TRUE; } + + --safe_to_invoke_callback; return did_free; } @@ -1450,6 +1466,9 @@ invoke_callback(channel_T *channel, char_u *callback, partial_T *partial, typval_T rettv; int dummy; + if (safe_to_invoke_callback == 0) + EMSG("INTERNAL: Invoking callback when it is not safe"); + argv[0].v_type = VAR_CHANNEL; argv[0].vval.v_channel = channel; @@ -3515,6 +3534,8 @@ channel_parse_messages(void) int r; int part = PART_SOCK; + ++safe_to_invoke_callback; + /* Only do this message when another message was given, otherwise we get * lots of them. */ if (did_log_msg) @@ -3532,6 +3553,13 @@ channel_parse_messages(void) channel = first_channel; continue; } + if (channel->ch_to_be_freed) + { + channel_free(channel); + /* channel has been freed, start over */ + channel = first_channel; + continue; + } if (channel->ch_refcount == 0 && !channel_still_useful(channel)) { /* channel is no longer useful, free it */ @@ -3572,6 +3600,8 @@ channel_parse_messages(void) redraw_after_callback(); } + --safe_to_invoke_callback; + return ret; } diff --git a/src/structs.h b/src/structs.h index a2b38bffd8..24d819bf5d 100644 --- a/src/structs.h +++ b/src/structs.h @@ -1419,6 +1419,8 @@ struct channel_S { int ch_to_be_closed; /* When TRUE reading or writing failed and * the channel must be closed when it's safe * to invoke callbacks. */ + int ch_to_be_freed; /* When TRUE channel must be freed when it's + * safe to invoke callbacks. */ int ch_error; /* When TRUE an error was reported. Avoids * giving pages full of error messages when * the other side has exited, only mention the diff --git a/src/version.c b/src/version.c index 373aff2dd0..266e591488 100644 --- a/src/version.c +++ b/src/version.c @@ -753,6 +753,8 @@ static char *(features[]) = static int included_patches[] = { /* Add new patch number below this line */ +/**/ + 1827, /**/ 1826, /**/ From e0f76d00979c972329f6c371463a20da61ccad65 Mon Sep 17 00:00:00 2001 From: Bram Moolenaar Date: Mon, 9 May 2016 20:38:53 +0200 Subject: [PATCH 3/4] patch 7.4.1828 Problem: May try to access buffer that's already freed. Solution: When freeing a buffer remove it from any channel. --- src/buffer.c | 3 ++ src/channel.c | 67 ++++++++++++++++++++++++++++++++++--------- src/proto/channel.pro | 1 + src/version.c | 2 ++ 4 files changed, 59 insertions(+), 14 deletions(-) diff --git a/src/buffer.c b/src/buffer.c index 9bc24bc126..e884f55dd7 100644 --- a/src/buffer.c +++ b/src/buffer.c @@ -676,6 +676,9 @@ free_buffer(buf_T *buf) #ifdef FEAT_RUBY ruby_buffer_free(buf); #endif +#ifdef FEAT_JOB_CHANNEL + channel_buffer_free(buf); +#endif #ifdef FEAT_AUTOCMD aubuflocal_remove(buf); if (autocmd_busy) diff --git a/src/channel.c b/src/channel.c index a7f4c4b8cb..c191c2a0ff 100644 --- a/src/channel.c +++ b/src/channel.c @@ -1068,6 +1068,7 @@ channel_set_job(channel_T *channel, job_T *job, jobopt_T *options) /* * Find a buffer matching "name" or create a new one. + * Returns NULL if there is something very wrong (error already reported). */ static buf_T * find_buffer(char_u *name, int err) @@ -1081,6 +1082,8 @@ find_buffer(char_u *name, int err) { buf = buflist_new(name == NULL || *name == NUL ? NULL : name, NULL, (linenr_T)0, BLN_LISTED); + if (buf == NULL) + return NULL; buf_copy_options(buf, BCO_ENTER); curbuf = buf; #ifdef FEAT_QUICKFIX @@ -1187,37 +1190,54 @@ channel_set_options(channel_T *channel, jobopt_T *opt) if ((opt->jo_set & JO_OUT_IO) && opt->jo_io[PART_OUT] == JIO_BUFFER) { + buf_T *buf; + /* writing output to a buffer. Default mode is NL. */ if (!(opt->jo_set & JO_OUT_MODE)) channel->ch_part[PART_OUT].ch_mode = MODE_NL; if (opt->jo_set & JO_OUT_BUF) - channel->ch_part[PART_OUT].ch_buffer = - buflist_findnr(opt->jo_io_buf[PART_OUT]); + { + buf = buflist_findnr(opt->jo_io_buf[PART_OUT]); + if (buf == NULL) + EMSGN(_(e_nobufnr), (long)opt->jo_io_buf[PART_OUT]); + } else - channel->ch_part[PART_OUT].ch_buffer = - find_buffer(opt->jo_io_name[PART_OUT], FALSE); - ch_logs(channel, "writing out to buffer '%s'", - (char *)channel->ch_part[PART_OUT].ch_buffer->b_ffname); + { + buf = find_buffer(opt->jo_io_name[PART_OUT], FALSE); + } + if (buf != NULL) + { + ch_logs(channel, "writing out to buffer '%s'", + (char *)buf->b_ffname); + channel->ch_part[PART_OUT].ch_buffer = buf; + } } if ((opt->jo_set & JO_ERR_IO) && (opt->jo_io[PART_ERR] == JIO_BUFFER || (opt->jo_io[PART_ERR] == JIO_OUT && (opt->jo_set & JO_OUT_IO) && opt->jo_io[PART_OUT] == JIO_BUFFER))) { + buf_T *buf; + /* writing err to a buffer. Default mode is NL. */ if (!(opt->jo_set & JO_ERR_MODE)) channel->ch_part[PART_ERR].ch_mode = MODE_NL; if (opt->jo_io[PART_ERR] == JIO_OUT) - channel->ch_part[PART_ERR].ch_buffer = - channel->ch_part[PART_OUT].ch_buffer; + buf = channel->ch_part[PART_OUT].ch_buffer; else if (opt->jo_set & JO_ERR_BUF) - channel->ch_part[PART_ERR].ch_buffer = - buflist_findnr(opt->jo_io_buf[PART_ERR]); + { + buf = buflist_findnr(opt->jo_io_buf[PART_ERR]); + if (buf == NULL) + EMSGN(_(e_nobufnr), (long)opt->jo_io_buf[PART_ERR]); + } else - channel->ch_part[PART_ERR].ch_buffer = - find_buffer(opt->jo_io_name[PART_ERR], TRUE); - ch_logs(channel, "writing err to buffer '%s'", - (char *)channel->ch_part[PART_ERR].ch_buffer->b_ffname); + buf = find_buffer(opt->jo_io_name[PART_ERR], TRUE); + if (buf != NULL) + { + ch_logs(channel, "writing err to buffer '%s'", + (char *)buf->b_ffname); + channel->ch_part[PART_ERR].ch_buffer = buf; + } } channel->ch_part[PART_OUT].ch_io = opt->jo_io[PART_OUT]; @@ -1387,6 +1407,25 @@ channel_write_in(channel_T *channel) buf->b_ml.ml_line_count - lnum + 1); } +/* + * Handle buffer "buf" beeing freed, remove it from any channels. + */ + void +channel_buffer_free(buf_T *buf) +{ + channel_T *channel; + int part; + + for (channel = first_channel; channel != NULL; channel = channel->ch_next) + for (part = PART_SOCK; part <= PART_IN; ++part) + { + chanpart_T *ch_part = &channel->ch_part[part]; + + if (ch_part->ch_buffer == buf) + ch_part->ch_buffer = NULL; + } +} + /* * Write any lines waiting to be written to a channel. */ diff --git a/src/proto/channel.pro b/src/proto/channel.pro index 5e0bec54b5..5dc512181f 100644 --- a/src/proto/channel.pro +++ b/src/proto/channel.pro @@ -14,6 +14,7 @@ void channel_set_pipes(channel_T *channel, sock_T in, sock_T out, sock_T err); void channel_set_job(channel_T *channel, job_T *job, jobopt_T *options); void channel_set_options(channel_T *channel, jobopt_T *opt); void channel_set_req_callback(channel_T *channel, int part, char_u *callback, partial_T *partial, int id); +void channel_buffer_free(buf_T *buf); void channel_write_any_lines(void); void channel_write_new_lines(buf_T *buf); char_u *channel_get(channel_T *channel, int part); diff --git a/src/version.c b/src/version.c index 266e591488..fb3b3d48b9 100644 --- a/src/version.c +++ b/src/version.c @@ -753,6 +753,8 @@ static char *(features[]) = static int included_patches[] = { /* Add new patch number below this line */ +/**/ + 1828, /**/ 1827, /**/ From de7eb0a47b557eb4656c6b63d421c7e7bae1ef30 Mon Sep 17 00:00:00 2001 From: Bram Moolenaar Date: Mon, 9 May 2016 20:54:33 +0200 Subject: [PATCH 4/4] patch 7.4.1829 Problem: No message on channel log when buffer was freed. Solution: Log a message. --- src/channel.c | 6 ++++++ src/version.c | 2 ++ 2 files changed, 8 insertions(+) diff --git a/src/channel.c b/src/channel.c index c191c2a0ff..aca31a265d 100644 --- a/src/channel.c +++ b/src/channel.c @@ -63,6 +63,8 @@ static int channel_need_redraw = FALSE; * is safe to invoke callbacks. */ static int safe_to_invoke_callback = 0; +static char *part_names[] = {"sock", "out", "err", "in"}; + #ifdef WIN32 static int fd_read(sock_T fd, char *buf, size_t len) @@ -1422,7 +1424,11 @@ channel_buffer_free(buf_T *buf) chanpart_T *ch_part = &channel->ch_part[part]; if (ch_part->ch_buffer == buf) + { + ch_logs(channel, "%s buffer has been wiped out", + part_names[part]); ch_part->ch_buffer = NULL; + } } } diff --git a/src/version.c b/src/version.c index fb3b3d48b9..42bb079824 100644 --- a/src/version.c +++ b/src/version.c @@ -753,6 +753,8 @@ static char *(features[]) = static int included_patches[] = { /* Add new patch number below this line */ +/**/ + 1829, /**/ 1828, /**/