Community
Participate
Working Groups
Created attachment 267097 [details] Crash Dump The JVM crashes during the Drag and Drop of a simple text file. Machine is brand new and should have all the latest patches for windows.
Tried on Win7: I couldn't reproduce this crash by Drag and Drop an external text file into Eclipse or DnD text file across projects. Are you able to consistently reproduce the crash on different machines ? Can you please share the exact steps to reproduce this crash ?
A very close look at our source code revealed that an old DropTargetEvent that was no longer valid potentially caused a wrong memory address to be used. Core of my question would be how to avoid a crash on such an scenario. Wouldn't it be correct to catch the access violation.
(In reply to Jens Kuebler from comment #2) > A very close look at our source code revealed that an old DropTargetEvent > that was no longer valid potentially caused a wrong memory address to be > used. Core of my question would be how to avoid a crash on such an scenario. > Wouldn't it be correct to catch the access violation. That is true. It should throw something rather than crashing. Can you provide a snippet to reproduce this issue or some detailed step-by-step description on how to reporduce it?
Created attachment 267164 [details] Snippet that crashes the vm
(In reply to Jens Kuebler from comment #4) > Created attachment 267164 [details] > Snippet that crashes the vm I tried to drop a text file onto the snippet. Works on Gtk3.22 as well as on a Windows 10 VM. I see a little red square if I drop a text file onto a tree item (when fully expanded). It could be a windows-7 specific issue or maybe an issue with the configuration of your windows 7 vm. If someone with a windows 7 box could test this, it'd be great.
(In reply to Leo Ufimtsev from comment #5) > (In reply to Jens Kuebler from comment #4) > > Created attachment 267164 [details] > > Snippet that crashes the vm > > I tried to drop a text file onto the snippet. > > Works on Gtk3.22 as well as on a Windows 10 VM. I see a little red square if > I drop a text file onto a tree item (when fully expanded). Tested the attached snippet on Win7, I see same behavior as mentioned above and no crash seen.
Created attachment 267192 [details] repro video for dnd crash You need to drag the file onto one red square, then on another tree item and finally on that new red square (see attached video). Note that a SWTError is thrown when dragging the file onto the first square: org.eclipse.swt.SWTError: cought native exception: 0xc0000005 at org.eclipse.swt.internal.win32.OS.VtblCall(Native Method) at org.eclipse.swt.internal.ole.win32.IUnknown.AddRef(IUnknown.java:20) at org.eclipse.swt.dnd.FileTransfer.nativeToJava(FileTransfer.java:113) at Snippet92$Popup$1.dragEnter(Snippet92.java:51) But dragging it onto the second square crashes the VM: j org.eclipse.swt.internal.ole.win32.COM.VtblCall(IJLorg/eclipse/swt/internal/ole/win32/FORMATETC;Lorg/eclipse/swt/internal/ole/win32/STGMEDIUM;)I+0 j org.eclipse.swt.internal.ole.win32.IDataObject.GetData(Lorg/eclipse/swt/internal/ole/win32/FORMATETC;Lorg/eclipse/swt/internal/ole/win32/STGMEDIUM;)I+7 j org.eclipse.swt.dnd.Transfer.getData(Lorg/eclipse/swt/internal/ole/win32/IDataObject;Lorg/eclipse/swt/internal/ole/win32/FORMATETC;Lorg/eclipse/swt/internal/ole/win32/STGMEDIUM;)I+3 j org.eclipse.swt.dnd.FileTransfer.nativeToJava(Lorg/eclipse/swt/dnd/TransferData;)Ljava/lang/Object;+91 j Snippet92$Popup$1.dragEnter(Lorg/eclipse/swt/dnd/DropTargetEvent;)V+16
(In reply to Julian Honnen from comment #7) > Created attachment 267192 [details] > repro video for dnd crash > > You need to drag the file onto one red square, then on another tree item and > finally on that new red square (see attached video). Tried the snippet on Win7/Win10, it doesn't crash immediately. Complex set of dragging operation needs to be performed to reproduce the crash. Not 100% sure if the test-Snippet is missing something, but I agree proper SWTError should be thrown instead of crashing.
New Gerrit change created: https://git.eclipse.org/r/99533
The attached gerrit patch fixes the issue, by testing the given TransferData handle with a call to QueryGetData before actually calling getData. If QueryGetData returns anything else than OK, we should skip the call to getData, as that crashes the VM. The call to getData cannot be surrounded with a native try-catch, because that's too late; the JVM has already crashed. @Niraj: Please review the patch.
(In reply to Conrad Groth from comment #10) > The attached gerrit patch fixes the issue, by testing the given TransferData > handle with a call to QueryGetData before actually calling getData. If > QueryGetData returns anything else than OK, we should skip the call to > getData, as that crashes the VM. The call to getData cannot be surrounded > with a native try-catch, because that's too late; the JVM has already > crashed. > > @Niraj: Please review the patch. Sorry for the delay in reviewing this patch, was occupied with other set of things at my end. Targeting for Photon M3 Given patch doesn't addresses the crash issue for me as tested at Win64bit: https://git.eclipse.org/r/99533 Test case I tried, DnD any file on the Tree widget using the test snippet: I am seeing the crash at line 113 of FileTransfer.java befoe Also note, this DnD crash is only seen at 64bit and not on 32bit windows.
Moved to M4, since given patch doesn't addresses the crash issue for me as tested on Win7.
(In reply to Niraj Modi from comment #12) > Moved to M4, since given patch doesn't addresses the crash issue for me as > tested on Win7. Hi Conrad, Into last week of development for M4, do you plan to look into this ?
Once we have a working patch, will re-target for milestone.
Moving to 4.9, please re-target as required.
Please re-target as required.
Hi Nikita/Alexandr, This one can be reproduced as per comment 8, but tiven patch doesn't addresses the crash issue for me: https://git.eclipse.org/r/99533 Adding you for review on the associated gerrit. Please see if you narrow down on this, we can consider it for 4.12, Thanks!
Unfortunately, the suggested patch doesn't fix anything. The root cause of the crash seems to be use-after-free of the native IDataObject and the resulting memory corruption. Multiple bugs contribute to the problem. 1. Crashing snippet stashes away the DropTargetEvent event (which references the native IDataObject) and later extracts data from it. This is API misuse, at least on Windows, because the native IDataObject is not valid after the drop/dragLeave event completes. Drop event is the last chance to extract the data. 2. Native heap detects corruption, but SWT swallows the exception using trycatch JNIGen flag. Further access to corrupt memory results in a crash. 3. SWT DropTarget doesn't release the native IDataObject in its IDropTarget.Drop implementation, although it must do so. SWT should invalidate all created TransferData objects after drop/dragLeave to prevent misuse. Also, trycatch flag has to go. There are exactly zero valid uses for it.
Created attachment 277769 [details] Improved repro snippet
I submitted another snippet, with less unrelated code, and it also reproduces more reliably. Nikita, will you fix the bug? Note: The problem is easily seen with Microsoft Application Verifier. I actually suggest that you use it often, as it exposes hard-to-find bugs. The downside is that tested application runs slower and uses more RAM. Steps to use it: 1) Install Win10 SDK -- https://developer.microsoft.com/en-US/windows/downloads/windows-10-sdk 2) You only need Application Verifier component in setup 3) Find Application Verifier in menu, or in C:\Windows\System32\appverif.exe 4) Use 'File | Add Application...' to add any java.exe, specific path doesn't matters it remembers executable name only. 5) IMPORTANT: Unselect anything under 'Basics' except 'Heaps'. 6) Save. Application Verifier applies settings forever until you explicitly clear them, so it doesn't have to run along with tested application.
Curiously, with my snippet VM crashes so hard that its crash handler fails to work (therefore, no hs_err_pid*.log files)
I think this problem is interesting to us. So if Nikita isn't willing to fix it, I will do it, just maybe not right now.
(In reply to Alexandr Miloslavskiy from comment #20) > Nikita, will you fix the bug? Nikita?
(In reply to Lars Vogel from comment #23) > (In reply to Alexandr Miloslavskiy from comment #20) > > Nikita, will you fix the bug? > > Nikita? I'll make a patch, unless someone beats me to it. The core issue is user code doing things it shouldn't, so it's low proprity.
Re-setting target for now.
AFAIK Nikita is not available at the moment. Resetting the assignee.
Created attachment 280884 [details] Crash free snippet (In reply to Nikita Nemkin from comment #18) > Unfortunately, the suggested patch doesn't fix anything. > > The root cause of the crash seems to be use-after-free of the native > IDataObject and the resulting memory corruption. Multiple bugs contribute to > the problem. > > 1. Crashing snippet stashes away the DropTargetEvent event (which references > the native IDataObject) and later extracts data from it. This is API misuse, > at least on Windows, because the native IDataObject is not valid after the > drop/dragLeave event completes. Drop event is the last chance to extract the > data. > > 2. Native heap detects corruption, but SWT swallows the exception using > trycatch JNIGen flag. Further access to corrupt memory results in a crash. > > 3. SWT DropTarget doesn't release the native IDataObject in its > IDropTarget.Drop implementation, although it must do so. > > SWT should invalidate all created TransferData objects after drop/dragLeave > to prevent misuse. Also, trycatch flag has to go. There are exactly zero > valid uses for it. I'm also not sure there is something to fix here. As Nikita said user code is doing things it shouldn't do and accessing fields which are effective private (see javadoc of pIDataObject) We could invalidate the TransferData but the next client will notice it is later 0 and will backup the pointer directly instead of the data object and we get this crash again. So not much improvement from this. For the JNIGen flag. Certainly a good point, had not checked this. Might be something for a separate ticket. The last point (release object in drop) is fixed meanwhile as part of bug 549643. A possible workaround for this problem (even if I would not recommend it) is to properly use the IDataObject. Those objects are reference counted. If you want to access the data after the drop is done add a reference while it is still valid. I attached a modification of your second snippet which does not crash anymore in my test. This could work for most drag sources. I expect it to not work with SWT drag source before bug 549643.
Thanks, Paul. Closing as of Paul comment. Please reopen if I misunderstood.
I'll reopen because I miss read one detail a bit. I wrote invalidating the TransferData does not bring much since someone could still save the pointer directly. This is true but I missed that we pass the object not the pointer to the nativeToJava method. So if we invalidate the object someone would need to save the pointer before and later create a new TransferData object with this pointer to query for the data. It might be more obvious that this is a bad idea this way. And invalidating the pointer should be trivial so I'll provide a patch for this.
New Gerrit change created: https://git.eclipse.org/r/153997
Gerrit change https://git.eclipse.org/r/153997 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=f5b4cf340a10760ac5f0e339d6b5dba39863a310
This one is done for me. For the remaining trycatch flag stuff Nikita mentioned I opened bug 558501.
Created attachment 281364 [details] Snippet showing how dragOver() is now broken This patch has broken our software. Please see attached snippet. I understand that the problem here is that DropTarget.clearEventData() indirectly clears DropTarget.selectedDataType.
Reopening because the recent patch introduced new bugs.
New Gerrit change created: https://git.eclipse.org/r/155124
(In reply to Alexandr Miloslavskiy from comment #33) > This patch has broken our software. Yes it does and I'm surprised my DnD unit test did not caught it. (atm a Windows only test but I run it for this change) Unfortunately my just uploaded change fix the problem but is actually useless for the original problem. Will improve it or revert the original change. Anyway sorry for the trouble.
Unfortunately I don't have time to investigate at the moment, but I find it strange that it's OK for 'DropTarget.selectedDataType' to survive between events, while it's not OK to keep the enclosing object around. This is merely a note and I don't really expect a reply.
It is OK for the object to survive between events. Same for the internal selectedDataType and also for the objects provided at events. It is not ok to use those objects after the DnD operation was finished or more restricted after the drag leaved the drop target. (for canceled after the leave event for drop after the drop event) I'll go for a revert. I see no easy way to prevent misuse without making things unnecessary complicate. Instead I add a Javadoc warning that those event objects are risky to use after the event. A bit off topic: I assume your snippet is the reduced version of a real use case. Had you ever used it on Linux? From my quick look into Linux DnD implementation it seems to be impossible to receive the dragged data in any way but the final Drop event.
Yes, indeed we never managed to obtain data during dragOver() on macOS and Linux. This code was windows-specific. I assume that Linux and macOS implementations could be improved to allow that, but we decided to just disable Windows custom code for now.
After I had to rethink with Alexandr's problem and a closer look on the other platforms implementations I must change my opinion a bit. It is possible to really protect against this crash but not with current SWT and the condition to support Alexandr's use case. To come back to one of the initial questions: (In reply to Jens Kuebler from comment #2) > A very close look at our source code revealed that an old DropTargetEvent > that was no longer valid potentially caused a wrong memory address to be > used. Core of my question would be how to avoid a crash on such an scenario. > Wouldn't it be correct to catch the access violation. The problem starts at the point where we give the user access to those memory address and continues that the user can give Transfer implementations arbitrary pointers to access wrong memory. Linux and Mac do not have such crashes because they do not provide such internal addresses in event objects. My attempt to subsequent clear those event fields had obvious failed so I revert the change for now and opened bug 558794. With such a safe way to query data before drop we could remove (read as 'always 0') the internal pointer from the event and prevent the crash.
Gerrit change https://git.eclipse.org/r/155124 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=989303fb9319a4aa1e17ebe83d9cc3f68784ba47
We have the responsibility to develop a workflow where content is dragged and dropped from a SWT based application to an application developed in CSharp(WPF application based on .NET Framework 4.7.2) on Windows 10. Recently we were upgrading from Eclipse 4.7.3 to Eclipse 4.13 where we ran into a drag and drop issue between the two applications. Intermittently but frequently when dragging an object(string) from the SWT application to the CSharp application, SWT application crashes. I have outlined the issue with example projects(working on 4.12 and crashing on 4.13), problem descriptions, recreation steps with video and other details here https://github.com/iamankushpandit/eclipseDnDIssue We are unable to update to 4.13 SDK and beyond for SWT libraries because of the crash we are experiencing. We think that the issue was possibly introduced with the changes introduced to DragSource in SWT 4.13 for Bug 549643(https://bugs.eclipse.org/bugs/show_bug.cgi?id=549643). This can be seen when the DragSource source code is compared between 4.12 and 4.13 SDK. Please let me know what other information can be helpful for troubleshooting and resolving this issue.
Hey Ankush, could you please open a new bug for your problem? From what I saw in your example it's not the same issue as this one. (even if the title fits, of cause)
Paul, I have created https://bugs.eclipse.org/bugs/show_bug.cgi?id=567422 for the issue.
