From 2ebf27fd45685e7f89bb4aae11e1709d49863717 Mon Sep 17 00:00:00 2001 From: clm Date: Tue, 13 Oct 2020 10:54:32 -0700 Subject: [PATCH] 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 --- .../util/concurrent/AbstractFutureTest.java | 52 ++++++++++++++++--- .../common/util/concurrent/FuturesTest.java | 6 +-- .../util/concurrent/AbstractFuture.java | 20 ++++++- .../util/concurrent/AbstractFutureTest.java | 52 ++++++++++++++++--- .../common/util/concurrent/FuturesTest.java | 6 +-- .../util/concurrent/AbstractFuture.java | 20 ++++++- 6 files changed, 136 insertions(+), 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 index 1ef7cbbe826f..3c210ce7e768 100644 --- 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 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 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 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 testFuture = new AbstractFuture() { @@ -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 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() { @Override - public String toString() { - return orig.toString(); + protected ListenableFuture 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 index 4445aab46b5c..797ee4882d40 100644 --- 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 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 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 index 2e77ab7cc76b..e7992415c9c7 100644 --- 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 index 1ef7cbbe826f..3c210ce7e768 100644 --- 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 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 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 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 testFuture = new AbstractFuture() { @@ -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 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() { @Override - public String toString() { - return orig.toString(); + protected ListenableFuture 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 index 6c834db14f03..9586a65a1b43 100644 --- 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 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 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 index dd863df05152..06feb5a21473 100644 --- 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