Skip to content

Commit ae73f7c

Browse files
woesswoess
committed
[GR-59981] Fix potential crash with DynamicObjectLibrary.removeKey.
PullRequest: graal/22608
2 parents 9f2f6fb + ddb6907 commit ae73f7c

File tree

3 files changed

+82
-31
lines changed

3 files changed

+82
-31
lines changed

‎truffle/src/com.oracle.truffle.api.object/src/com/oracle/truffle/api/object/DynamicObjectLibraryImpl.java‎

Lines changed: 40 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -390,14 +390,13 @@ static RemovePlan prepareRemove(Shape shapeBefore, Shape shapeAfter, Property re
390390
moves.add(move);
391391
}
392392
}
393+
Location removedPropertyLoc = removedProperty.getLocation();
394+
if (!removedPropertyLoc.isValue()) {
395+
// Use a no-op move to clear the location of the removed property. Must be first.
396+
moves.add(new Move(removedPropertyLoc, null, removedPropertyLoc.getOrdinal(), Integer.MIN_VALUE));
397+
}
393398
if (canMoveInPlace) {
394-
if (moves.isEmpty()) {
395-
Location removedPropertyLoc = removedProperty.getLocation();
396-
if (!removedPropertyLoc.isPrimitive()) {
397-
// Use a no-op move to clear the location of the removed property.
398-
moves.add(new Move(removedPropertyLoc, removedPropertyLoc, 0, 0));
399-
}
400-
} else if (!isSorted(moves)) {
399+
if (!isSorted(moves)) {
401400
moves.sort(Move::compareTo);
402401
}
403402
}
@@ -415,6 +414,10 @@ private static boolean isSorted(List<Move> moves) {
415414
return true;
416415
}
417416

417+
/**
418+
* Moves the value from one location to another, and clears the from location. If both locations
419+
* are the same, only clears the location.
420+
*/
418421
private static final class Move implements Comparable<Move> {
419422
private final Location fromLoc;
420423
private final Location toLoc;
@@ -424,30 +427,51 @@ private static final class Move implements Comparable<Move> {
424427
private static final Move[] EMPTY_ARRAY = new Move[0];
425428

426429
Move(Location fromLoc, Location toLoc, int fromOrd, int toOrd) {
427-
assert (fromLoc == toLoc) == (fromOrd == toOrd);
428-
this.fromLoc = fromLoc;
430+
assert fromLoc != toLoc;
431+
this.fromLoc = Objects.requireNonNull(fromLoc);
429432
this.toLoc = toLoc;
430433
this.fromOrd = fromOrd;
431434
this.toOrd = toOrd;
432435
}
433436

434437
void perform(DynamicObject obj) {
435-
if (fromLoc == toLoc) {
436-
return;
438+
if (toLoc != null) {
439+
performSet(obj, performGet(obj));
437440
}
438-
performSet(obj, performGet(obj));
441+
/*
442+
* It does not matter for correctness whether we clear after the get or set. Clearing
443+
* after the set might be slightly preferable w.r.t. store ordering.
444+
*/
445+
clear(obj);
439446
}
440447

441448
Object performGet(DynamicObject obj) {
442449
return fromLoc.get(obj, false);
443450
}
444451

445452
void performSet(DynamicObject obj, Object value) {
453+
if (toLoc == null) {
454+
return; // clear only
455+
}
446456
toLoc.setSafe(obj, value, false, true);
447457
}
448458

449-
void clear(DynamicObject obj) {
450-
// clear location to avoid memory leak
459+
Object performGetAndClear(DynamicObject obj) {
460+
Object value = performGet(obj);
461+
clear(obj);
462+
return value;
463+
}
464+
465+
/*
466+
* Clears the location to avoid potential memory leaks AND kills the from location identity.
467+
*
468+
* Note: It is important that we always set the "from" location in compiled code to kill the
469+
* location identity at this point, so as to prevent reads from that location identity (of
470+
* the old shape) to move below any write to the same memory location but with a different
471+
* location identity (of the new shape), since that would be seen as independent and
472+
* therefore not kill the "from" location that is being overwritten or cleared. (GR-59981)
473+
*/
474+
private void clear(DynamicObject obj) {
451475
fromLoc.clear(obj);
452476
}
453477

@@ -498,25 +522,19 @@ void perform(DynamicObject object) {
498522
moves[i].perform(object);
499523
}
500524

501-
if (moves.length > 0) {
502-
moves[0].clear(object);
503-
}
504-
505525
DynamicObjectSupport.trimToSize(object, shapeBefore, shapeAfter);
506-
object.setShape(shapeAfter);
507526
} else {
508527
// we cannot perform the moves in place, so stash away the values
509528
Object[] tempValues = new Object[moves.length];
510529
for (int i = moves.length - 1; i >= 0; i--) {
511-
tempValues[i] = moves[i].performGet(object);
512-
moves[i].clear(object);
530+
tempValues[i] = moves[i].performGetAndClear(object);
513531
}
514532
DynamicObjectSupport.resize(object, shapeBefore, shapeAfter);
515533
for (int i = moves.length - 1; i >= 0; i--) {
516534
moves[i].performSet(object, tempValues[i]);
517535
}
518-
object.setShape(shapeAfter);
519536
}
537+
object.setShape(shapeAfter);
520538
}
521539

522540
@TruffleBoundary

‎truffle/src/com.oracle.truffle.api.object/src/com/oracle/truffle/api/object/ExtLocations.java‎

Lines changed: 39 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,8 @@
5151
import com.oracle.truffle.api.CompilerDirectives.CompilationFinal;
5252
import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
5353
import com.oracle.truffle.api.Truffle;
54-
5554
import com.oracle.truffle.api.impl.AbstractAssumption;
55+
5656
import sun.misc.Unsafe;
5757

5858
/**
@@ -197,6 +197,10 @@ public final void set(DynamicObject store, Object value, boolean guard, boolean
197197
}
198198
}
199199

200+
@Override
201+
void clear(DynamicObject store) {
202+
}
203+
200204
@Override
201205
public String toString() {
202206
return "=" + value;
@@ -637,8 +641,8 @@ private void setObjectInternal(DynamicObject store, Object value, boolean guard)
637641
}
638642

639643
@Override
640-
protected void clear(DynamicObject store) {
641-
UnsafeAccess.unsafePutObject(getArray(store, false), getOffset(), null, this);
644+
void clear(DynamicObject store) {
645+
setObjectInternal(store, null, false);
642646
}
643647

644648
@Override
@@ -696,8 +700,8 @@ private void setObjectInternal(DynamicObject store, Object value) {
696700
}
697701

698702
@Override
699-
protected void clear(DynamicObject store) {
700-
UnsafeAccess.unsafePutObject(store, getOffset(), null, this);
703+
void clear(DynamicObject store) {
704+
setObjectInternal(store, null);
701705
}
702706

703707
@Override
@@ -825,6 +829,11 @@ private void setIntInternal(DynamicObject store, int value) {
825829
UnsafeAccess.unsafePutLong(store, getOffset(), value & 0xffff_ffffL, this);
826830
}
827831

832+
@Override
833+
protected void clear(DynamicObject store) {
834+
setIntInternal(store, 0);
835+
}
836+
828837
@Override
829838
public boolean canStore(Object value) {
830839
return value instanceof Integer;
@@ -885,6 +894,11 @@ private void setDoubleInternal(DynamicObject store, double value) {
885894
UnsafeAccess.unsafePutLong(store, getOffset(), Double.doubleToRawLongBits(value), this);
886895
}
887896

897+
@Override
898+
void clear(DynamicObject store) {
899+
setDoubleInternal(store, 0);
900+
}
901+
888902
@Override
889903
protected void set(DynamicObject store, Object value, boolean guard, boolean init) {
890904
if (canStore(value)) {
@@ -1009,6 +1023,11 @@ public void setInt(DynamicObject store, int value, boolean guard, boolean init)
10091023
setIntInternal(store, value, guard);
10101024
}
10111025

1026+
@Override
1027+
void clear(DynamicObject store) {
1028+
setIntInternal(store, 0, false);
1029+
}
1030+
10121031
@Override
10131032
public boolean canStore(Object value) {
10141033
return value instanceof Integer;
@@ -1076,6 +1095,11 @@ private void setDoubleInternal(DynamicObject store, double value, boolean guard)
10761095
UnsafeAccess.unsafePutDouble(getArray(store, guard), getOffset(), value, this);
10771096
}
10781097

1098+
@Override
1099+
void clear(DynamicObject store) {
1100+
setDoubleInternal(store, 0, false);
1101+
}
1102+
10791103
@Override
10801104
public void setDouble(DynamicObject store, double value, boolean guard, boolean init) {
10811105
if (!init) {
@@ -1164,6 +1188,11 @@ private void setLongInternal(DynamicObject store, long value) {
11641188
UnsafeAccess.unsafePutLong(store, getOffset(), value, this);
11651189
}
11661190

1191+
@Override
1192+
void clear(DynamicObject store) {
1193+
setLongInternal(store, 0L);
1194+
}
1195+
11671196
@Override
11681197
protected void set(DynamicObject store, Object value, boolean guard, boolean init) {
11691198
if (canStore(value)) {
@@ -1260,6 +1289,11 @@ private void setLongInternal(DynamicObject store, long value, boolean guard) {
12601289
UnsafeAccess.unsafePutLong(getArray(store, guard), getOffset(), value, this);
12611290
}
12621291

1292+
@Override
1293+
void clear(DynamicObject store) {
1294+
setLongInternal(store, 0L, false);
1295+
}
1296+
12631297
@Override
12641298
public void setLong(DynamicObject store, long value, boolean guard, boolean init) {
12651299
if (!init) {

‎truffle/src/com.oracle.truffle.api.object/src/com/oracle/truffle/api/object/Location.java‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,12 +40,12 @@
4040
*/
4141
package com.oracle.truffle.api.object;
4242

43+
import java.util.Objects;
44+
4345
import com.oracle.truffle.api.Assumption;
4446
import com.oracle.truffle.api.CompilerDirectives;
4547
import com.oracle.truffle.api.nodes.UnexpectedResultException;
4648

47-
import java.util.Objects;
48-
4949
/**
5050
* Property location.
5151
*
@@ -416,8 +416,7 @@ Class<?> getType() {
416416
return null;
417417
}
418418

419-
void clear(@SuppressWarnings("unused") DynamicObject store) {
420-
}
419+
abstract void clear(DynamicObject store);
421420

422421
abstract int getOrdinal();
423422

0 commit comments

Comments
 (0)