Skip to content

Commit 04987c0

Browse files
zkxvhzkxvh
committed
Run SWT.SetData callbacks inside Display.asyncExec() #678
Move execution of SWT.SetData callbacks from cellDataProc(...) to Display.asyncExec(...) to avoid app crashes and/or other native code problems. Why: 1. cellDataProc() is a so called "cell data function" (a GTK3 term), GTK3 expects no tree structure modifications inside such functions. Failing to do so may lead to app crash and/or other native code problems. 2. SWT.SetData callbacks are written by users, and can contain any code, including code for tree modifications. Fixes #678
1 parent a0a0485 commit 04987c0

File tree

4 files changed

+253
-20
lines changed

4 files changed

+253
-20
lines changed

‎bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Table.java

+75-8
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515
package org.eclipse.swt.widgets;
1616

1717

18+
import java.util.*;
19+
1820
import org.eclipse.swt.*;
1921
import org.eclipse.swt.events.*;
2022
import org.eclipse.swt.graphics.*;
@@ -98,6 +100,7 @@ public class Table extends Composite {
98100
int headerHeight;
99101
boolean boundsChangedSinceLastDraw, headerVisible, wasScrolled;
100102
boolean rowActivated;
103+
AsyncSetDataTask asyncSetDataTask = new AsyncSetDataTask();
101104

102105
private long headerCSSProvider;
103106

@@ -220,15 +223,15 @@ long cellDataProc (long tree_column, long cell, long tree_model, long iter, long
220223
}
221224
}
222225
if (modelIndex == -1) return 0;
223-
boolean setData = false;
226+
boolean updated = false;
227+
long [] ptr = new long [1];
224228
if ((style & SWT.VIRTUAL) != 0) {
225-
if (!item.cached) {
226-
lastIndexOf = index[0];
227-
setData = checkData (item);
229+
if (item.cached) {
230+
updated = item.updated;
231+
item.updated = false;
232+
} else {
233+
asyncSetDataTask.enqueueItem (item);
228234
}
229-
}
230-
long [] ptr = new long [1];
231-
if (setData) {
232235
ptr [0] = 0;
233236
if (isPixbuf) {
234237
GTK.gtk_tree_model_get (tree_model, iter, modelIndex + CELL_PIXBUF, ptr, -1);
@@ -266,7 +269,7 @@ long cellDataProc (long tree_column, long cell, long tree_model, long iter, long
266269
}
267270
}
268271
}
269-
if (setData) {
272+
if (updated) {
270273
ignoreCell = cell;
271274
setScrollWidth (tree_column, item);
272275
ignoreCell = 0;
@@ -4249,4 +4252,68 @@ public void dispose() {
42494252
headerCSSProvider = 0;
42504253
}
42514254
}
4255+
4256+
/**
4257+
* A task to set data for {@link TableItem items} via {@link SWT#SetData} user
4258+
* callbacks asynchronously.
4259+
* <p>
4260+
* {@link SWT#SetData} callbacks should not be run inside
4261+
* {@link Table#cellDataProc(long, long, long, long, long)} because it's a cell
4262+
* data function (in GTK terms) and no tree structure modifications are allowed
4263+
* in such functions.<br>
4264+
* Violation of the above may cause application crashes and other native-code
4265+
* problems in gtk.<br>
4266+
* Since {@link SWT#SetData} callbacks are written by users, we run them inside
4267+
* {@link Display#asyncExec(Runnable)} to avoid the native-code problems.
4268+
*/
4269+
class AsyncSetDataTask implements Runnable {
4270+
boolean scheduled;
4271+
LinkedHashSet<TableItem> itemsQueue = new LinkedHashSet<> ();
4272+
4273+
void enqueueItem (TableItem item) {
4274+
itemsQueue.add (item);
4275+
ensureExecutionScheduled ();
4276+
}
4277+
4278+
void ensureExecutionScheduled () {
4279+
if (!scheduled && !isDisposed ()) {
4280+
display.asyncExec (this);
4281+
scheduled = true;
4282+
}
4283+
}
4284+
4285+
@Override
4286+
public void run () {
4287+
scheduled = false;
4288+
if (itemsQueue.isEmpty () || isDisposed ()) {
4289+
return;
4290+
}
4291+
boolean updated = false;
4292+
LinkedHashSet<TableItem> items = itemsQueue;
4293+
itemsQueue = new LinkedHashSet<> ();
4294+
try {
4295+
for (Iterator<TableItem> it = items.iterator (); it.hasNext ();) {
4296+
TableItem item = it.next ();
4297+
it.remove ();
4298+
if (!item.cached && !item.isDisposed ()) {
4299+
if (checkData (item)) {
4300+
updated = item.updated = true;
4301+
}
4302+
}
4303+
}
4304+
} catch (Throwable t) {
4305+
if (!items.isEmpty ()) {
4306+
itemsQueue.addAll (items);
4307+
if (updated) {
4308+
display.asyncExec ( () -> redraw ());
4309+
}
4310+
ensureExecutionScheduled ();
4311+
}
4312+
throw t;
4313+
}
4314+
if (updated) {
4315+
redraw ();
4316+
}
4317+
}
4318+
}
42524319
}

‎bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/TableItem.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ public class TableItem extends Item {
4444
Font font;
4545
Font[] cellFont;
4646
String [] strings;
47-
boolean cached, grayed, settingData;
47+
boolean cached, grayed, updated, settingData;
4848

4949
/**
5050
* Constructs a new instance of this class given its parent

‎bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Tree.java

+71-11
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,7 @@ public class Tree extends Composite {
110110
Color headerBackground, headerForeground;
111111
boolean boundsChangedSinceLastDraw, wasScrolled;
112112
boolean rowActivated;
113+
AsyncSetDataTask asyncSetDataTask = new AsyncSetDataTask();
113114

114115
private long headerCSSProvider;
115116

@@ -296,20 +297,15 @@ long cellDataProc (long tree_column, long cell, long tree_model, long iter, long
296297
}
297298
}
298299
if (modelIndex == -1) return 0;
299-
boolean setData = false;
300300
boolean updated = false;
301+
long [] ptr = new long [1];
301302
if ((style & SWT.VIRTUAL) != 0) {
302-
if (!item.cached) {
303-
//lastIndexOf = index [0];
304-
setData = checkData (item);
305-
}
306-
if (item.updated) {
307-
updated = true;
303+
if (item.cached) {
304+
updated = item.updated;
308305
item.updated = false;
306+
} else {
307+
asyncSetDataTask.enqueueItem (item);
309308
}
310-
}
311-
long [] ptr = new long [1];
312-
if (setData) {
313309
if (isPixbuf) {
314310
ptr [0] = 0;
315311
GTK.gtk_tree_model_get (tree_model, iter, modelIndex + CELL_PIXBUF, ptr, -1);
@@ -348,7 +344,7 @@ long cellDataProc (long tree_column, long cell, long tree_model, long iter, long
348344
}
349345
}
350346
}
351-
if (setData || updated) {
347+
if (updated) {
352348
ignoreCell = cell;
353349
setScrollWidth (tree_column, item);
354350
ignoreCell = 0;
@@ -4333,4 +4329,68 @@ public void dispose() {
43334329
headerCSSProvider = 0;
43344330
}
43354331
}
4332+
4333+
/**
4334+
* A task to set data for {@link TreeItem items} via {@link SWT#SetData} user
4335+
* callbacks asynchronously.
4336+
* <p>
4337+
* {@link SWT#SetData} callbacks should not be run inside
4338+
* {@link Tree#cellDataProc(long, long, long, long, long)} because it's a cell
4339+
* data function (in GTK terms) and no tree structure modifications are allowed
4340+
* in such functions.<br>
4341+
* Violation of the above may cause application crashes and other native-code
4342+
* problems in gtk.<br>
4343+
* Since {@link SWT#SetData} callbacks are written by users, we run them inside
4344+
* {@link Display#asyncExec(Runnable)} to avoid the native-code problems.
4345+
*/
4346+
class AsyncSetDataTask implements Runnable {
4347+
boolean scheduled;
4348+
LinkedHashSet<TreeItem> itemsQueue = new LinkedHashSet<> ();
4349+
4350+
void enqueueItem (TreeItem item) {
4351+
itemsQueue.add (item);
4352+
ensureExecutionScheduled ();
4353+
}
4354+
4355+
void ensureExecutionScheduled () {
4356+
if (!scheduled && !isDisposed ()) {
4357+
display.asyncExec (this);
4358+
scheduled = true;
4359+
}
4360+
}
4361+
4362+
@Override
4363+
public void run () {
4364+
scheduled = false;
4365+
if (itemsQueue.isEmpty () || isDisposed ()) {
4366+
return;
4367+
}
4368+
boolean updated = false;
4369+
LinkedHashSet<TreeItem> items = itemsQueue;
4370+
itemsQueue = new LinkedHashSet<> ();
4371+
try {
4372+
for (Iterator<TreeItem> it = items.iterator (); it.hasNext ();) {
4373+
TreeItem item = it.next ();
4374+
it.remove ();
4375+
if (!item.cached && !item.isDisposed ()) {
4376+
if (checkData (item)) {
4377+
updated = item.updated = true;
4378+
}
4379+
}
4380+
}
4381+
} catch (Throwable t) {
4382+
if (!items.isEmpty ()) {
4383+
itemsQueue.addAll (items);
4384+
if (updated) {
4385+
display.asyncExec ( () -> redraw ());
4386+
}
4387+
ensureExecutionScheduled ();
4388+
}
4389+
throw t;
4390+
}
4391+
if (updated) {
4392+
redraw ();
4393+
}
4394+
}
4395+
}
43364396
}

‎tests/org.eclipse.swt.tests.gtk/ManualTests/org/eclipse/swt/tests/gtk/snippets/Issue678_JvmCrashOnTreeItemSetImage.java

+106
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
package org.eclipse.swt.tests.gtk.snippets;
2+
3+
import org.eclipse.swt.SWT;
4+
import org.eclipse.swt.graphics.Image;
5+
import org.eclipse.swt.layout.FillLayout;
6+
import org.eclipse.swt.widgets.Display;
7+
import org.eclipse.swt.widgets.Shell;
8+
import org.eclipse.swt.widgets.Tree;
9+
import org.eclipse.swt.widgets.TreeItem;
10+
11+
/**
12+
* Title: app crash
13+
* <p>
14+
*
15+
* How to run: run this class as java application.
16+
* <p>
17+
*
18+
* Bug description: when {@link TreeItem#setImage(Image)} is called within an
19+
* {@link SWT#SetData} event handler for a {@link SWT#VIRTUAL} tree, then a JVM
20+
* crash can happen because of use-after-free gtk3 renderer in
21+
* {@code Tree.cellDataProc()}.
22+
*
23+
* <pre>
24+
* Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
25+
* C [libgobject-2.0.so.0+0x3b251] g_type_check_instance_is_fundamentally_a+0x11
26+
* C [libswt-pi3-gtk-4958r2.so+0x4b609] Java_org_eclipse_swt_internal_gtk_OS_g_1object_1set__J_3BJJ+0x4a
27+
*
28+
* Java frames: (J=compiled Java code, j=interpreted, Vv=VM code)
29+
* J 11988 org.eclipse.swt.internal.gtk.OS.g_object_set(J[BJJ)V (0 bytes)
30+
* J 10921 c1 org.eclipse.swt.widgets.Tree.cellDataProc(JJJJJ)J (486 bytes)
31+
* J 10920 c1 org.eclipse.swt.widgets.Display.cellDataProc(JJJJJ)J (29 bytes)
32+
* v ~StubRoutines::call_stub
33+
* J 11619 org.eclipse.swt.internal.gtk3.GTK3.gtk_main_iteration_do(Z)Z (0 bytes)
34+
* J 11623 c1 org.eclipse.swt.widgets.Display.readAndDispatch()Z (88 bytes)
35+
* </pre>
36+
*
37+
* Please note that the crash isn't guaranteed to happen.<br>
38+
* Why: the causes of the bug is use-after-free in native code. <br>
39+
* The app crash happens when in-between "free" and "use" the released memory is
40+
* allocated and modified by some other native code.<br>
41+
* For example some if some pointer accessed during "use" is changed to contain
42+
* address to inaccessible memory, then the app will crash with SIGSEGV.<br>
43+
* The problem is that the native code in-between "free" and "use" (i.e. jvm and
44+
* 3d-party libraries) is too complex to predict.
45+
*
46+
* Expected results: image is shown in the tree, no crashes, no errors.
47+
* <p>
48+
*
49+
* GTK Version(s): 3.24.37
50+
*/
51+
public class Issue678_JvmCrashOnTreeItemSetImage {
52+
53+
private static final int NUM_ITERATIONS = 100;
54+
55+
public static void main (String[] args) {
56+
Display display = new Display ();
57+
Shell shell = new Shell (display);
58+
shell.setSize (400, 300);
59+
shell.setLayout (new FillLayout ());
60+
shell.open ();
61+
Image image = new Image (display, 20, 20);
62+
63+
for (int i = 0; i < NUM_ITERATIONS; i++) {
64+
Tree tree = new Tree (shell, SWT.VIRTUAL);
65+
tree.addListener (SWT.SetData, e -> {
66+
TreeItem item = (TreeItem) e.item;
67+
item.setText (0, "A");
68+
69+
// for some reason sleeping increases probability of crash
70+
try {
71+
Thread.sleep (50);
72+
} catch (InterruptedException ex) {
73+
throw new RuntimeException (ex);
74+
}
75+
76+
item.setImage (image); // <-- this is the critical line!
77+
});
78+
tree.setItemCount (1);
79+
shell.layout ();
80+
81+
waitUntilIdle ();
82+
83+
tree.dispose ();
84+
}
85+
86+
display.dispose ();
87+
}
88+
89+
private static void waitUntilIdle () {
90+
long lastActive = System.currentTimeMillis ();
91+
while (true) {
92+
if (Thread.interrupted ()) {
93+
throw new AssertionError ();
94+
}
95+
if (Display.getCurrent ().readAndDispatch ()) {
96+
lastActive = System.currentTimeMillis ();
97+
} else {
98+
if (lastActive + 10 < System.currentTimeMillis ()) {
99+
return;
100+
}
101+
Thread.yield ();
102+
}
103+
}
104+
}
105+
106+
}

0 commit comments

Comments
 (0)