From 59ec98c505c17b3efa882522c3312022d1ca88da Mon Sep 17 00:00:00 2001 From: Sam Judd Date: Wed, 6 Jan 2021 11:27:25 -0800 Subject: [PATCH] Make Exception wrapper throw instead of returning -1/0 This will hopefully fix issues like https://github.com/bumptech/glide/issues/4438. PiperOrigin-RevId: 350393488 --- .../resource/bitmap/StreamBitmapDecoder.java | 10 +- .../util/ExceptionCatchingInputStream.java | 5 + .../util/ExceptionPassthroughInputStream.java | 139 +++++++++++ .../ExceptionCatchingInputStreamTest.java | 173 -------------- .../ExceptionPassthroughInputStreamTest.java | 220 ++++++++++++++++++ 5 files changed, 369 insertions(+), 178 deletions(-) create mode 100644 library/src/main/java/com/bumptech/glide/util/ExceptionPassthroughInputStream.java delete mode 100644 library/test/src/test/java/com/bumptech/glide/util/ExceptionCatchingInputStreamTest.java create mode 100644 library/test/src/test/java/com/bumptech/glide/util/ExceptionPassthroughInputStreamTest.java diff --git a/library/src/main/java/com/bumptech/glide/load/resource/bitmap/StreamBitmapDecoder.java b/library/src/main/java/com/bumptech/glide/load/resource/bitmap/StreamBitmapDecoder.java index 8d977e0fd0..866aaa15d8 100644 --- a/library/src/main/java/com/bumptech/glide/load/resource/bitmap/StreamBitmapDecoder.java +++ b/library/src/main/java/com/bumptech/glide/load/resource/bitmap/StreamBitmapDecoder.java @@ -7,7 +7,7 @@ import com.bumptech.glide.load.engine.Resource; import com.bumptech.glide.load.engine.bitmap_recycle.ArrayPool; import com.bumptech.glide.load.engine.bitmap_recycle.BitmapPool; -import com.bumptech.glide.util.ExceptionCatchingInputStream; +import com.bumptech.glide.util.ExceptionPassthroughInputStream; import com.bumptech.glide.util.MarkEnforcingInputStream; import java.io.IOException; import java.io.InputStream; @@ -49,8 +49,8 @@ public Resource decode( // Use to retrieve exceptions thrown while reading. // TODO(#126): when the framework no longer returns partially decoded Bitmaps or provides a // way to determine if a Bitmap is partially decoded, consider removing. - ExceptionCatchingInputStream exceptionStream = - ExceptionCatchingInputStream.obtain(bufferedStream); + ExceptionPassthroughInputStream exceptionStream = + ExceptionPassthroughInputStream.obtain(bufferedStream); // Use to read data. // Ensures that we can always reset after reading an image header so that we can still @@ -74,11 +74,11 @@ public Resource decode( */ static class UntrustedCallbacks implements Downsampler.DecodeCallbacks { private final RecyclableBufferedInputStream bufferedStream; - private final ExceptionCatchingInputStream exceptionStream; + private final ExceptionPassthroughInputStream exceptionStream; UntrustedCallbacks( RecyclableBufferedInputStream bufferedStream, - ExceptionCatchingInputStream exceptionStream) { + ExceptionPassthroughInputStream exceptionStream) { this.bufferedStream = bufferedStream; this.exceptionStream = exceptionStream; } diff --git a/library/src/main/java/com/bumptech/glide/util/ExceptionCatchingInputStream.java b/library/src/main/java/com/bumptech/glide/util/ExceptionCatchingInputStream.java index 9dc2244012..2fe220e770 100644 --- a/library/src/main/java/com/bumptech/glide/util/ExceptionCatchingInputStream.java +++ b/library/src/main/java/com/bumptech/glide/util/ExceptionCatchingInputStream.java @@ -13,7 +13,12 @@ * android.graphics.BitmapFactory} can return partially decoded bitmaps. * *

See https://github.com/bumptech/glide/issues/126. + * + * @deprecated In some cases, callers may not handle getting 0 or -1 return values from methods, + * which can lead to infinite loops (see #4438). Use {@link ExceptionPassthroughInputStream} + * instead. This class will be deleted in a future version of Glide. */ +@Deprecated public class ExceptionCatchingInputStream extends InputStream { private static final Queue QUEUE = Util.createQueue(0); diff --git a/library/src/main/java/com/bumptech/glide/util/ExceptionPassthroughInputStream.java b/library/src/main/java/com/bumptech/glide/util/ExceptionPassthroughInputStream.java new file mode 100644 index 0000000000..aaf8388474 --- /dev/null +++ b/library/src/main/java/com/bumptech/glide/util/ExceptionPassthroughInputStream.java @@ -0,0 +1,139 @@ +package com.bumptech.glide.util; + +import androidx.annotation.GuardedBy; +import androidx.annotation.NonNull; +import androidx.annotation.Nullable; +import java.io.IOException; +import java.io.InputStream; +import java.util.Queue; + +/** + * An {@link java.io.InputStream} that catches, stores and rethrows {@link java.io.IOException}s + * during read and skip calls. This allows users of this API to handle the exception at a higher + * level if the exception is swallowed by some intermediate library. This class is a workaround for + * a framework issue where exceptions during reads while decoding bitmaps in {@link + * android.graphics.BitmapFactory} can return partially decoded bitmaps. + * + *

Unlike the deprecated {@link ExceptionCatchingInputStream}, this class will both store and + * re-throw any IOExceptions. Rethrowing works around bugs in wrapping streams that may not fully + * obey the stream contract. This is really only useful if some middle layer is going to catch the + * exception (like BitmapFactory) but we want to propagate the exception instead. + * + *

See https://github.com/bumptech/glide/issues/126 and #4438. + */ +public final class ExceptionPassthroughInputStream extends InputStream { + + @GuardedBy("POOL") + private static final Queue POOL = Util.createQueue(0); + + private InputStream wrapped; + private IOException exception; + + @NonNull + public static ExceptionPassthroughInputStream obtain(@NonNull InputStream toWrap) { + ExceptionPassthroughInputStream result; + synchronized (POOL) { + result = POOL.poll(); + } + if (result == null) { + result = new ExceptionPassthroughInputStream(); + } + result.setInputStream(toWrap); + return result; + } + + // Exposed for testing. + static void clearQueue() { + synchronized (POOL) { + while (!POOL.isEmpty()) { + POOL.remove(); + } + } + } + + ExceptionPassthroughInputStream() { + // Do nothing. + } + + void setInputStream(@NonNull InputStream toWrap) { + wrapped = toWrap; + } + + @Override + public int available() throws IOException { + return wrapped.available(); + } + + @Override + public void close() throws IOException { + wrapped.close(); + } + + @Override + public void mark(int readLimit) { + wrapped.mark(readLimit); + } + + @Override + public boolean markSupported() { + return wrapped.markSupported(); + } + + @Override + public int read() throws IOException { + try { + return wrapped.read(); + } catch (IOException e) { + exception = e; + throw e; + } + } + + @Override + public int read(byte[] buffer) throws IOException { + try { + return wrapped.read(buffer); + } catch (IOException e) { + exception = e; + throw e; + } + } + + @Override + public int read(byte[] buffer, int byteOffset, int byteCount) throws IOException { + try { + return wrapped.read(buffer, byteOffset, byteCount); + } catch (IOException e) { + exception = e; + throw e; + } + } + + @Override + public synchronized void reset() throws IOException { + wrapped.reset(); + } + + @Override + public long skip(long byteCount) throws IOException { + try { + return wrapped.skip(byteCount); + } catch (IOException e) { + exception = e; + throw e; + } + } + + @Nullable + public IOException getException() { + return exception; + } + + public void release() { + exception = null; + wrapped = null; + synchronized (POOL) { + POOL.offer(this); + } + } +} diff --git a/library/test/src/test/java/com/bumptech/glide/util/ExceptionCatchingInputStreamTest.java b/library/test/src/test/java/com/bumptech/glide/util/ExceptionCatchingInputStreamTest.java deleted file mode 100644 index 28551162d1..0000000000 --- a/library/test/src/test/java/com/bumptech/glide/util/ExceptionCatchingInputStreamTest.java +++ /dev/null @@ -1,173 +0,0 @@ -package com.bumptech.glide.util; - -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyInt; -import static org.mockito.ArgumentMatchers.anyLong; -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; - -import com.bumptech.glide.load.resource.bitmap.RecyclableBufferedInputStream; -import java.io.IOException; -import java.io.InputStream; -import java.net.SocketTimeoutException; -import org.junit.After; -import org.junit.Before; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -@RunWith(JUnit4.class) -public class ExceptionCatchingInputStreamTest { - - private RecyclableBufferedInputStream wrapped; - private ExceptionCatchingInputStream is; - - @Before - public void setUp() throws Exception { - wrapped = mock(RecyclableBufferedInputStream.class); - is = new ExceptionCatchingInputStream(); - is.setInputStream(wrapped); - } - - @After - public void tearDown() { - ExceptionCatchingInputStream.clearQueue(); - } - - @Test - public void testReturnsWrappedAvailable() throws IOException { - when(wrapped.available()).thenReturn(25); - assertEquals(25, is.available()); - } - - @Test - public void testCallsCloseOnWrapped() throws IOException { - is.close(); - verify(wrapped).close(); - } - - @Test - public void testCallsMarkOnWrapped() { - int toMark = 50; - is.mark(toMark); - verify(wrapped).mark(eq(toMark)); - } - - @Test - public void testReturnsWrappedMarkSupported() { - when(wrapped.markSupported()).thenReturn(true); - assertTrue(is.markSupported()); - } - - @Test - public void testCallsReadByteArrayOnWrapped() throws IOException { - byte[] buffer = new byte[100]; - when(wrapped.read(eq(buffer))).thenReturn(buffer.length); - assertEquals(buffer.length, is.read(buffer)); - } - - @Test - public void testCallsReadArrayWithOffsetAndCountOnWrapped() throws IOException { - int offset = 5; - int count = 100; - byte[] buffer = new byte[105]; - - when(wrapped.read(eq(buffer), eq(offset), eq(count))).thenReturn(count); - assertEquals(count, is.read(buffer, offset, count)); - } - - @Test - public void testCallsReadOnWrapped() throws IOException { - when(wrapped.read()).thenReturn(1); - assertEquals(1, is.read()); - } - - @Test - public void testCallsResetOnWrapped() throws IOException { - is.reset(); - verify(wrapped).reset(); - } - - @Test - public void testCallsSkipOnWrapped() throws IOException { - long toSkip = 67; - long expected = 55; - when(wrapped.skip(eq(toSkip))).thenReturn(expected); - assertEquals(expected, is.skip(toSkip)); - } - - @Test - public void testCatchesExceptionOnRead() throws IOException { - IOException expected = new SocketTimeoutException(); - when(wrapped.read()).thenThrow(expected); - int read = is.read(); - - assertEquals(-1, read); - assertEquals(expected, is.getException()); - } - - @Test - public void testCatchesExceptionOnReadBuffer() throws IOException { - IOException exception = new SocketTimeoutException(); - when(wrapped.read(any(byte[].class))).thenThrow(exception); - - int read = is.read(new byte[0]); - assertEquals(-1, read); - assertEquals(exception, is.getException()); - } - - @Test - public void testCatchesExceptionOnReadBufferWithOffsetAndCount() throws IOException { - IOException exception = new SocketTimeoutException(); - when(wrapped.read(any(byte[].class), anyInt(), anyInt())).thenThrow(exception); - - int read = is.read(new byte[0], 10, 100); - assertEquals(-1, read); - assertEquals(exception, is.getException()); - } - - @Test - public void testCatchesExceptionOnSkip() throws IOException { - IOException exception = new SocketTimeoutException(); - when(wrapped.skip(anyLong())).thenThrow(exception); - - long skipped = is.skip(100); - assertEquals(0, skipped); - assertEquals(exception, is.getException()); - } - - @Test - public void testExceptionIsNotSetInitially() { - assertNull(is.getException()); - } - - @SuppressWarnings("ResultOfMethodCallIgnored") - @Test - public void testResetsExceptionToNullOnRelease() throws IOException { - IOException exception = new SocketTimeoutException(); - when(wrapped.read()).thenThrow(exception); - is.read(); - is.release(); - assertNull(is.getException()); - } - - @Test - public void testCanReleaseAnObtainFromPool() { - is.release(); - InputStream fromPool = ExceptionCatchingInputStream.obtain(wrapped); - assertEquals(is, fromPool); - } - - @Test - public void testCanObtainNewStreamFromPool() throws IOException { - InputStream fromPool = ExceptionCatchingInputStream.obtain(wrapped); - when(wrapped.read()).thenReturn(1); - int read = fromPool.read(); - assertEquals(1, read); - } -} diff --git a/library/test/src/test/java/com/bumptech/glide/util/ExceptionPassthroughInputStreamTest.java b/library/test/src/test/java/com/bumptech/glide/util/ExceptionPassthroughInputStreamTest.java new file mode 100644 index 0000000000..51f2e17c13 --- /dev/null +++ b/library/test/src/test/java/com/bumptech/glide/util/ExceptionPassthroughInputStreamTest.java @@ -0,0 +1,220 @@ +package com.bumptech.glide.util; + +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.net.SocketTimeoutException; +import java.util.concurrent.atomic.AtomicBoolean; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.function.ThrowingRunnable; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class ExceptionPassthroughInputStreamTest { + + private final InputStream validInputStream = + new ByteArrayInputStream( + new byte[] { + 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, + }); + private final InputStream throwingInputStream = new ExceptionThrowingInputStream(); + private ExceptionPassthroughInputStream validWrapper; + private ExceptionPassthroughInputStream throwingWrapper; + + @Before + public void setUp() throws Exception { + validWrapper = new ExceptionPassthroughInputStream(); + validWrapper.setInputStream(validInputStream); + throwingWrapper = new ExceptionPassthroughInputStream(); + throwingWrapper.setInputStream(throwingInputStream); + } + + @After + public void tearDown() { + ExceptionPassthroughInputStream.clearQueue(); + } + + @Test + public void testReturnsWrappedAvailable() throws IOException { + assertEquals(validInputStream.available(), validWrapper.available()); + } + + @Test + public void testCallsCloseOnWrapped() throws IOException { + ExceptionPassthroughInputStream wrapper = new ExceptionPassthroughInputStream(); + final AtomicBoolean isClosed = new AtomicBoolean(); + wrapper.setInputStream( + new InputStream() { + @Override + public int read() { + return 0; + } + + @Override + public void close() throws IOException { + super.close(); + isClosed.set(true); + } + }); + wrapper.close(); + assertThat(isClosed.get()).isTrue(); + } + + @Test + public void testCallsMarkOnWrapped() throws IOException { + int toMark = 5; + validWrapper.mark(toMark); + assertThat(validWrapper.read(new byte[5], 0, 5)).isEqualTo(5); + validInputStream.reset(); + assertThat(validInputStream.read()).isEqualTo(0); + } + + @Test + public void testReturnsWrappedMarkSupported() { + assertTrue(validWrapper.markSupported()); + } + + @Test + public void testCallsReadByteArrayOnWrapped() throws IOException { + byte[] buffer = new byte[8]; + assertEquals(buffer.length, validWrapper.read(buffer)); + } + + @Test + public void testCallsReadArrayWithOffsetAndCountOnWrapped() throws IOException { + int offset = 1; + int count = 4; + byte[] buffer = new byte[5]; + + assertEquals(count, validWrapper.read(buffer, offset, count)); + } + + @Test + public void testCallsReadOnWrapped() throws IOException { + assertEquals(0, validWrapper.read()); + assertEquals(1, validWrapper.read()); + assertEquals(2, validWrapper.read()); + } + + @Test + public void testCallsResetOnWrapped() throws IOException { + validWrapper.mark(5); + assertThat(validWrapper.read()).isEqualTo(0); + assertThat(validWrapper.read()).isEqualTo(1); + validWrapper.reset(); + assertThat(validWrapper.read()).isEqualTo(0); + } + + @Test + public void testCallsSkipOnWrapped() throws IOException { + int toSkip = 5; + assertThat(validWrapper.skip(toSkip)).isEqualTo(toSkip); + assertThat(validWrapper.read()).isEqualTo(5); + } + + @Test + public void testCatchesExceptionOnRead() { + SocketTimeoutException expected = + assertThrows( + SocketTimeoutException.class, + new ThrowingRunnable() { + @Override + public void run() throws Throwable { + throwingWrapper.read(); + } + }); + assertEquals(expected, throwingWrapper.getException()); + } + + @Test + public void testCatchesExceptionOnReadBuffer() { + SocketTimeoutException exception = + assertThrows( + SocketTimeoutException.class, + new ThrowingRunnable() { + @Override + public void run() throws Throwable { + throwingWrapper.read(new byte[1]); + } + }); + assertEquals(exception, throwingWrapper.getException()); + } + + @Test + public void testCatchesExceptionOnReadBufferWithOffsetAndCount() { + SocketTimeoutException exception = + assertThrows( + SocketTimeoutException.class, + new ThrowingRunnable() { + @Override + public void run() throws Throwable { + throwingWrapper.read(new byte[2], 1, 1); + } + }); + assertEquals(exception, throwingWrapper.getException()); + } + + @Test + public void testCatchesExceptionOnSkip() { + SocketTimeoutException exception = + assertThrows( + SocketTimeoutException.class, + new ThrowingRunnable() { + @Override + public void run() throws Throwable { + throwingWrapper.skip(100); + } + }); + assertEquals(exception, throwingWrapper.getException()); + } + + @Test + public void testExceptionIsNotSetInitially() { + assertNull(validWrapper.getException()); + } + + @SuppressWarnings("ResultOfMethodCallIgnored") + @Test + public void testResetsExceptionToNullOnRelease() { + assertThrows( + SocketTimeoutException.class, + new ThrowingRunnable() { + @Override + public void run() throws Throwable { + throwingWrapper.read(); + } + }); + throwingWrapper.release(); + assertNull(validWrapper.getException()); + } + + @Test + public void testCanReleaseAnObtainFromPool() { + validWrapper.release(); + InputStream fromPool = ExceptionPassthroughInputStream.obtain(validInputStream); + assertEquals(validWrapper, fromPool); + } + + @Test + public void testCanObtainNewStreamFromPool() throws IOException { + InputStream fromPool = ExceptionPassthroughInputStream.obtain(validInputStream); + int read = fromPool.read(); + assertEquals(0, read); + } + + private static final class ExceptionThrowingInputStream extends InputStream { + @Override + public int read() throws IOException { + throw new SocketTimeoutException(); + } + } +}