Patch 8.0.0737

[Bram Moolenaar]

Patch 8.0.0737
Problem: Crash when X11 selection is very big.
Solution: Use static items instead of allocating them. Add callbacks.
(Ozaki Kiichi)
Files: src/testdir/shared.vim, src/testdir/test_quotestar.vim,
src/ui.c


*** ../vim-8.0.0736/src/testdir/shared.vim 2017-03-18 16:18:25.099693814 +0100
--- src/testdir/shared.vim 2017-07-19 19:16:11.235499909 +0200
***************
*** 111,124 ****
" Wait for up to a second for "expr" to become true.
" Return time slept in milliseconds. With the +reltime feature this can be
" more than the actual waiting time. Without +reltime it can also be less.
! func WaitFor(expr)
" using reltime() is more accurate, but not always available
if has('reltime')
let start = reltime()
else
let slept = 0
endif
! for i in range(100)
try
if eval(a:expr)
if has('reltime')
--- 111,125 ----
" Wait for up to a second for "expr" to become true.
" Return time slept in milliseconds. With the +reltime feature this can be
" more than the actual waiting time. Without +reltime it can also be less.
! func WaitFor(expr, ...)
! let timeout = get(a:000, 0, 1000)
" using reltime() is more accurate, but not always available
if has('reltime')
let start = reltime()
else
let slept = 0
endif
! for i in range(timeout / 10)
try
if eval(a:expr)
if has('reltime')
***************
*** 133,139 ****
endif
sleep 10m
endfor
! return 1000
endfunc

" Wait for up to a given milliseconds.
--- 134,140 ----
endif
sleep 10m
endfor
! return timeout
endfunc

" Wait for up to a given milliseconds.
*** ../vim-8.0.0736/src/testdir/test_quotestar.vim 2017-06-10 16:30:27.965876961 +0200
--- src/testdir/test_quotestar.vim 2017-07-19 19:16:11.235499909 +0200
***************
*** 88,93 ****
--- 88,105 ----
call WaitFor('@* == "yes"')
call assert_equal('yes', @*)

+ " Handle the large selection over 262040 byte.
+ let length = 262044
+ let sample = 'a' . repeat('b', length - 2) . 'c'
+ let @* = sample
+ call WaitFor('remote_expr("' . name . '", "len(@*) >= ' . length . '", "", 1)', 3000)
+ let res = remote_expr(name, "@*", "", 2)
+ call assert_equal(length, len(res))
+ " Check length to prevent a large amount of output at assertion failure.
+ if length == len(res)
+ call assert_equal(sample, res)
+ endif
+
if has('unix') && has('gui') && !has('gui_running')
let @* = ''

*** ../vim-8.0.0736/src/ui.c 2017-03-29 19:20:25.385015086 +0200
--- src/ui.c 2017-07-19 19:26:34.987051580 +0200
***************
*** 2042,2051 ****
* X Selection stuff, for cutting and pasting text to other windows.
*/

! static Boolean clip_x11_convert_selection_cb(Widget, Atom *, Atom *, Atom *, XtPointer *, long_u *, int *);
! static void clip_x11_lose_ownership_cb(Widget, Atom *);
static void clip_x11_timestamp_cb(Widget w, XtPointer n, XEvent *event, Boolean *cont);
! static void clip_x11_request_selection_cb(Widget, XtPointer, Atom *, Atom *, XtPointer, long_u *, int *);

/*
* Property callback to get a timestamp for XtOwnSelection.
--- 2042,2052 ----
* X Selection stuff, for cutting and pasting text to other windows.
*/

! static Boolean clip_x11_convert_selection_cb(Widget w, Atom *sel_atom, Atom *target, Atom *type, XtPointer *value, long_u *length, int *format);
! static void clip_x11_lose_ownership_cb(Widget w, Atom *sel_atom);
! static void clip_x11_notify_cb(Widget w, Atom *sel_atom, Atom *target);
static void clip_x11_timestamp_cb(Widget w, XtPointer n, XEvent *event, Boolean *cont);
! static void clip_x11_request_selection_cb(Widget w, XtPointer success, Atom *sel_atom, Atom *type, XtPointer value, long_u *length, int *format);

/*
* Property callback to get a timestamp for XtOwnSelection.
***************
*** 2085,2091 ****
/* Get the selection, using the event timestamp. */
if (XtOwnSelection(w, xproperty->atom, xproperty->time,
clip_x11_convert_selection_cb, clip_x11_lose_ownership_cb,
! NULL) == OK)
{
/* Set the "owned" flag now, there may have been a call to
* lose_ownership_cb in between. */
--- 2086,2092 ----
/* Get the selection, using the event timestamp. */
if (XtOwnSelection(w, xproperty->atom, xproperty->time,
clip_x11_convert_selection_cb, clip_x11_lose_ownership_cb,
! clip_x11_notify_cb) == OK)
{
/* Set the "owned" flag now, there may have been a call to
* lose_ownership_cb in between. */
***************
*** 2276,2284 ****
start_time = time(NULL);
while (success == MAYBE)
{
! if (XCheckTypedEvent(dpy, SelectionNotify, &event)
! || XCheckTypedEvent(dpy, SelectionRequest, &event)
! || XCheckTypedEvent(dpy, PropertyNotify, &event))
{
/* This is where clip_x11_request_selection_cb() should be
* called. It may actually happen a bit later, so we loop
--- 2277,2285 ----
start_time = time(NULL);
while (success == MAYBE)
{
! if (XCheckTypedEvent(dpy, PropertyNotify, &event)
! || XCheckTypedEvent(dpy, SelectionNotify, &event)
! || XCheckTypedEvent(dpy, SelectionRequest, &event))
{
/* This is where clip_x11_request_selection_cb() should be
* called. It may actually happen a bit later, so we loop
***************
*** 2331,2341 ****
long_u *length,
int *format)
{
! char_u *string;
! char_u *result;
! int motion_type;
! VimClipboard *cbd;
! int i;

if (*sel_atom == clip_plus.sel_atom)
cbd = &clip_plus;
--- 2332,2343 ----
long_u *length,
int *format)
{
! static char_u *save_result = NULL;
! static long_u save_length = 0;
! char_u *string;
! int motion_type;
! VimClipboard *cbd;
! int i;

if (*sel_atom == clip_plus.sel_atom)
cbd = &clip_plus;
***************
*** 2348,2357 ****
/* requestor wants to know what target types we support */
if (*target == targets_atom)
{
! Atom *array;

- if ((array = (Atom *)XtMalloc((unsigned)(sizeof(Atom) * 7))) == NULL)
- return False;
*value = (XtPointer)array;
i = 0;
array[i++] = targets_atom;
--- 2350,2357 ----
/* requestor wants to know what target types we support */
if (*target == targets_atom)
{
! static Atom array[7];

*value = (XtPointer)array;
i = 0;
array[i++] = targets_atom;
***************
*** 2400,2412 ****
*length += STRLEN(p_enc) + 2;
#endif

! *value = XtMalloc((Cardinal)*length);
! result = (char_u *)*value;
! if (result == NULL)
{
vim_free(string);
return False;
}

if (*target == XA_STRING
#ifdef FEAT_MBYTE
--- 2400,2416 ----
*length += STRLEN(p_enc) + 2;
#endif

! if (save_length < *length || save_length / 2 >= *length)
! *value = XtRealloc((char *)save_result, (Cardinal)*length + 1);
! else
! *value = save_result;
! if (*value == NULL)
{
vim_free(string);
return False;
}
+ save_result = (char_u *)*value;
+ save_length = *length;

if (*target == XA_STRING
#ifdef FEAT_MBYTE
***************
*** 2414,2426 ****
#endif
)
{
! mch_memmove(result, string, (size_t)(*length));
*type = *target;
}
else if (*target == compound_text_atom || *target == text_atom)
{
XTextProperty text_prop;
! char *string_nt = (char *)alloc((unsigned)*length + 1);
int conv_result;

/* create NUL terminated string which XmbTextListToTextProperty wants */
--- 2418,2430 ----
#endif
)
{
! mch_memmove(save_result, string, (size_t)(*length));
*type = *target;
}
else if (*target == compound_text_atom || *target == text_atom)
{
XTextProperty text_prop;
! char *string_nt = (char *)save_result;
int conv_result;

/* create NUL terminated string which XmbTextListToTextProperty wants */
***************
*** 2428,2435 ****
string_nt[*length] = NUL;
conv_result = XmbTextListToTextProperty(X_DISPLAY, (char **)&string_nt,
1, XCompoundTextStyle, &text_prop);
- vim_free(string_nt);
- XtFree(*value); /* replace with COMPOUND text */
if (conv_result != Success)
{
vim_free(string);
--- 2432,2437 ----
***************
*** 2438,2461 ****
*value = (XtPointer)(text_prop.value); /* from plain text */
*length = text_prop.nitems;
*type = compound_text_atom;
}
-
#ifdef FEAT_MBYTE
else if (*target == vimenc_atom)
{
int l = STRLEN(p_enc);

! result[0] = motion_type;
! STRCPY(result + 1, p_enc);
! mch_memmove(result + l + 2, string, (size_t)(*length - l - 2));
*type = vimenc_atom;
}
#endif
-
else
{
! result[0] = motion_type;
! mch_memmove(result + 1, string, (size_t)(*length - 1));
*type = vim_atom;
}
*format = 8; /* 8 bits per char */
--- 2440,2464 ----
*value = (XtPointer)(text_prop.value); /* from plain text */
*length = text_prop.nitems;
*type = compound_text_atom;
+ XtFree((char *)save_result);
+ save_result = (char_u *)*value;
+ save_length = *length;
}
#ifdef FEAT_MBYTE
else if (*target == vimenc_atom)
{
int l = STRLEN(p_enc);

! save_result[0] = motion_type;
! STRCPY(save_result + 1, p_enc);
! mch_memmove(save_result + l + 2, string, (size_t)(*length - l - 2));
*type = vimenc_atom;
}
#endif
else
{
! save_result[0] = motion_type;
! mch_memmove(save_result + 1, string, (size_t)(*length - 1));
*type = vim_atom;
}
*format = 8; /* 8 bits per char */
***************
*** 2479,2484 ****
--- 2482,2493 ----
XtLastTimestampProcessed(XtDisplay(myShell)));
}

+ static void
+ clip_x11_notify_cb(Widget w UNUSED, Atom *sel_atom UNUSED, Atom *target UNUSED)
+ {
+ /* To prevent automatically freeing the selection value. */
+ }
+
int
clip_x11_own_selection(Widget myShell, VimClipboard *cbd)
{
***************
*** 2492,2498 ****
if (XtOwnSelection(myShell, cbd->sel_atom,
XtLastTimestampProcessed(XtDisplay(myShell)),
clip_x11_convert_selection_cb, clip_x11_lose_ownership_cb,
! NULL) == False)
return FAIL;
}
else
--- 2501,2507 ----
if (XtOwnSelection(myShell, cbd->sel_atom,
XtLastTimestampProcessed(XtDisplay(myShell)),
clip_x11_convert_selection_cb, clip_x11_lose_ownership_cb,
! clip_x11_notify_cb) == False)
return FAIL;
}
else
*** ../vim-8.0.0736/src/version.c 2017-07-19 18:18:27.828135659 +0200
--- src/version.c 2017-07-19 19:18:49.030374635 +0200
***************
*** 771,772 ****
--- 771,774 ----
{ /* Add new patch number below this line */
+ /**/
+ 737,
/**/

--
A computer program does what you tell it to do, not what you want it to do.

/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[Kazunobu Kuriyama]

2017-07-20 2:57 GMT+09:00 Bram Moolenaar <Br...@moolenaar.net>:

Patch 8.0.0737
Problem:    Crash when X11 selection is very big.
Solution:   Use static items instead of allocating them.  Add callbacks.
            (Ozaki Kiichi)
Files:      src/testdir/shared.vim, src/testdir/test_quotestar.vim,
            src/ui.c

<snip>

Bram,

I'm afraid you made a decision too early.

This patch has outstanding problems. They are difficult to explain briefly, but let me try doing that.

First, the patch seems to prevent Vim from crashing by simply changing the order of the calls of XCheckTypedEvent().  This happens to work for the purpose because the call for PropertyNotify happens to catch the INCL PropertyNotify which is to be sent to the requestor prior to the actual data transaction when the selection is large relative to max-request-size (implementation dependent).  The event is, however, not handled properly because, according to ICCCM, the property has to be deleted, yet never by Vim actually.  We have a server resource leak here.

Secondly, the same manual also says that after deleting the property, the transaction of the body of the selection data begins and then finishes with the owner's sending a zero-length property to the requestor.  It's requestor's responsibility to delete the zero-length property; however, Vim will never send it.  Another resource leak takes place in the peer client.

Lastly, since Vim didn't know what the zero-length property means, it tried to free the same memory block twice, resulting in crash.   With the patch, the memory block is statically allocated to avoid the crash.  However, if Vim understood the fact that it would get PropertyNotify twice per large selection at the end of the transaction, this change wouldn't be necessary at all.

I think those explain well what is described in the remark made to the TODO :

> X11: Putting more than about 262040 characters of text on the clipboard and

> pasting it in another Vim doesn't work.  (Dominique Pelle, 2008 Aug 21-23)

> clip_x11_request_selection_cb() is called with zero value and length.

> Also: Get an error message from free() in the process that owns the selection.

> Seems to happen when the selection is requested the second time, but before

> clip_x11_convert_selection_cb() is invoked, thus in X library code.

I thought my comment https://github.com/vim/vim/issues/1822#issuecomment-316115383 was sufficient for people to evaluate the patch appropriately, but now I found it was not...

Best regards,

Kazunobu Kuriyama

--
A computer program does what you tell it to do, not what you want it to do.

 /// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net   \\\
///        sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\  an exciting new programming language -- http://www.Zimbu.org        ///
 \\\            help me help AIDS victims -- http://ICCF-Holland.org    ///

--
--
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+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Bram Moolenaar]

I thought this patch at least fixes a crash. I know it's not the final
solution, but since it might still take a while for that, including a
patch that avoids a crash seems like a good intermediate step.

This X11 stuff is just too complicated (they wrote a dozen books of 1000
pages to describe it...). I do hope that you or someone else can
provide a better solution soon.

--
hundred-and-one symptoms of being an internet addict:
194. Your business cards contain your e-mail and home page address.

[Ozaki Kiichi]

> First, the patch seems to prevent Vim from crashing by simply changing the order of the calls of XCheckTypedEvent().  This happens to work for the purpose because the call for PropertyNotify happens to catch the INCL PropertyNotify which is to be sent to the requestor prior to the actual data transaction when the selection is large relative to max-request-size (implementation dependent).  The event is, however, not handled properly because, according to ICCCM, the property has to be deleted, yet never by Vim actually.  We have a server resource leak here.
>
> Secondly, the same manual also says that after deleting the property, the transaction of the body of the selection data begins and then finishes with the owner's sending a zero-length property to the requestor.  It's requestor's responsibility to delete the zero-length property; however, Vim will never send it.  Another resource leak takes place in the peer client.

Changing the order of XCheckTypedEvent() is a fix to paste correctly, not directly related with preventing crash.

I might be wrong but, as far as I browsed libxt code, INCR processes seem to be handled by the inside of libxt.

> Lastly, since Vim didn't know what the zero-length property means, it tried to free the same memory block twice, resulting in crash.   With the patch, the memory block is statically allocated to avoid the crash.  However, if Vim understood the fact that it would get PropertyNotify twice per large selection at the end of the transaction, this change wouldn't be necessary at all.

Does the above description (about zero-length property, PropertyNotify) explain the requestor-side behavior?
A crash happens in the owner side Vim, so I think this kind (owner-side) of fix is need in addition to correcting the way of handling property at requestor.

Note:
Crash when receiving selection-request from some other application.
(Therefore I think owner-side fix is need)

Example: vim (prior than v8.0.0737) and xclip
----
$ vim --clean -c 'h eval.txt'

and input `ggvG$` (on visual mode, selecting all)

and on another terminal:

$ while :; do xclip -o -selection primary | wc -c; sleep 1; done

(when xclip stalls, do Ctrl-C and restart)
----

[Kazunobu Kuriyama]

IMHO, you'd rather need to explain to us first why your patch is OK, which should have been done when you posted it, Because of lack of your explanation, I had to read bulky references, manuals and source code to review it while I was quite busy for repairing and restoring my PC.  Anyway, arguing only about my views won't help to validate your patch. 

There's one thing I didn't write in my previous mail: You included an empty stub as XtSelectionDoneProc in the patch. A manual says, when XtSelectionDoneProc is non-NULL, the selection owner owns the storage containing the converted selection value.  We now have an XtSelectionDoneProc which does nothing but claims the ownership of the storage.  What is this for?  The implication is clear...

[Bram Moolenaar]

My understanding was the combination of making some data static, instead
of allocating it, and using the DoneProc, means that the data won't be
freed unexpectedly.

It would be good to include comments in the code to explain these
things. Especially since it's likely other people go in there trying to
fix their bug or add a new feature.

--
"Marriage is when a man and woman become as one; the trouble starts
when they try to decide which one"

[Kazunobu Kuriyama]

TL;DR  Hmm, seems I should do something about it.  The following lengthy comment is for those who has an interest in technical aspects of the issue...

--------

Xt releases relevant resources in accordance with the conventions of ICCCM.  From Vim's point of view, it might look like Xt frees them arbitrarily, but that's due to Vim's ignorance of the conventions.

Let's start with some preliminary stuff.

At the final phase of a selection data transaction, we need to consider two possible cases because the way of the information exchange varies depending on maximum-request-size (XMaxRequestSize(3)).

1.  The data of a selection is small enough, and hence the transaction is carried out atomically.

In this case, to avoid resource leak, the requestor is supposed to delete a copy of the converted selection data that was sent from the owner when it is done with the copy.  Since the data is an X property, i.e., a server resource, the deletion induces a PropertyNotify(state==Deleted) event.  

The owner who wants to manage the storage of the original copy of the converted selection data for himself (and other resources associated with it, if any) has to select PropertyChangeMask to show an interest in the event and waits for the arrival of it.  On arrival of the event, the owner knows he is now safely able to release the relevant resource(s) since the requestor has implicitly acknowledged receipt of the data by deleting the property.  In terms of Xt, this means that XtSelectionDoneProc is set to non-NULL, and the proc is responsible for the management of the storage for the original copy of the converted selection data.

In summary, the sole PropertyNotify(state==Deleted) event signals the owner to release the resource(s), as there's no other PropertyNotify event.

2.  The data of a selection is large relative to maximum-request-size, and hence the transaction is carried out incrementally.

In this case, the requestor deletes a copy of each segment of the whole converted selection data when he is done with it.  Accordingly, the owner is notified of PropertyNotify(state==Delete) every time the requestor deletes it.

Then, how did the requestor know the transaction would be done incrementally?  How will the owner know whether or not the requestor receives the whole selection data. 

For the former, the owner is supposed to send the INCR property to the requestor prior to sending the actual converted selection data to get the requestor ready for the incremental transfer.  After the owner sends the last segment, he is supposed to send a zero-length property to the requestor to notify him of the dispatch of the last segment.  

For the latter, the requestor is supposed to send a zero-length property to the owner to notify him of receipt of the whole data.

In summary, of several PropertyNotify events, only the zero-length property event signals the owner to release the resource(s), which makes a striking contrast to Case #1.

To make this hypothetical information exchange scenario practical, it is important to note that a timeout mechanism is incorporated into the actual selection mechanism implementation as a defense against communication breakdown.

Lastly, Vim is not implemented to cope with Case #2 yet; it doesn't know about incremental transfers.

That's all for the preliminary stuff! (I hope).

Based on that, we can think of the following as one of various scenarios to disaster.

There's a user on Vim who has visually selected a piece of text which is so large that Xt will have to decide to use an incremental transfer.  Vim is now the owner of a selection. Then another application requests Vim to send the selection to it.  All goes well, and the requestor has just deleted the last segment.

 (I wrote "the last segment", but remember, the requestor actually cannot know if a given segment is the last one until the owner notifies him of that fact using a zero-length property later.)  

Although the requestor only wants to tell Vim that he is done with a segment in question, Vim thinks it as notification that the whole transaction completed, because Vim is still sure all selection transactions are atomic.  Naturally, Vim releases the relevant resource(s) and thinks everything went well.

In the meantime, Xt is watching the development of the whole transaction and waiting for Vim's giving a zero-length property to complete the data transfers on its side, starting clock ticks for timeout (The timeout is set to five seconds by default).   When time is up, Xt aborts the operation forcefully and starts a mandated cleanup, trying to release the resource(s) Vim already freed wrongly... 

It would be good to include comments in the code to explain these
things.  Especially since it's likely other people go in there trying to
fix their bug or add a new feature.

I'll spend my time more for this issue...  Probably, my solution would be to fully implement incremental transfers, which I expect resolves the issue as well as the GitHub issue on clipboard managers.

Best regards,

Kazunobu Kuriyama

[Bram Moolenaar]

Thanks for the information. The X specs are very complicated. I hope
you can find a nice way to make it work.

--
hundred-and-one symptoms of being an internet addict:

259. When you enter your name in the AltaVista search engine, the top ten
matches do indeed refer to you.