On Sat, Aug 22, 2015 at 10:04 AM, Bram Moolenaar <b...@moolenaar.net> wrote:
>
> James Kolb wrote:
>
> > On Friday, August 21, 2015 at 1:39:10 PM UTC-4, Bram Moolenaar wrote:
> > > James Kolb wrote:
> > >
> > > > The current regex code may call mch_breakcheck which can process X
> events.
> > > > One of these events could be a remote_expr that makes its own call
> into the
> > > > regex engine. This usually crashes because the regex engine isn't
> > > > reentrant. This is probably also a problem for anything else that
> runs long
> > > > enough to make breakchecks and isn't reentrant.
> > > >
> > > > This crash can usually be reproduced in linux by running vim with a
> > > > --servername argument and typing the command:
> > > > :call system("sleep 1 && vim --servername ".v:servername."
> --remote-expr
> > > > 'substitute(string(range(5000)), \"a\", \"b\", \"g\")' &") | call
> > > > substitute(string(range(5000)), '\(,.*\)\@<!,', '', 'g')
> > > >
> > > > The attached patch fixes the problem by preventing RealWaitForChar
> from
> > > > processing X events if it is called from mch_breakcheck.
> > >
> > > Thanks for the patch. However, I think the proper solution would be to
> > > make the regexp code reentrant. That means getting rid of the global
> > > variables. It's been messy like that for a long time.
> > >
> > > I think your patch fixes one specific situation, but it can happen in
> > > other situations as well.
> >
> > I agree that the regexp code should be reentrant, but I think regexes
> > are just one mine in a larger breakchecks-can-run-arbitrary-code
> > minefield.
> >
> > Most of the places that call breakchecks don't seem to assume that
> > they can call arbitrary vim commands. Commonly run functions like
> > buflist_list() or searchit(), for example, can read-after-free if
> > somebody deletes a buffer using remote-expr. Anything that calls the
> > regex engine will have the same problem that the regex engine can call
> > arbitrary commands, even if the regex engine were reentrant.
>
> OK, thus even though your patch refers to regexp, it has a wider target.
>
> I'm not sure if your patch also introduces a problem. If we don't
> handle X events while busy doing something, the application becomes
> unresponsive from an X server point of view. E.g. for releasing the
> selection.
>
> If we are calling breakcheck, then we do not expect all kinds of things
> to happen. Some kind of global lock, or a queue to put postponed
> commands on would work better. The netbeans feature already does this,
> we could build on top of it. Specifically netbeans_parse_messages().
>
I wrote a new patch that adds queuing for clientserver similar to the
netbeans queue.
--
--
You received this message from the "vim_dev" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php
---
You received this message because you are subscribed to the Google Groups
"vim_dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to vim_dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
diff --git a/src/ex_docmd.c b/src/ex_docmd.c
index 7633d54..5fc8014 100644
--- a/src/ex_docmd.c
+++ b/src/ex_docmd.c
@@ -8984,11 +8984,11 @@ do_sleep(msec)
{
ui_delay(msec - done > 1000L ? 1000L : msec - done, TRUE);
ui_breakcheck();
-#ifdef FEAT_NETBEANS_INTG
- /* Process the netbeans messages that may have been received in the
- * call to ui_breakcheck() when the GUI is in use. This may occur when
- * running a test case. */
- netbeans_parse_messages();
+#if (defined(FEAT_NETBEANS_INTG) || defined(FEAT_CLIENTSERVER))
+ /* Process the netbeans and clientserver messages that may have been
+ * received in the call to ui_breakcheck() when the GUI is in use. This
+ * may occur when running a test case. */
+ parse_queued_messages();
#endif
}
}
diff --git a/src/getchar.c b/src/getchar.c
index a80432f..31128ec 100644
--- a/src/getchar.c
+++ b/src/getchar.c
@@ -3034,9 +3034,8 @@ inchar(buf, maxlen, wait_time, tb_change_cnt)
)
{
-#if defined(FEAT_NETBEANS_INTG)
- /* Process the queued netbeans messages. */
- netbeans_parse_messages();
+#if (defined(FEAT_NETBEANS_INTG) || defined(FEAT_CLIENTSERVER))
+ parse_queued_messages();
#endif
if (got_int || (script_char = getc(scriptin[curscript])) < 0)
diff --git a/src/gui_gtk_x11.c b/src/gui_gtk_x11.c
index 5538446..910508e 100644
--- a/src/gui_gtk_x11.c
+++ b/src/gui_gtk_x11.c
@@ -650,7 +650,7 @@ property_event(GtkWidget *widget,
xev.xproperty.atom = commProperty;
xev.xproperty.window = commWindow;
xev.xproperty.state = PropertyNewValue;
- serverEventProc(GDK_WINDOW_XDISPLAY(widget->window), &xev);
+ serverEventProc(GDK_WINDOW_XDISPLAY(widget->window), &xev, 0);
}
return FALSE;
}
@@ -5471,9 +5471,8 @@ gui_mch_wait_for_chars(long wtime)
focus = gui.in_focus;
}
-#if defined(FEAT_NETBEANS_INTG)
- /* Process any queued netbeans messages. */
- netbeans_parse_messages();
+#if (defined(FEAT_NETBEANS_INTG) || defined(FEAT_CLIENTSERVER))
+ parse_queued_messages();
#endif
/*
diff --git a/src/gui_w48.c b/src/gui_w48.c
index 1096ec8..e56a17c 100644
--- a/src/gui_w48.c
+++ b/src/gui_w48.c
@@ -2016,9 +2016,8 @@ gui_mch_wait_for_chars(int wtime)
s_need_activate = FALSE;
}
-#ifdef FEAT_NETBEANS_INTG
- /* Process the queued netbeans messages. */
- netbeans_parse_messages();
+#if (defined(FEAT_NETBEANS_INTG) || defined(FEAT_CLIENTSERVER))
+ parse_queued_messages();
#endif
/*
diff --git a/src/gui_x11.c b/src/gui_x11.c
index ed71b26..8b7dad5 100644
--- a/src/gui_x11.c
+++ b/src/gui_x11.c
@@ -2895,9 +2895,8 @@ gui_mch_wait_for_chars(wtime)
focus = gui.in_focus;
}
-#if defined(FEAT_NETBEANS_INTG)
- /* Process any queued netbeans messages. */
- netbeans_parse_messages();
+#if (defined(FEAT_NETBEANS_INTG) || defined(FEAT_CLIENTSERVER))
+ parse_queued_messages();
#endif
/*
@@ -3199,7 +3198,7 @@ gui_x11_send_event_handler(w, client_data, event, dum)
if (e->type == PropertyNotify && e->window == commWindow
&& e->atom == commProperty && e->state == PropertyNewValue)
{
- serverEventProc(gui.dpy, event);
+ serverEventProc(gui.dpy, event, 0);
}
}
#endif
diff --git a/src/if_xcmdsrv.c b/src/if_xcmdsrv.c
index d8c64ad..11a8ee8 100644
--- a/src/if_xcmdsrv.c
+++ b/src/if_xcmdsrv.c
@@ -169,6 +169,18 @@ enum ServerReplyOp { SROP_Find, SROP_Add, SROP_Delete };
typedef int (*EndCond) __ARGS((void *));
+struct x_cmdqueue
+{
+ char_u *buffer;
+ int len;
+ struct x_cmdqueue *next;
+ struct x_cmdqueue *prev;
+};
+
+typedef struct x_cmdqueue x_queue_T;
+
+static x_queue_T head; /* dummy node, header for circular queue */
+
/*
* Forward declarations for procedures defined later in this file:
*/
@@ -186,6 +198,8 @@ static struct ServerReply *ServerReplyFind __ARGS((Window
w, enum ServerReplyOp
static int AppendPropCarefully __ARGS((Display *display, Window window,
Atom property, char_u *value, int length));
static int x_error_check __ARGS((Display *dpy, XErrorEvent *error_event));
static int IsSerialName __ARGS((char_u *name));
+static void save_in_queue __ARGS((char_u *buf, int len));
+static void server_parse_message __ARGS((Display *dpy, char_u* propInfo,
int numItems));
/* Private variables for the "server" functionality */
static Atom registryProperty = None;
@@ -595,7 +609,7 @@ ServerWait(dpy, w, endCond, endData, localLoop, seconds)
while (TRUE)
{
while (XCheckWindowEvent(dpy, commWindow, PropertyChangeMask, &event))
- serverEventProc(dpy, &event);
+ serverEventProc(dpy, &event, 1);
if (endCond(endData) != 0)
break;
@@ -1127,22 +1141,25 @@ GetRegProp(dpy, regPropp, numItemsp, domsg)
return OK;
}
+
/*
* This procedure is invoked by the various X event loops throughout Vims when
* a property changes on the communication window. This procedure reads the
- * property and handles command requests and responses.
+ * property and enqueues command requests and responses. If immediate is true,
+ * it runs the event immediatly instead of enqueuing it. Immediate can cause
+ * unintended behavior and should only be used for code that blocks for a
+ * response.
*/
void
-serverEventProc(dpy, eventPtr)
+serverEventProc(dpy, eventPtr, immediate)
Display *dpy;
XEvent *eventPtr; /* Information about event. */
+ int immediate; /* Run event immediately. Should
usually be 0. */
{
char_u *propInfo;
- char_u *p;
- int result, actualFormat, code;
+ int result, actualFormat;
long_u numItems, bytesAfter;
Atom actualType;
- char_u *tofree;
if (eventPtr != NULL)
{
@@ -1168,7 +1185,99 @@ serverEventProc(dpy, eventPtr)
XFree(propInfo);
return;
}
+ if (immediate)
+ {
+ server_parse_message(dpy, propInfo, numItems);
+ }
+ else
+ {
+ save_in_queue(propInfo, numItems);
+ }
+}
+
+/*
+ * Saves x clientserver commands in a queue so that they can be called when
vim is idle.
+ */
+ static void
+save_in_queue(buf, len)
+ char_u *buf;
+ int len;
+{
+ x_queue_T *node;
+
+ node = (x_queue_T *)alloc(sizeof(x_queue_T));
+ if (node == NULL)
+ return; /* out of memory */
+ node->buffer = alloc(len + 1);
+ if (node->buffer == NULL)
+ {
+ vim_free(node);
+ return; /* out of memory */
+ }
+ mch_memmove(node->buffer, buf, (size_t)len);
+ node->buffer[len] = NUL;
+ node->len = len;
+
+ if (head.next == NULL) /* initialize circular queue */
+ {
+ head.next = &head;
+ head.prev = &head;
+ }
+
+ /* insert node at tail of queue */
+ node->next = &head;
+ node->prev = head.prev;
+ head.prev->next = node;
+ head.prev = node;
+}
+
+/*
+ * Parses queued clientserver messages.
+ */
+ void
+server_parse_messages()
+{
+ char_u *p;
+ x_queue_T *node;
+ int own_node;
+
+ while (head.next != NULL && head.next != &head)
+ {
+ node = head.next;
+ if(!X_DISPLAY)
+ {
+ return;
+ }
+ server_parse_message(X_DISPLAY, node->buffer, node->len);
+ head.next = node->next;
+ node->next->prev = node->prev;
+ vim_free(node);
+ }
+}
+/*
+ * Returns a non-zero value if there are clientserver messages waiting
+ * int the queue.
+ */
+ int
+server_waiting()
+{
+ return head.next != NULL && head.next != &head;
+}
+
+/*
+ * Prases a single clientserver message. A single message may contain multiple
+ * commands.
+ */
+ static void
+server_parse_message(dpy, propInfo, numItems)
+ Display *dpy;
+ char_u* propInfo; //A string containing 0 or more X commands
+ int numItems; //The size of propInfo in bytes.
+{
+ char_u *p;
+ int code;
+ char_u *tofree;
/*
* Several commands and results could arrive in the property at
* one time; each iteration through the outer loop handles a
diff --git a/src/misc2.c b/src/misc2.c
index 379916b..33c1c9d 100644
--- a/src/misc2.c
+++ b/src/misc2.c
@@ -6328,3 +6328,21 @@ has_non_ascii(s)
return FALSE;
}
#endif
+
+/*
+ * Process messages that have been queued for netbeans or clientserver.
+ * These functions can call arbitrary vimscript and should only be called when
+ * it is safe to do so.
+ */
+ void
+parse_queued_messages()
+{
+#ifdef FEAT_NETBEANS_INTG
+ /* Process the queued netbeans messages. */
+ netbeans_parse_messages();
+#endif
+#ifdef FEAT_CLIENTSERVER
+ /* Process the queued clientserver messages. */
+ server_parse_messages();
+#endif
+}
diff --git a/src/os_unix.c b/src/os_unix.c
index 2439c5e..55812c0 100644
--- a/src/os_unix.c
+++ b/src/os_unix.c
@@ -388,9 +388,8 @@ mch_inchar(buf, maxlen, wtime, tb_change_cnt)
{
int len;
-#ifdef FEAT_NETBEANS_INTG
- /* Process the queued netbeans messages. */
- netbeans_parse_messages();
+#if (defined(FEAT_NETBEANS_INTG) || defined(FEAT_CLIENTSERVER))
+ parse_queued_messages();
#endif
/* Check if window changed size while we were busy, perhaps the ":set
@@ -405,9 +404,8 @@ mch_inchar(buf, maxlen, wtime, tb_change_cnt)
if (!do_resize) /* return if not interrupted by resize */
return 0;
handle_resize();
-#ifdef FEAT_NETBEANS_INTG
- /* Process the queued netbeans messages. */
- netbeans_parse_messages();
+#if (defined(FEAT_NETBEANS_INTG) || defined(FEAT_CLIENTSERVER))
+ parse_queued_messages();
#endif
}
}
@@ -439,9 +437,8 @@ mch_inchar(buf, maxlen, wtime, tb_change_cnt)
while (do_resize) /* window changed size */
handle_resize();
-#ifdef FEAT_NETBEANS_INTG
- /* Process the queued netbeans messages. */
- netbeans_parse_messages();
+#if (defined(FEAT_NETBEANS_INTG) || defined(FEAT_CLIENTSERVER))
+ parse_queued_messages();
#endif
/*
* We want to be interrupted by the winch signal
@@ -5208,6 +5205,7 @@ WaitForChar(msec)
* When a GUI is being used, this will not be used for input -- webb
* Returns also, when a request from Sniff is waiting -- toni.
* Or when a Linux GPM mouse event is waiting.
+ * Or when a clientserver message is on the queue.
*/
#if defined(__BEOS__)
int
@@ -5601,6 +5599,11 @@ select_eintr:
if (finished || msec == 0)
break;
+#ifdef FEAT_CLIENTSERVER
+ if (server_waiting())
+ break;
+#endif
+
/* We're going to loop around again, find out for how long */
if (msec > 0)
{
@@ -7121,7 +7124,7 @@ xterm_update()
if (e->type == PropertyNotify && e->window == commWindow
&& e->atom == commProperty && e->state == PropertyNewValue)
- serverEventProc(xterm_dpy, &event);
+ serverEventProc(xterm_dpy, &event, 0);
}
#endif
XtDispatchEvent(&event);
diff --git a/src/proto/if_xcmdsrv.pro b/src/proto/if_xcmdsrv.pro
index dd6a120..fb937e5 100644
--- a/src/proto/if_xcmdsrv.pro
+++ b/src/proto/if_xcmdsrv.pro
@@ -7,5 +7,5 @@ Window serverStrToWin __ARGS((char_u *str));
int serverSendReply __ARGS((char_u *name, char_u *str));
int serverReadReply __ARGS((Display *dpy, Window win, char_u **str, int
localLoop));
int serverPeekReply __ARGS((Display *dpy, Window win, char_u **str));
-void serverEventProc __ARGS((Display *dpy, XEvent *eventPtr));
+void serverEventProc __ARGS((Display *dpy, XEvent *eventPtr, int immediate));
/* vim: set ft=c : */
