Skip to content

Commit ddd4a49

Browse files
cgdeckernetdpb
cgdecker
authored and
netdpb
committed
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 #3542 RELNOTES=n/a ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=261712883
1 parent a3c9f2c commit ddd4a49

File tree

4 files changed

+178
-30
lines changed

4 files changed

+178
-30
lines changed

‎android/guava-tests/test/com/google/common/io/BaseEncodingTest.java‎

Lines changed: 68 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import java.io.IOException;
3232
import java.io.InputStream;
3333
import java.io.OutputStream;
34+
import java.io.Reader;
3435
import java.io.StringReader;
3536
import java.io.StringWriter;
3637
import junit.framework.TestCase;
@@ -389,23 +390,75 @@ private static void assertFailsToDecode(BaseEncoding encoding, String cannotDeco
389390

390391
private static void assertFailsToDecode(
391392
BaseEncoding encoding, String cannotDecode, @NullableDecl String expectedMessage) {
392-
assertFalse(encoding.canDecode(cannotDecode));
393-
try {
394-
encoding.decode(cannotDecode);
395-
fail("Expected IllegalArgumentException");
396-
} catch (IllegalArgumentException expected) {
397-
if (expectedMessage != null) {
398-
assertThat(expected).hasCauseThat().hasMessageThat().isEqualTo(expectedMessage);
399-
}
393+
// We use this somewhat weird pattern with an enum for each assertion we want to make as a way
394+
// of dealing with the fact that one of the assertions is @GwtIncompatible but we don't want to
395+
// have to have duplicate @GwtIncompatible test methods just to make that assertion.
396+
for (AssertFailsToDecodeStrategy strategy : AssertFailsToDecodeStrategy.values()) {
397+
strategy.assertFailsToDecode(encoding, cannotDecode, expectedMessage);
400398
}
401-
try {
402-
encoding.decodeChecked(cannotDecode);
403-
fail("Expected DecodingException");
404-
} catch (DecodingException expected) {
405-
if (expectedMessage != null) {
406-
assertThat(expected).hasMessageThat().isEqualTo(expectedMessage);
399+
}
400+
401+
enum AssertFailsToDecodeStrategy {
402+
@GwtIncompatible // decodingStream(Reader)
403+
DECODING_STREAM {
404+
@Override
405+
void assertFailsToDecode(
406+
BaseEncoding encoding, String cannotDecode, @NullableDecl String expectedMessage) {
407+
// Regression test for case where DecodingException was swallowed by default implementation
408+
// of
409+
// InputStream.read(byte[], int, int)
410+
// See https://github.com/google/guava/issues/3542
411+
Reader reader = new StringReader(cannotDecode);
412+
InputStream decodingStream = encoding.decodingStream(reader);
413+
try {
414+
ByteStreams.exhaust(decodingStream);
415+
fail("Expected DecodingException");
416+
} catch (DecodingException expected) {
417+
// Don't assert on the expectedMessage; the messages for exceptions thrown from the
418+
// decoding stream may differ from the messages for the decode methods.
419+
} catch (IOException e) {
420+
fail("Expected DecodingException but got: " + e);
421+
}
407422
}
408-
}
423+
},
424+
CAN_DECODE {
425+
@Override
426+
void assertFailsToDecode(
427+
BaseEncoding encoding, String cannotDecode, @NullableDecl String expectedMessage) {
428+
assertFalse(encoding.canDecode(cannotDecode));
429+
}
430+
},
431+
DECODE {
432+
@Override
433+
void assertFailsToDecode(
434+
BaseEncoding encoding, String cannotDecode, @NullableDecl String expectedMessage) {
435+
try {
436+
encoding.decode(cannotDecode);
437+
fail("Expected IllegalArgumentException");
438+
} catch (IllegalArgumentException expected) {
439+
if (expectedMessage != null) {
440+
assertThat(expected).hasCauseThat().hasMessageThat().isEqualTo(expectedMessage);
441+
}
442+
}
443+
}
444+
},
445+
DECODE_CHECKED {
446+
@Override
447+
void assertFailsToDecode(
448+
BaseEncoding encoding, String cannotDecode, @NullableDecl String expectedMessage) {
449+
try {
450+
encoding.decodeChecked(cannotDecode);
451+
fail("Expected DecodingException");
452+
} catch (DecodingException expected) {
453+
if (expectedMessage != null) {
454+
assertThat(expected).hasMessageThat().isEqualTo(expectedMessage);
455+
}
456+
}
457+
}
458+
};
459+
460+
abstract void assertFailsToDecode(
461+
BaseEncoding encoding, String cannotDecode, @NullableDecl String expectedMessage);
409462
}
410463

411464
@GwtIncompatible // Reader/Writer

‎android/guava/src/com/google/common/io/BaseEncoding.java‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -771,6 +771,27 @@ public int read() throws IOException {
771771
}
772772
}
773773

774+
@Override
775+
public int read(byte[] buf, int off, int len) throws IOException {
776+
// Overriding this to work around the fact that InputStream's default implementation of
777+
// this method will silently swallow exceptions thrown by the single-byte read() method
778+
// (other than on the first call to it), which in this case can cause invalid encoded
779+
// strings to not throw an exception.
780+
// See https://github.com/google/guava/issues/3542
781+
checkPositionIndexes(off, off + len, buf.length);
782+
783+
int i = off;
784+
for (; i < off + len; i++) {
785+
int b = read();
786+
if (b == -1) {
787+
int read = i - off;
788+
return read == 0 ? -1 : read;
789+
}
790+
buf[i] = (byte) b;
791+
}
792+
return i - off;
793+
}
794+
774795
@Override
775796
public void close() throws IOException {
776797
reader.close();

‎guava-tests/test/com/google/common/io/BaseEncodingTest.java‎

Lines changed: 68 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import java.io.IOException;
3232
import java.io.InputStream;
3333
import java.io.OutputStream;
34+
import java.io.Reader;
3435
import java.io.StringReader;
3536
import java.io.StringWriter;
3637
import junit.framework.TestCase;
@@ -389,23 +390,75 @@ private static void assertFailsToDecode(BaseEncoding encoding, String cannotDeco
389390

390391
private static void assertFailsToDecode(
391392
BaseEncoding encoding, String cannotDecode, @Nullable String expectedMessage) {
392-
assertFalse(encoding.canDecode(cannotDecode));
393-
try {
394-
encoding.decode(cannotDecode);
395-
fail("Expected IllegalArgumentException");
396-
} catch (IllegalArgumentException expected) {
397-
if (expectedMessage != null) {
398-
assertThat(expected).hasCauseThat().hasMessageThat().isEqualTo(expectedMessage);
399-
}
393+
// We use this somewhat weird pattern with an enum for each assertion we want to make as a way
394+
// of dealing with the fact that one of the assertions is @GwtIncompatible but we don't want to
395+
// have to have duplicate @GwtIncompatible test methods just to make that assertion.
396+
for (AssertFailsToDecodeStrategy strategy : AssertFailsToDecodeStrategy.values()) {
397+
strategy.assertFailsToDecode(encoding, cannotDecode, expectedMessage);
400398
}
401-
try {
402-
encoding.decodeChecked(cannotDecode);
403-
fail("Expected DecodingException");
404-
} catch (DecodingException expected) {
405-
if (expectedMessage != null) {
406-
assertThat(expected).hasMessageThat().isEqualTo(expectedMessage);
399+
}
400+
401+
enum AssertFailsToDecodeStrategy {
402+
@GwtIncompatible // decodingStream(Reader)
403+
DECODING_STREAM {
404+
@Override
405+
void assertFailsToDecode(
406+
BaseEncoding encoding, String cannotDecode, @Nullable String expectedMessage) {
407+
// Regression test for case where DecodingException was swallowed by default implementation
408+
// of
409+
// InputStream.read(byte[], int, int)
410+
// See https://github.com/google/guava/issues/3542
411+
Reader reader = new StringReader(cannotDecode);
412+
InputStream decodingStream = encoding.decodingStream(reader);
413+
try {
414+
ByteStreams.exhaust(decodingStream);
415+
fail("Expected DecodingException");
416+
} catch (DecodingException expected) {
417+
// Don't assert on the expectedMessage; the messages for exceptions thrown from the
418+
// decoding stream may differ from the messages for the decode methods.
419+
} catch (IOException e) {
420+
fail("Expected DecodingException but got: " + e);
421+
}
407422
}
408-
}
423+
},
424+
CAN_DECODE {
425+
@Override
426+
void assertFailsToDecode(
427+
BaseEncoding encoding, String cannotDecode, @Nullable String expectedMessage) {
428+
assertFalse(encoding.canDecode(cannotDecode));
429+
}
430+
},
431+
DECODE {
432+
@Override
433+
void assertFailsToDecode(
434+
BaseEncoding encoding, String cannotDecode, @Nullable String expectedMessage) {
435+
try {
436+
encoding.decode(cannotDecode);
437+
fail("Expected IllegalArgumentException");
438+
} catch (IllegalArgumentException expected) {
439+
if (expectedMessage != null) {
440+
assertThat(expected).hasCauseThat().hasMessageThat().isEqualTo(expectedMessage);
441+
}
442+
}
443+
}
444+
},
445+
DECODE_CHECKED {
446+
@Override
447+
void assertFailsToDecode(
448+
BaseEncoding encoding, String cannotDecode, @Nullable String expectedMessage) {
449+
try {
450+
encoding.decodeChecked(cannotDecode);
451+
fail("Expected DecodingException");
452+
} catch (DecodingException expected) {
453+
if (expectedMessage != null) {
454+
assertThat(expected).hasMessageThat().isEqualTo(expectedMessage);
455+
}
456+
}
457+
}
458+
};
459+
460+
abstract void assertFailsToDecode(
461+
BaseEncoding encoding, String cannotDecode, @Nullable String expectedMessage);
409462
}
410463

411464
@GwtIncompatible // Reader/Writer

‎guava/src/com/google/common/io/BaseEncoding.java‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -771,6 +771,27 @@ public int read() throws IOException {
771771
}
772772
}
773773

774+
@Override
775+
public int read(byte[] buf, int off, int len) throws IOException {
776+
// Overriding this to work around the fact that InputStream's default implementation of
777+
// this method will silently swallow exceptions thrown by the single-byte read() method
778+
// (other than on the first call to it), which in this case can cause invalid encoded
779+
// strings to not throw an exception.
780+
// See https://github.com/google/guava/issues/3542
781+
checkPositionIndexes(off, off + len, buf.length);
782+
783+
int i = off;
784+
for (; i < off + len; i++) {
785+
int b = read();
786+
if (b == -1) {
787+
int read = i - off;
788+
return read == 0 ? -1 : read;
789+
}
790+
buf[i] = (byte) b;
791+
}
792+
return i - off;
793+
}
794+
774795
@Override
775796
public void close() throws IOException {
776797
reader.close();

0 commit comments

Comments
 (0)