From bbf9942641c858289dc20041f7dbb557a5f013f8 Mon Sep 17 00:00:00 2001 From: brandjon Date: Sun, 6 Aug 2017 19:14:56 +0200 Subject: Fix mutability bug in SkylarkList/SkylarkMap Java APIs Previously, you could modify a frozen MutableList indirectly via its iterator(). This CL wraps the underlying list with a Collections.unmodifiableList when obtaining the iterator, and likewise for other methods that return views over Skylark data structures. This also eliminates the SkylarkList#getContents method in favor of just using the SkylarkList object itself. RELNOTES: None PiperOrigin-RevId: 164402129 --- .../devtools/build/lib/syntax/SkylarkDict.java | 21 ++++++++++-- .../devtools/build/lib/syntax/SkylarkList.java | 39 +--------------------- .../devtools/build/lib/syntax/SkylarkMutable.java | 25 +++++++------- 3 files changed, 32 insertions(+), 53 deletions(-) (limited to 'src/main/java/com/google/devtools') diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkDict.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkDict.java index bb9dbbab55..a5fafaeedc 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkDict.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkDict.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.syntax; +import com.google.common.annotations.VisibleForTesting; import com.google.devtools.build.lib.events.Location; import com.google.devtools.build.lib.skylarkinterface.SkylarkModule; import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory; @@ -58,8 +59,16 @@ public final class SkylarkDict extends MutableMap return mutability; } + private SkylarkDict(@Nullable Mutability mutability) { + this.mutability = mutability == null ? Mutability.IMMUTABLE : mutability; + } + private SkylarkDict(@Nullable Environment env) { - mutability = env == null ? Mutability.IMMUTABLE : env.mutability(); + this.mutability = env == null ? Mutability.IMMUTABLE : env.mutability(); + } + + private static SkylarkDict of(@Nullable Mutability mutability) { + return new SkylarkDict<>(mutability); } /** @return a dict mutable in given environment only */ @@ -78,6 +87,14 @@ public final class SkylarkDict extends MutableMap return SkylarkDict.of(env).putUnsafe(k1, v1).putUnsafe(k2, v2); } + // TODO(bazel-team): Make other methods that take in mutabilities instead of environments, make + // this method public. + @VisibleForTesting + static SkylarkDict copyOf( + @Nullable Mutability mutability, Map m) { + return SkylarkDict.of(mutability).putAllUnsafe(m); + } + /** @return a dict mutable in given environment only, with contents copied from given map */ public static SkylarkDict copyOf( @Nullable Environment env, Map m) { @@ -248,7 +265,7 @@ public final class SkylarkDict extends MutableMap return result; } - private static final SkylarkDict EMPTY = of(null); + private static final SkylarkDict EMPTY = of((Mutability) null); // Safe because the empty singleton is immutable. @SuppressWarnings("unchecked") diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java index 13f693ee0d..f9e92ca4cf 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java @@ -50,26 +50,6 @@ public abstract class SkylarkList extends BaseMutableList */ public abstract ImmutableList getImmutableList(); - /** - * Returns a List object with the current underlying contents of this SkylarkList. - * This object must not be mutated, but need not be an {@link ImmutableList}. - * Indeed it can sometimes be a {@link GlobList}. - */ - // TODO(bazel-team): move GlobList out of Skylark, into an extension. - // TODO(bazel-team): Remove this function, prefer getImmutableList()? Looks like a possible source - // of mutability bugs. - public abstract List getContents(); - - // For subList, use the immutable getContents() rather than getContentsUnsafe, - // to prevent subsequent mutation. To get a mutable SkylarkList, - // use a method that takes a Mutability into account. - @Override - public List subList(int fromIndex, int toIndex) { - // TODO(bazel-team): remove this, use the version in BaseMutableList instead, fix that version - // to make a copy if that's what's needed here. - return getContents().subList(fromIndex, toIndex); - } - /** * Retrieve an entry from a SkylarkList. * @@ -241,6 +221,7 @@ public abstract class SkylarkList extends BaseMutableList // TODO(bazel-team): make data structures *and binary operators* extensible // (via e.g. interface classes for each binary operator) so that GlobList // can be implemented outside of the core of Skylark. + // TODO(bazel-team): move GlobList out of Skylark, into an extension. @Nullable private GlobList globList; private final Mutability mutability; @@ -357,19 +338,6 @@ public abstract class SkylarkList extends BaseMutableList return ImmutableList.copyOf(contents); } - /** - * Returns the {@link GlobList} if there is one, otherwise an {@link ImmutableList} copy of the - * regular contents. - */ - @Override - @SuppressWarnings("unchecked") - public List getContents() { - if (globList != null) { - return globList; - } - return getImmutableList(); - } - @Override protected List getContentsUnsafe() { return contents; @@ -597,11 +565,6 @@ public abstract class SkylarkList extends BaseMutableList return contents; } - @Override - public List getContents() { - return contents; - } - @Override protected List getContentsUnsafe() { return contents; diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkMutable.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkMutable.java index 6d28fff616..947d2d6689 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkMutable.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkMutable.java @@ -89,9 +89,10 @@ public abstract class SkylarkMutable implements Freezable, SkylarkValue { * *

A subclass implements a specific data structure interface, say {@link List}, and refines the * return type of {@link #getContentsUnsafe} to be that interface. The subclass implements all of - * the interface's accessors such that they defer to the result of {@code getContentsUnsafe}. The - * subclass implements final versions of all the interface's mutating methods such that they - * are marked {@code @Deprecated} and throw {@link UnsupportedOperationException}. + * the interface's accessors such that they defer to the result of {@code getContentsUnsafe}. + * Accessors such as {@link Collection#iterator()} must return unmodifiable views. The subclass + * implements final versions of all the interface's mutating methods such that they are marked + * {@code @Deprecated} and throw {@link UnsupportedOperationException}. * *

A concrete subclass may provide alternative mutating methods that take in a {@link * Mutability} and validate that the mutation is allowed using {@link #checkMutable}. This @@ -145,12 +146,9 @@ public abstract class SkylarkMutable implements Freezable, SkylarkValue { return getContentsUnsafe().isEmpty(); } - // TODO(bazel-team): Is exposing this a mutability violation? Same for listIterator() and - // subList() below, as well as the entrySet(), keySet(), and values() map methods. Could address - // by returning views over a Collections.unmodifiableCollection(), etc. @Override public Iterator iterator() { - return getContentsUnsafe().iterator(); + return Collections.unmodifiableCollection(getContentsUnsafe()).iterator(); } @Override @@ -158,6 +156,7 @@ public abstract class SkylarkMutable implements Freezable, SkylarkValue { return getContentsUnsafe().size(); } + // toArray() and toArray(T[]) return copies, so we don't need an unmodifiable view. @Override public Object[] toArray() { return getContentsUnsafe().toArray(); @@ -232,17 +231,17 @@ public abstract class SkylarkMutable implements Freezable, SkylarkValue { @Override public ListIterator listIterator() { - return getContentsUnsafe().listIterator(); + return Collections.unmodifiableList(getContentsUnsafe()).listIterator(); } @Override public ListIterator listIterator(int index) { - return getContentsUnsafe().listIterator(index); + return Collections.unmodifiableList(getContentsUnsafe()).listIterator(index); } @Override public List subList(int fromIndex, int toIndex) { - return getContentsUnsafe().subList(fromIndex, toIndex); + return Collections.unmodifiableList(getContentsUnsafe()).subList(fromIndex, toIndex); } // (Disallowed) writing methods of List, in alphabetic order. @@ -292,7 +291,7 @@ public abstract class SkylarkMutable implements Freezable, SkylarkValue { @Override public Set> entrySet() { - return getContentsUnsafe().entrySet(); + return Collections.unmodifiableMap(getContentsUnsafe()).entrySet(); } @Override @@ -307,7 +306,7 @@ public abstract class SkylarkMutable implements Freezable, SkylarkValue { @Override public Set keySet() { - return getContentsUnsafe().keySet(); + return Collections.unmodifiableMap(getContentsUnsafe()).keySet(); } @Override @@ -317,7 +316,7 @@ public abstract class SkylarkMutable implements Freezable, SkylarkValue { @Override public Collection values() { - return getContentsUnsafe().values(); + return Collections.unmodifiableMap(getContentsUnsafe()).values(); } // (Disallowed) writing methods of Map, in alphabetic order. -- cgit v1.2.3