2017-07-26 4:35 GMT+09:00 Bram Moolenaar <[email protected]>: > > Kazunobu Kuriyama wrote: > > > 2017-07-25 18:10 GMT+09:00 Ozaki Kiichi <[email protected]>: > > > > > > 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) > > > ---- > > > > 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... > > 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. >
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 -- > "Marriage is when a man and woman become as one; the trouble starts > when they try to decide which one" > > /// Bram Moolenaar -- [email protected] -- 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
