Skip to content

Commit 7815650

Browse files
garydgregorygarydgregory
committed
[IO-871] IOUtils.contentEquals is incorrect when InputStream.available
under-reports
1 parent 3efdfd7 commit 7815650

File tree

4 files changed

+186
-22
lines changed

4 files changed

+186
-22
lines changed

‎src/changes/changes.xml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@ The <action> type attribute can be add,update,fix,remove.
6666
<action dev="ggregory" type="fix" due-to="Gary Gregory">Deprecate FileChannels.contentEquals(FileChannel, FileChannel, int) in favor of FileChannels.contentEquals(SeekableByteChannel, SeekableByteChannel, int).</action>
6767
<action dev="ggregory" type="fix" due-to="Gary Gregory">Improve performance of IOUtils.contentEquals(InputStream, InputStream) by about 13%.</action>
6868
<action dev="ggregory" type="fix" issue="IO-870" due-to="Gary Gregory">PathUtils.copyFileToDirectory() across file systems #728.</action>
69+
<action dev="ggregory" type="fix" issue="IO-871" due-to="Éamonn McManus, Gary Gregory">IOUtils.contentEquals is incorrect when InputStream.available under-reports.</action>
6970
<!-- ADD -->
7071
<action dev="ggregory" type="add" issue="IO-860" due-to="Nico Strecker, Gary Gregory">Add ThrottledInputStream.Builder.setMaxBytes(long, ChronoUnit).</action>
7172
<action dev="ggregory" type="add" due-to="Gary Gregory">Add IOIterable.</action>

‎src/main/java/org/apache/commons/io/channels/FileChannels.java‎

Lines changed: 56 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -60,43 +60,78 @@ public static boolean contentEquals(final FileChannel channel1, final FileChanne
6060
*/
6161
public static boolean contentEquals(final ReadableByteChannel channel1, final ReadableByteChannel channel2, final int bufferCapacity) throws IOException {
6262
// Before making any changes, please test with org.apache.commons.io.jmh.IOUtilsContentEqualsInputStreamsBenchmark
63+
if (bufferCapacity <= 0) {
64+
throw new IllegalArgumentException();
65+
}
6366
// Short-circuit test
6467
if (Objects.equals(channel1, channel2)) {
6568
return true;
6669
}
6770
// Dig in and do the work
68-
final ByteBuffer byteBuffer1 = ByteBuffer.allocateDirect(bufferCapacity);
69-
final ByteBuffer byteBuffer2 = ByteBuffer.allocateDirect(bufferCapacity);
70-
int numRead1 = 0;
71-
int numRead2 = 0;
72-
boolean read0On1 = false;
73-
boolean read0On2 = false;
71+
final ByteBuffer c1Buffer = ByteBuffer.allocateDirect(bufferCapacity);
72+
final ByteBuffer c2Buffer = ByteBuffer.allocateDirect(bufferCapacity);
73+
int c1ReadNum = -1;
74+
int c2ReadNum = -1;
75+
boolean c1Read = true;
76+
boolean c2Read = true;
77+
boolean equals = false;
7478
// If a channel is a non-blocking channel, it may return 0 bytes read for any given call.
7579
while (true) {
76-
if (!read0On2) {
77-
numRead1 = channel1.read(byteBuffer1);
78-
byteBuffer1.clear();
79-
read0On1 = numRead1 == 0;
80+
// don't call compact() in this method to avoid copying
81+
if (c1Read) {
82+
c1ReadNum = channel1.read(c1Buffer);
83+
c1Buffer.position(0);
84+
c1Read = c1ReadNum >= 0;
8085
}
81-
if (!read0On1) {
82-
numRead2 = channel2.read(byteBuffer2);
83-
byteBuffer2.clear();
84-
read0On2 = numRead2 == 0;
86+
if (c2Read) {
87+
c2ReadNum = channel2.read(c2Buffer);
88+
c2Buffer.position(0);
89+
c2Read = c2ReadNum >= 0;
8590
}
86-
if (numRead1 == IOUtils.EOF && numRead2 == IOUtils.EOF) {
87-
return byteBuffer1.equals(byteBuffer2);
91+
if (c1ReadNum == IOUtils.EOF && c2ReadNum == IOUtils.EOF) {
92+
return equals || c1Buffer.equals(c2Buffer);
8893
}
89-
if (numRead1 == 0 || numRead2 == 0) {
90-
// 0 may be returned from a non-blocking channel.
94+
if (c1ReadNum == 0 || c2ReadNum == 0) {
9195
Thread.yield();
96+
}
97+
if (c1ReadNum == 0 && c2ReadNum == IOUtils.EOF || c2ReadNum == 0 && c1ReadNum == IOUtils.EOF) {
9298
continue;
9399
}
94-
if (numRead1 != numRead2) {
95-
return false;
100+
if (c1ReadNum != c2ReadNum) {
101+
final int limit = Math.min(c1ReadNum, c2ReadNum);
102+
if (limit == IOUtils.EOF) {
103+
return false;
104+
}
105+
c1Buffer.limit(limit);
106+
c2Buffer.limit(limit);
107+
if (!c1Buffer.equals(c2Buffer)) {
108+
return false;
109+
}
110+
equals = true;
111+
c1Buffer.limit(bufferCapacity);
112+
c2Buffer.limit(bufferCapacity);
113+
c1Read = c2ReadNum > c1ReadNum;
114+
c2Read = c1ReadNum > c2ReadNum;
115+
if (c1Read) {
116+
c1Buffer.position(0);
117+
} else {
118+
c1Buffer.position(limit);
119+
c2Buffer.limit(c1Buffer.remaining());
120+
c1ReadNum -= c2ReadNum;
121+
}
122+
if (c2Read) {
123+
c2Buffer.position(0);
124+
} else {
125+
c2Buffer.position(limit);
126+
c1Buffer.limit(c2Buffer.remaining());
127+
c2ReadNum -= c1ReadNum;
128+
}
129+
continue;
96130
}
97-
if (!byteBuffer1.equals(byteBuffer2)) {
131+
if (!c1Buffer.equals(c2Buffer)) {
98132
return false;
99133
}
134+
equals = c1Read = c2Read = true;
100135
}
101136
}
102137

‎src/test/java/org/apache/commons/io/IOUtilsTest.java‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@
4545
import java.io.InputStreamReader;
4646
import java.io.OutputStream;
4747
import java.io.Reader;
48+
import java.io.SequenceInputStream;
4849
import java.io.StringReader;
4950
import java.io.Writer;
5051
import java.net.ServerSocket;
@@ -689,6 +690,64 @@ public void testContentEqualsIgnoreEOL() throws Exception {
689690
testSingleEOL("1235", "1234", false);
690691
}
691692

693+
@Test
694+
public void testContentEqualsSequenceInputStream() throws Exception {
695+
// not equals
696+
// @formatter:off
697+
assertFalse(IOUtils.contentEquals(
698+
new ByteArrayInputStream("ab".getBytes()),
699+
new SequenceInputStream(
700+
new ByteArrayInputStream("a".getBytes()),
701+
new ByteArrayInputStream("b-".getBytes()))));
702+
assertFalse(IOUtils.contentEquals(
703+
new ByteArrayInputStream("ab".getBytes()),
704+
new SequenceInputStream(
705+
new ByteArrayInputStream("a-".getBytes()),
706+
new ByteArrayInputStream("b".getBytes()))));
707+
assertFalse(IOUtils.contentEquals(
708+
new ByteArrayInputStream("ab-".getBytes()),
709+
new SequenceInputStream(
710+
new ByteArrayInputStream("a".getBytes()),
711+
new ByteArrayInputStream("b".getBytes()))));
712+
assertFalse(IOUtils.contentEquals(
713+
new ByteArrayInputStream("".getBytes()),
714+
new SequenceInputStream(
715+
new ByteArrayInputStream("a".getBytes()),
716+
new ByteArrayInputStream("b".getBytes()))));
717+
assertFalse(IOUtils.contentEquals(
718+
new ByteArrayInputStream("".getBytes()),
719+
new SequenceInputStream(
720+
new ByteArrayInputStream("".getBytes()),
721+
new ByteArrayInputStream("b".getBytes()))));
722+
assertFalse(IOUtils.contentEquals(
723+
new ByteArrayInputStream("ab".getBytes()),
724+
new SequenceInputStream(
725+
new ByteArrayInputStream("".getBytes()),
726+
new ByteArrayInputStream("".getBytes()))));
727+
// equals
728+
assertTrue(IOUtils.contentEquals(
729+
new ByteArrayInputStream("".getBytes()),
730+
new SequenceInputStream(
731+
new ByteArrayInputStream("".getBytes()),
732+
new ByteArrayInputStream("".getBytes()))));
733+
assertTrue(IOUtils.contentEquals(
734+
new ByteArrayInputStream("ab".getBytes()),
735+
new SequenceInputStream(
736+
new ByteArrayInputStream("a".getBytes()),
737+
new ByteArrayInputStream("b".getBytes()))));
738+
assertTrue(IOUtils.contentEquals(
739+
new ByteArrayInputStream("ab".getBytes()),
740+
new SequenceInputStream(
741+
new ByteArrayInputStream("ab".getBytes()),
742+
new ByteArrayInputStream("".getBytes()))));
743+
assertTrue(IOUtils.contentEquals(
744+
new ByteArrayInputStream("ab".getBytes()),
745+
new SequenceInputStream(
746+
new ByteArrayInputStream("".getBytes()),
747+
new ByteArrayInputStream("ab".getBytes()))));
748+
// @formatter:on
749+
}
750+
692751
@Test
693752
public void testCopy_ByteArray_OutputStream() throws Exception {
694753
final File destination = TestUtils.newFile(temporaryFolder, "copy8.txt");

‎src/test/java/org/apache/commons/io/channels/FileChannelsTest.java‎

Lines changed: 70 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,13 @@
2222
import static org.junit.jupiter.api.Assertions.assertNotEquals;
2323
import static org.junit.jupiter.api.Assertions.assertTrue;
2424

25+
import java.io.ByteArrayInputStream;
2526
import java.io.File;
2627
import java.io.FileInputStream;
2728
import java.io.IOException;
29+
import java.io.InputStream;
30+
import java.io.SequenceInputStream;
31+
import java.nio.channels.Channels;
2832
import java.nio.channels.FileChannel;
2933
import java.nio.channels.SeekableByteChannel;
3034
import java.nio.file.Files;
@@ -35,6 +39,8 @@
3539
import org.apache.commons.io.file.AbstractTempDirTest;
3640
import org.apache.commons.lang3.ArrayUtils;
3741
import org.apache.commons.lang3.StringUtils;
42+
import org.junit.jupiter.params.ParameterizedTest;
43+
import org.junit.jupiter.params.provider.ValueSource;
3844
import org.junitpioneer.jupiter.cartesian.CartesianTest;
3945
import org.junitpioneer.jupiter.cartesian.CartesianTest.Values;
4046

@@ -47,7 +53,7 @@ enum FileChannelType {
4753
STOCK, PROXY, NON_BLOCKING, FIXED_READ_SIZE
4854
}
4955

50-
private static final int LARGE_FILE_SIZE = Integer.getInteger(FileChannelsTest.class.getSimpleName(), 100000);
56+
private static final int LARGE_FILE_SIZE = Integer.getInteger(FileChannelsTest.class.getSimpleName(), 100_000);
5157

5258
private static final int SMALL_BUFFER_SIZE = 1024;
5359
private static final String CONTENT = StringUtils.repeat("x", SMALL_BUFFER_SIZE);
@@ -93,6 +99,10 @@ private static FileChannel wrap(final FileChannel fc, final FileChannelType file
9399
}
94100
}
95101

102+
private boolean contentEquals(final InputStream in1, final InputStream in2, final int bufferCapacity) throws IOException {
103+
return FileChannels.contentEquals(Channels.newChannel(in1), Channels.newChannel(in2), bufferCapacity);
104+
}
105+
96106
private void testContentEquals(final String content1, final String content2, final int bufferSize, final FileChannelType fileChannelType)
97107
throws IOException {
98108
assertTrue(FileChannels.contentEquals(null, null, bufferSize));
@@ -287,4 +297,63 @@ public void testContentEqualsSeekableByteChannel(
287297
Files.deleteIfExists(bigFile3);
288298
}
289299
}
300+
301+
@ParameterizedTest
302+
@ValueSource(ints = { 1, 2, 4, 8, 16, 1024, 4096, 8192 })
303+
public void testContentEqualsSequenceInputStream(final int bufferCapacity) throws Exception {
304+
// not equals
305+
// @formatter:off
306+
assertFalse(contentEquals(
307+
new ByteArrayInputStream("ab".getBytes()),
308+
new SequenceInputStream(
309+
new ByteArrayInputStream("a".getBytes()),
310+
new ByteArrayInputStream("b-".getBytes())), bufferCapacity));
311+
assertFalse(contentEquals(
312+
new ByteArrayInputStream("ab".getBytes()),
313+
new SequenceInputStream(
314+
new ByteArrayInputStream("a-".getBytes()),
315+
new ByteArrayInputStream("b".getBytes())), bufferCapacity));
316+
assertFalse(contentEquals(
317+
new ByteArrayInputStream("ab-".getBytes()),
318+
new SequenceInputStream(
319+
new ByteArrayInputStream("a".getBytes()),
320+
new ByteArrayInputStream("b".getBytes())), bufferCapacity));
321+
assertFalse(contentEquals(
322+
new ByteArrayInputStream("".getBytes()),
323+
new SequenceInputStream(
324+
new ByteArrayInputStream("a".getBytes()),
325+
new ByteArrayInputStream("b".getBytes())), bufferCapacity));
326+
assertFalse(contentEquals(
327+
new ByteArrayInputStream("".getBytes()),
328+
new SequenceInputStream(
329+
new ByteArrayInputStream("".getBytes()),
330+
new ByteArrayInputStream("b".getBytes())), bufferCapacity));
331+
assertFalse(contentEquals(
332+
new ByteArrayInputStream("ab".getBytes()),
333+
new SequenceInputStream(
334+
new ByteArrayInputStream("".getBytes()),
335+
new ByteArrayInputStream("".getBytes())), bufferCapacity));
336+
// equals
337+
assertTrue(contentEquals(
338+
new ByteArrayInputStream("".getBytes()),
339+
new SequenceInputStream(
340+
new ByteArrayInputStream("".getBytes()),
341+
new ByteArrayInputStream("".getBytes())), bufferCapacity));
342+
assertTrue(contentEquals(
343+
new ByteArrayInputStream("ab".getBytes()),
344+
new SequenceInputStream(
345+
new ByteArrayInputStream("a".getBytes()),
346+
new ByteArrayInputStream("b".getBytes())), bufferCapacity));
347+
assertTrue(contentEquals(
348+
new ByteArrayInputStream("ab".getBytes()),
349+
new SequenceInputStream(
350+
new ByteArrayInputStream("ab".getBytes()),
351+
new ByteArrayInputStream("".getBytes())), bufferCapacity));
352+
assertTrue(contentEquals(
353+
new ByteArrayInputStream("ab".getBytes()),
354+
new SequenceInputStream(
355+
new ByteArrayInputStream("".getBytes()),
356+
new ByteArrayInputStream("ab".getBytes())), bufferCapacity));
357+
// @formatter:on
358+
}
290359
}

0 commit comments

Comments
 (0)