Don't call toString() on the results of successful futures.

RELNOTES=AbstractFuture.toString() no longer includes the toString() of the result. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=336911024
google · Oct 13, 2020 · 2ebf27f · 2ebf27f
1 parent 1b20a37
commit 2ebf27f
Show file tree

Hide file tree

Showing 6 changed files with 136 additions and 20 deletions.
diff --git a/android/guava-tests/test/com/google/common/util/concurrent/AbstractFutureTest.java b/android/guava-tests/test/com/google/common/util/concurrent/AbstractFutureTest.java
@@ -25,6 +25,7 @@
 import com.google.common.collect.Sets;
 import com.google.common.util.concurrent.internal.InternalFutureFailureAccess;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.Set;
@@ -213,6 +214,44 @@ public void testToString_allUnique() throws Exception {
     assertThat(SettableFuture.create().toString()).isNotEqualTo(SettableFuture.create().toString());
   }
 
+  public void testToString_oom() throws Exception {
+    SettableFuture<Object> future = SettableFuture.create();
+    future.set(
+        new Object() {
+          @Override
+          public String toString() {
+            throw new OutOfMemoryError();
+          }
+
+          @Override
+          public int hashCode() {
+            throw new OutOfMemoryError();
+          }
+        });
+
+    String unused = future.toString();
+
+    SettableFuture<Object> future2 = SettableFuture.create();
+
+    // A more organic OOM from a toString implementation
+    Object object =
+        new Object() {
+          @Override
+          public String toString() {
+            return new String(new char[50_000]);
+          }
+        };
+    List<Object> list = Collections.singletonList(object);
+    for (int i = 0; i < 10; i++) {
+      Object[] array = new Object[500];
+      Arrays.fill(array, list);
+      list = Arrays.asList(array);
+    }
+    future2.set(list);
+
+    unused = future.toString();
+  }
+
   public void testToString_notDone() throws Exception {
     AbstractFuture<Object> testFuture =
         new AbstractFuture<Object>() {
@@ -243,7 +282,8 @@ public String pendingToString() {
             return "cause=[Because this test isn't done]";
           }
         };
-    assertThat(testFuture.toString()).matches("[^\\[]+\\[status=SUCCESS, result=\\[true\\]\\]");
+    assertThat(testFuture.toString())
+        .matches("[^\\[]+\\[status=SUCCESS, result=\\[java.lang.Boolean@\\w+\\]\\]");
   }
 
   /**
@@ -308,7 +348,7 @@ public String pendingToString() {
                 + " info=\\[cause=\\[Someday...]]]]]");
     testFuture2.set("result string");
     assertThat(testFuture3.toString())
-        .matches("[^\\[]+\\[status=SUCCESS, result=\\[result string\\]\\]");
+        .matches("[^\\[]+\\[status=SUCCESS, result=\\[java.lang.String@\\w+\\]\\]");
   }
 
   public void testToString_cancelled() throws Exception {
@@ -944,11 +984,11 @@ public String toString() {
   public void testSetIndirectSelf_toString() {
     final SettableFuture<Object> orig = SettableFuture.create();
     // unlike the above this indirection defeats the trivial cycle detection and causes a SOE
-    orig.set(
-        new Object() {
+    orig.setFuture(
+        new ForwardingListenableFuture<Object>() {
           @Override
-          public String toString() {
-            return orig.toString();
+          protected ListenableFuture<Object> delegate() {
+            return orig;
           }
         });
     assertThat(orig.toString())

diff --git a/android/guava-tests/test/com/google/common/util/concurrent/FuturesTest.java b/android/guava-tests/test/com/google/common/util/concurrent/FuturesTest.java
@@ -2627,8 +2627,8 @@ public ListenableFuture<String> call() throws Exception {
     assertThat(futureResult.toString())
         .matches(
             "CombinedFuture@\\w+\\[status=PENDING,"
-                + " info=\\[futures=\\[SettableFuture@\\w+\\[status=SUCCESS, result=\\[1]],"
-                + " SettableFuture@\\w+\\[status=PENDING]]]]");
+                + " info=\\[futures=\\[SettableFuture@\\w+\\[status=SUCCESS,"
+                + " result=\\[java.lang.Integer@\\w+]], SettableFuture@\\w+\\[status=PENDING]]]]");
 
     // Backing futures complete
     Boolean booleanPartial = true;
@@ -2649,7 +2649,7 @@ public ListenableFuture<String> call() throws Exception {
     String expectedResult = createCombinedResult(integerPartial, booleanPartial);
     assertEquals(expectedResult, futureResult.get());
     assertThat(futureResult.toString())
-        .matches("CombinedFuture@\\w+\\[status=SUCCESS, result=\\[" + expectedResult + "]]");
+        .matches("CombinedFuture@\\w+\\[status=SUCCESS, result=\\[java.lang.String@\\w+]]");
   }
 
   public void testWhenAllComplete_asyncError() throws Exception {

diff --git a/android/guava/src/com/google/common/util/concurrent/AbstractFuture.java b/android/guava/src/com/google/common/util/concurrent/AbstractFuture.java
@@ -1158,7 +1158,7 @@ private void addDoneString(StringBuilder builder) {
     try {
       V value = getUninterruptibly(this);
       builder.append("SUCCESS, result=[");
-      appendUserObject(builder, value);
+      appendResultObject(builder, value);
       builder.append("]");
     } catch (ExecutionException e) {
       builder.append("FAILURE, cause=[").append(e.getCause()).append("]");
@@ -1169,6 +1169,24 @@ private void addDoneString(StringBuilder builder) {
     }
   }
 
+  /**
+   * Any object can be the result of a Future, and not every object has a reasonable toString()
+   * implementation. Using a reconstruction of the default Object.toString() prevents OOMs and stack
+   * overflows, and helps avoid sensitive data inadvertently ending up in exception messages.
+   */
+  private void appendResultObject(StringBuilder builder, Object o) {
+    if (o == null) {
+      builder.append("null");
+    } else if (o == this) {
+      builder.append("this future");
+    } else {
+      builder
+          .append(o.getClass().getName())
+          .append("@")
+          .append(Integer.toHexString(System.identityHashCode(o)));
+    }
+  }
+
   /** Helper for printing user supplied objects into our toString method. */
   private void appendUserObject(StringBuilder builder, Object o) {
     // This is some basic recursion detection for when people create cycles via set/setFuture or

diff --git a/guava-tests/test/com/google/common/util/concurrent/AbstractFutureTest.java b/guava-tests/test/com/google/common/util/concurrent/AbstractFutureTest.java
@@ -25,6 +25,7 @@
 import com.google.common.collect.Sets;
 import com.google.common.util.concurrent.internal.InternalFutureFailureAccess;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.Set;
@@ -213,6 +214,44 @@ public void testToString_allUnique() throws Exception {
     assertThat(SettableFuture.create().toString()).isNotEqualTo(SettableFuture.create().toString());
   }
 
+  public void testToString_oom() throws Exception {
+    SettableFuture<Object> future = SettableFuture.create();
+    future.set(
+        new Object() {
+          @Override
+          public String toString() {
+            throw new OutOfMemoryError();
+          }
+
+          @Override
+          public int hashCode() {
+            throw new OutOfMemoryError();
+          }
+        });
+
+    String unused = future.toString();
+
+    SettableFuture<Object> future2 = SettableFuture.create();
+
+    // A more organic OOM from a toString implementation
+    Object object =
+        new Object() {
+          @Override
+          public String toString() {
+            return new String(new char[50_000]);
+          }
+        };
+    List<Object> list = Collections.singletonList(object);
+    for (int i = 0; i < 10; i++) {
+      Object[] array = new Object[500];
+      Arrays.fill(array, list);
+      list = Arrays.asList(array);
+    }
+    future2.set(list);
+
+    unused = future.toString();
+  }
+
   public void testToString_notDone() throws Exception {
     AbstractFuture<Object> testFuture =
         new AbstractFuture<Object>() {
@@ -243,7 +282,8 @@ public String pendingToString() {
             return "cause=[Because this test isn't done]";
           }
         };
-    assertThat(testFuture.toString()).matches("[^\\[]+\\[status=SUCCESS, result=\\[true\\]\\]");
+    assertThat(testFuture.toString())
+        .matches("[^\\[]+\\[status=SUCCESS, result=\\[java.lang.Boolean@\\w+\\]\\]");
   }
 
   /**
@@ -308,7 +348,7 @@ public String pendingToString() {
                 + " info=\\[cause=\\[Someday...]]]]]");
     testFuture2.set("result string");
     assertThat(testFuture3.toString())
-        .matches("[^\\[]+\\[status=SUCCESS, result=\\[result string\\]\\]");
+        .matches("[^\\[]+\\[status=SUCCESS, result=\\[java.lang.String@\\w+\\]\\]");
   }
 
   public void testToString_cancelled() throws Exception {
@@ -944,11 +984,11 @@ public String toString() {
   public void testSetIndirectSelf_toString() {
     final SettableFuture<Object> orig = SettableFuture.create();
     // unlike the above this indirection defeats the trivial cycle detection and causes a SOE
-    orig.set(
-        new Object() {
+    orig.setFuture(
+        new ForwardingListenableFuture<Object>() {
           @Override
-          public String toString() {
-            return orig.toString();
+          protected ListenableFuture<Object> delegate() {
+            return orig;
           }
         });
     assertThat(orig.toString())

diff --git a/guava-tests/test/com/google/common/util/concurrent/FuturesTest.java b/guava-tests/test/com/google/common/util/concurrent/FuturesTest.java
@@ -2627,8 +2627,8 @@ public ListenableFuture<String> call() throws Exception {
     assertThat(futureResult.toString())
         .matches(
             "CombinedFuture@\\w+\\[status=PENDING,"
-                + " info=\\[futures=\\[SettableFuture@\\w+\\[status=SUCCESS, result=\\[1]],"
-                + " SettableFuture@\\w+\\[status=PENDING]]]]");
+                + " info=\\[futures=\\[SettableFuture@\\w+\\[status=SUCCESS,"
+                + " result=\\[java.lang.Integer@\\w+]], SettableFuture@\\w+\\[status=PENDING]]]]");
 
     // Backing futures complete
     Boolean booleanPartial = true;
@@ -2649,7 +2649,7 @@ public ListenableFuture<String> call() throws Exception {
     String expectedResult = createCombinedResult(integerPartial, booleanPartial);
     assertEquals(expectedResult, futureResult.get());
     assertThat(futureResult.toString())
-        .matches("CombinedFuture@\\w+\\[status=SUCCESS, result=\\[" + expectedResult + "]]");
+        .matches("CombinedFuture@\\w+\\[status=SUCCESS, result=\\[java.lang.String@\\w+]]");
   }
 
   public void testWhenAllComplete_asyncError() throws Exception {

diff --git a/guava/src/com/google/common/util/concurrent/AbstractFuture.java b/guava/src/com/google/common/util/concurrent/AbstractFuture.java
@@ -1156,7 +1156,7 @@ private void addDoneString(StringBuilder builder) {
     try {
       V value = getUninterruptibly(this);
       builder.append("SUCCESS, result=[");
-      appendUserObject(builder, value);
+      appendResultObject(builder, value);
       builder.append("]");
     } catch (ExecutionException e) {
       builder.append("FAILURE, cause=[").append(e.getCause()).append("]");
@@ -1167,6 +1167,24 @@ private void addDoneString(StringBuilder builder) {
     }
   }
 
+  /**
+   * Any object can be the result of a Future, and not every object has a reasonable toString()
+   * implementation. Using a reconstruction of the default Object.toString() prevents OOMs and stack
+   * overflows, and helps avoid sensitive data inadvertently ending up in exception messages.
+   */
+  private void appendResultObject(StringBuilder builder, Object o) {
+    if (o == null) {
+      builder.append("null");
+    } else if (o == this) {
+      builder.append("this future");
+    } else {
+      builder
+          .append(o.getClass().getName())
+          .append("@")
+          .append(Integer.toHexString(System.identityHashCode(o)));
+    }
+  }
+
   /** Helper for printing user supplied objects into our toString method. */
   private void appendUserObject(StringBuilder builder, Object o) {
     // This is some basic recursion detection for when people create cycles via set/setFuture or