From ddd4a49fb760ccccc7903305f68310f4f9a90587 Mon Sep 17 00:00:00 2001 From: cgdecker Date: Mon, 5 Aug 2019 10:21:14 -0700 Subject: [PATCH] Fix an issue where the InputStream returned by BaseEncoding.decodingStream(Reader) could fail to throw DecodingException while decoding an invalid string. This was caused by the default behavior of InputStream.read(byte[], int, int), which swallows any IOException thrown by any call to the single-byte read() method other than the first. To fix it, just override that method with an implementation that does not swallow any exceptions. Fixes https://github.com/google/guava/issues/3542 RELNOTES=n/a ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=261712883 --- .../google/common/io/BaseEncodingTest.java | 83 +++++++++++++++---- .../com/google/common/io/BaseEncoding.java | 21 +++++ .../google/common/io/BaseEncodingTest.java | 83 +++++++++++++++---- .../com/google/common/io/BaseEncoding.java | 21 +++++ 4 files changed, 178 insertions(+), 30 deletions(-) diff --git a/android/guava-tests/test/com/google/common/io/BaseEncodingTest.java b/android/guava-tests/test/com/google/common/io/BaseEncodingTest.java index 17bc3250d4d4..e3d65916ced5 100644 --- a/android/guava-tests/test/com/google/common/io/BaseEncodingTest.java +++ b/android/guava-tests/test/com/google/common/io/BaseEncodingTest.java @@ -31,6 +31,7 @@ import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; +import java.io.Reader; import java.io.StringReader; import java.io.StringWriter; import junit.framework.TestCase; @@ -389,23 +390,75 @@ private static void assertFailsToDecode(BaseEncoding encoding, String cannotDeco private static void assertFailsToDecode( BaseEncoding encoding, String cannotDecode, @NullableDecl String expectedMessage) { - assertFalse(encoding.canDecode(cannotDecode)); - try { - encoding.decode(cannotDecode); - fail("Expected IllegalArgumentException"); - } catch (IllegalArgumentException expected) { - if (expectedMessage != null) { - assertThat(expected).hasCauseThat().hasMessageThat().isEqualTo(expectedMessage); - } + // We use this somewhat weird pattern with an enum for each assertion we want to make as a way + // of dealing with the fact that one of the assertions is @GwtIncompatible but we don't want to + // have to have duplicate @GwtIncompatible test methods just to make that assertion. + for (AssertFailsToDecodeStrategy strategy : AssertFailsToDecodeStrategy.values()) { + strategy.assertFailsToDecode(encoding, cannotDecode, expectedMessage); } - try { - encoding.decodeChecked(cannotDecode); - fail("Expected DecodingException"); - } catch (DecodingException expected) { - if (expectedMessage != null) { - assertThat(expected).hasMessageThat().isEqualTo(expectedMessage); + } + + enum AssertFailsToDecodeStrategy { + @GwtIncompatible // decodingStream(Reader) + DECODING_STREAM { + @Override + void assertFailsToDecode( + BaseEncoding encoding, String cannotDecode, @NullableDecl String expectedMessage) { + // Regression test for case where DecodingException was swallowed by default implementation + // of + // InputStream.read(byte[], int, int) + // See https://github.com/google/guava/issues/3542 + Reader reader = new StringReader(cannotDecode); + InputStream decodingStream = encoding.decodingStream(reader); + try { + ByteStreams.exhaust(decodingStream); + fail("Expected DecodingException"); + } catch (DecodingException expected) { + // Don't assert on the expectedMessage; the messages for exceptions thrown from the + // decoding stream may differ from the messages for the decode methods. + } catch (IOException e) { + fail("Expected DecodingException but got: " + e); + } } - } + }, + CAN_DECODE { + @Override + void assertFailsToDecode( + BaseEncoding encoding, String cannotDecode, @NullableDecl String expectedMessage) { + assertFalse(encoding.canDecode(cannotDecode)); + } + }, + DECODE { + @Override + void assertFailsToDecode( + BaseEncoding encoding, String cannotDecode, @NullableDecl String expectedMessage) { + try { + encoding.decode(cannotDecode); + fail("Expected IllegalArgumentException"); + } catch (IllegalArgumentException expected) { + if (expectedMessage != null) { + assertThat(expected).hasCauseThat().hasMessageThat().isEqualTo(expectedMessage); + } + } + } + }, + DECODE_CHECKED { + @Override + void assertFailsToDecode( + BaseEncoding encoding, String cannotDecode, @NullableDecl String expectedMessage) { + try { + encoding.decodeChecked(cannotDecode); + fail("Expected DecodingException"); + } catch (DecodingException expected) { + if (expectedMessage != null) { + assertThat(expected).hasMessageThat().isEqualTo(expectedMessage); + } + } + } + }; + + abstract void assertFailsToDecode( + BaseEncoding encoding, String cannotDecode, @NullableDecl String expectedMessage); } @GwtIncompatible // Reader/Writer diff --git a/android/guava/src/com/google/common/io/BaseEncoding.java b/android/guava/src/com/google/common/io/BaseEncoding.java index d7f87e6335b1..86a19493bbcf 100644 --- a/android/guava/src/com/google/common/io/BaseEncoding.java +++ b/android/guava/src/com/google/common/io/BaseEncoding.java @@ -771,6 +771,27 @@ public int read() throws IOException { } } + @Override + public int read(byte[] buf, int off, int len) throws IOException { + // Overriding this to work around the fact that InputStream's default implementation of + // this method will silently swallow exceptions thrown by the single-byte read() method + // (other than on the first call to it), which in this case can cause invalid encoded + // strings to not throw an exception. + // See https://github.com/google/guava/issues/3542 + checkPositionIndexes(off, off + len, buf.length); + + int i = off; + for (; i < off + len; i++) { + int b = read(); + if (b == -1) { + int read = i - off; + return read == 0 ? -1 : read; + } + buf[i] = (byte) b; + } + return i - off; + } + @Override public void close() throws IOException { reader.close(); diff --git a/guava-tests/test/com/google/common/io/BaseEncodingTest.java b/guava-tests/test/com/google/common/io/BaseEncodingTest.java index d92bb0e9d84a..5547f28c0031 100644 --- a/guava-tests/test/com/google/common/io/BaseEncodingTest.java +++ b/guava-tests/test/com/google/common/io/BaseEncodingTest.java @@ -31,6 +31,7 @@ import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; +import java.io.Reader; import java.io.StringReader; import java.io.StringWriter; import junit.framework.TestCase; @@ -389,23 +390,75 @@ private static void assertFailsToDecode(BaseEncoding encoding, String cannotDeco private static void assertFailsToDecode( BaseEncoding encoding, String cannotDecode, @Nullable String expectedMessage) { - assertFalse(encoding.canDecode(cannotDecode)); - try { - encoding.decode(cannotDecode); - fail("Expected IllegalArgumentException"); - } catch (IllegalArgumentException expected) { - if (expectedMessage != null) { - assertThat(expected).hasCauseThat().hasMessageThat().isEqualTo(expectedMessage); - } + // We use this somewhat weird pattern with an enum for each assertion we want to make as a way + // of dealing with the fact that one of the assertions is @GwtIncompatible but we don't want to + // have to have duplicate @GwtIncompatible test methods just to make that assertion. + for (AssertFailsToDecodeStrategy strategy : AssertFailsToDecodeStrategy.values()) { + strategy.assertFailsToDecode(encoding, cannotDecode, expectedMessage); } - try { - encoding.decodeChecked(cannotDecode); - fail("Expected DecodingException"); - } catch (DecodingException expected) { - if (expectedMessage != null) { - assertThat(expected).hasMessageThat().isEqualTo(expectedMessage); + } + + enum AssertFailsToDecodeStrategy { + @GwtIncompatible // decodingStream(Reader) + DECODING_STREAM { + @Override + void assertFailsToDecode( + BaseEncoding encoding, String cannotDecode, @Nullable String expectedMessage) { + // Regression test for case where DecodingException was swallowed by default implementation + // of + // InputStream.read(byte[], int, int) + // See https://github.com/google/guava/issues/3542 + Reader reader = new StringReader(cannotDecode); + InputStream decodingStream = encoding.decodingStream(reader); + try { + ByteStreams.exhaust(decodingStream); + fail("Expected DecodingException"); + } catch (DecodingException expected) { + // Don't assert on the expectedMessage; the messages for exceptions thrown from the + // decoding stream may differ from the messages for the decode methods. + } catch (IOException e) { + fail("Expected DecodingException but got: " + e); + } } - } + }, + CAN_DECODE { + @Override + void assertFailsToDecode( + BaseEncoding encoding, String cannotDecode, @Nullable String expectedMessage) { + assertFalse(encoding.canDecode(cannotDecode)); + } + }, + DECODE { + @Override + void assertFailsToDecode( + BaseEncoding encoding, String cannotDecode, @Nullable String expectedMessage) { + try { + encoding.decode(cannotDecode); + fail("Expected IllegalArgumentException"); + } catch (IllegalArgumentException expected) { + if (expectedMessage != null) { + assertThat(expected).hasCauseThat().hasMessageThat().isEqualTo(expectedMessage); + } + } + } + }, + DECODE_CHECKED { + @Override + void assertFailsToDecode( + BaseEncoding encoding, String cannotDecode, @Nullable String expectedMessage) { + try { + encoding.decodeChecked(cannotDecode); + fail("Expected DecodingException"); + } catch (DecodingException expected) { + if (expectedMessage != null) { + assertThat(expected).hasMessageThat().isEqualTo(expectedMessage); + } + } + } + }; + + abstract void assertFailsToDecode( + BaseEncoding encoding, String cannotDecode, @Nullable String expectedMessage); } @GwtIncompatible // Reader/Writer diff --git a/guava/src/com/google/common/io/BaseEncoding.java b/guava/src/com/google/common/io/BaseEncoding.java index d8d4797f1c9a..f3b3983169ab 100644 --- a/guava/src/com/google/common/io/BaseEncoding.java +++ b/guava/src/com/google/common/io/BaseEncoding.java @@ -771,6 +771,27 @@ public int read() throws IOException { } } + @Override + public int read(byte[] buf, int off, int len) throws IOException { + // Overriding this to work around the fact that InputStream's default implementation of + // this method will silently swallow exceptions thrown by the single-byte read() method + // (other than on the first call to it), which in this case can cause invalid encoded + // strings to not throw an exception. + // See https://github.com/google/guava/issues/3542 + checkPositionIndexes(off, off + len, buf.length); + + int i = off; + for (; i < off + len; i++) { + int b = read(); + if (b == -1) { + int read = i - off; + return read == 0 ? -1 : read; + } + buf[i] = (byte) b; + } + return i - off; + } + @Override public void close() throws IOException { reader.close();