Skip to content

Commit

Permalink
starlark: fix ArrayStoreException crash in list.append
Browse files Browse the repository at this point in the history
...by establishing the invariant that StarlarkList.elems.getClass() == Object[].class.

+ Test

PiperOrigin-RevId: 343114456
  • Loading branch information
adonovan authored and copybara-github committed Nov 18, 2020
1 parent ec54013 commit 9b5df56
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 9 deletions.
2 changes: 1 addition & 1 deletion src/main/java/net/starlark/java/eval/Starlark.java
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ public static Iterable<?> toIterable(Object x) throws EvalException {
public static Object[] toArray(Object x) throws EvalException {
// Specialize Sequence and Dict to avoid allocation and/or indirection.
if (x instanceof Sequence) {
return ((Sequence<?>) x).toArray();
return ((Sequence<?>) x).toArray(new Object[((Sequence) x).size()]);
} else if (x instanceof Dict) {
return ((Dict<?, ?>) x).keySet().toArray();
} else {
Expand Down
12 changes: 7 additions & 5 deletions src/main/java/net/starlark/java/eval/StarlarkList.java
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ public final class StarlarkList<E> extends AbstractList<E>
// but without the extra indirection of using ArrayList.

// elems[0:size] holds the logical elements, and elems[size:] are not used.
// elems.getClass() == Object[].class. This is necessary to avoid ArrayStoreException.
private int size;
private int iteratorCount; // number of active iterators (unused once frozen)
private Object[] elems = EMPTY_ARRAY; // elems[i] == null iff i >= size
Expand All @@ -88,15 +89,16 @@ public final class StarlarkList<E> extends AbstractList<E>
private static final Object[] EMPTY_ARRAY = {};

private StarlarkList(@Nullable Mutability mutability, Object[] elems, int size) {
Preconditions.checkArgument(elems.getClass() == Object[].class);
this.elems = elems;
this.size = size;
this.mutability = mutability == null ? Mutability.IMMUTABLE : mutability;
}

/**
* Takes ownership of the supplied array and returns a new StarlarkList instance that initially
* wraps the array. The caller must not subsequently modify the array, but the StarlarkList
* instance may do so.
* Takes ownership of the supplied array of class Object[].class, and returns a new StarlarkList
* instance that initially wraps the array. The caller must not subsequently modify the array, but
* the StarlarkList instance may do so.
*/
static <T> StarlarkList<T> wrap(@Nullable Mutability mutability, Object[] elems) {
return new StarlarkList<>(mutability, elems, elems.length);
Expand Down Expand Up @@ -184,13 +186,13 @@ public static <T> StarlarkList<T> immutableCopyOf(Iterable<? extends T> elems) {
*/
public static <T> StarlarkList<T> of(@Nullable Mutability mutability, T... elems) {
checkElemsValid(elems);
return wrap(mutability, Arrays.copyOf(elems, elems.length));
return wrap(mutability, Arrays.copyOf(elems, elems.length, Object[].class));
}

/** Returns an immutable {@code StarlarkList} with the given items. */
public static <T> StarlarkList<T> immutableOf(T... elems) {
checkElemsValid(elems);
return wrap(null, Arrays.copyOf(elems, elems.length));
return wrap(null, Arrays.copyOf(elems, elems.length, Object[].class));
}

@Override
Expand Down
14 changes: 11 additions & 3 deletions src/test/java/net/starlark/java/eval/StarlarkListTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -321,12 +321,20 @@ public void testCopyOfTakesCopy() throws EvalException {

@Test
public void testWrapTakesOwnershipOfArray() throws EvalException {
String[] wrapped = {"hello"};
Object[] wrapped = {"hello"};
Mutability mutability = Mutability.create("test");
StarlarkList<String> mutableList = StarlarkList.wrap(mutability, wrapped);
StarlarkList<Object> mutableList = StarlarkList.wrap(mutability, wrapped);

// Big no-no, but we're proving a point.
wrapped[0] = "goodbye";
assertThat((List<String>) mutableList).containsExactly("goodbye");
assertThat((List<?>) mutableList).containsExactly("goodbye");
}

@Test
public void testOfReturnsListWhoseArrayElementTypeIsObject() throws EvalException {
Mutability mu = Mutability.create("test");
StarlarkList<Object> list = StarlarkList.of(mu, "a", "b");
list.addElement(StarlarkInt.of(1)); // no ArrayStoreException
assertThat(list.toString()).isEqualTo("[\"a\", \"b\", 1]");
}
}

0 comments on commit 9b5df56

Please sign in to comment.