[FLINK-6162]Fix bug in ByteArrayOutputStreamWithPos#setPosition #3595

wenlong88 · 2017-03-22T13:18:18Z

Currently the precondition check in setPosition will fail when the buffer is full:
{{Preconditions.checkArgument(position < getEndPosition(), "Position out of bounds.");}}
We should allow the expected position to be the end position

I have added a test on the fix.

greghogan

The test is not using the array. The only method depending on a positively bounded count is toString.

greghogan · 2017-03-22T14:13:20Z

flink-core/src/test/java/org/apache/flink/core/memory/ByteArrayOutputStreamWithPosTest.java

+public class ByteArrayOutputStreamWithPosTest {
+	@Test
+	public void setPositionWhenBufferIsFull() throws Exception {
+		ByteArrayOutputStreamWithPos stream = new ByteArrayOutputStreamWithPos();


Initialize with a constant size matching the next line.

wenlong88 · 2017-03-23T01:51:40Z

@greghogan thanks for the review, I have changed the change test to use explicit buffer size.
you are right that currently there is no dependency on this method. We used the ByteArrayOutputStreamWithPOs in our own scenario and caught the bug.

greghogan

We should add some tests since currently we are only checking that no exception is thrown.

greghogan · 2017-03-23T15:18:24Z

flink-core/src/test/java/org/apache/flink/core/memory/ByteArrayOutputStreamWithPosTest.java

+public class ByteArrayOutputStreamWithPosTest {
+	@Test
+	public void setPositionWhenBufferIsFull() throws Exception {
+		ByteArrayOutputStreamWithPos stream = new ByteArrayOutputStreamWithPos(32);


I think it is stronger to replace 32 on these lines with a descriptive local variable. After initialization could add an assertion that getBuf().length equals that value.

greghogan · 2017-03-23T15:20:54Z

flink-core/src/test/java/org/apache/flink/core/memory/ByteArrayOutputStreamWithPosTest.java

+	public void setPositionWhenBufferIsFull() throws Exception {
+		ByteArrayOutputStreamWithPos stream = new ByteArrayOutputStreamWithPos(32);
+		stream.write(new byte[32]);
+		stream.setPosition(stream.getPosition());


Assert that getPosition is equal to the expected value. Can validate that setPosition now works per the change above and does not change the position, and also check that toString can be executed.

wenlong88 · 2017-03-24T04:27:19Z

@greghogan comments addressed

StefanRRichter · 2017-03-24T11:29:42Z

I would suggest to introduce the analogue check also in ByteArrayInputStreamWithPos.

wenlong88 · 2017-03-28T06:27:33Z

hi, @StefanRRichter , I have add check in setPosition in ByteArrayInputSteamWithPos, there is no dependency on this method too, so I rename the method from setPos to setPosition for unifying.

greghogan

Let's also call ByteArrayOutputStreamWithPos#toString() at the end of the test (I don't think the output matters, but the call will test that the bounds are consistent).

greghogan · 2017-03-28T14:31:30Z

flink-core/src/main/java/org/apache/flink/core/memory/ByteArrayInputStreamWithPos.java

@@ -118,7 +118,8 @@ public int getPosition() {
 		return position;
 	}

-	public void setPos(int pos) {
+	public void setPosition(int pos) {
+		Preconditions.checkArgument(pos < count, "Position out of bounds.");


Allow for equality as with ByteArrayOutputStreamWithPos? And check for non-negative. Should we create a matching getEndPosition() as in ByteArrayOutputStreamWithPos?

greghogan · 2017-03-28T14:31:44Z

flink-core/src/main/java/org/apache/flink/core/memory/ByteArrayOutputStreamWithPos.java

@@ -110,7 +110,7 @@ public int getPosition() {
 	}

 	public void setPosition(int position) {
-		Preconditions.checkArgument(position < getEndPosition(), "Position out of bounds.");
+		Preconditions.checkArgument(position <= getEndPosition(), "Position out of bounds.");


Check for non-negative.

greghogan · 2017-03-28T14:36:00Z

flink-core/src/test/java/org/apache/flink/core/memory/ByteArrayInputStreamWithPosTest.java

+	/**
+	 * This tests that the expected position exceeds the capacity of the byte array.
+	 */
+	@Test(expected = IllegalArgumentException.class)


I think the preference is to instead use ExpectedException which can also take an expected message.

wenlong88 · 2017-03-30T05:24:34Z

@greghogan Thanks for the review. I have add non-negative check and update the test with ExpectedException. But I don't think we need to call toString in the test since we have check the position instead I add a test to verify that toString returns the substring with range(0, position) to meet your concern. One update about setPosition in outputStream, I think it is better to auto extend the buffer when setting a position larger than the buffer size, just like the auto extending in write.

greghogan · 2017-03-30T10:51:42Z

When expanding with write the new capacity (up to position of course) is filled with data. Expanding with setPosition can leave holes in the data and may mask a bug. Since these classes are undocumented it would be safer to leave this behavior unmodified.

wenlong88 · 2017-03-31T02:40:26Z

@greghogan Currently we check the position with the EndPosition which is the size of buffer instead of a max written size , so it is still possible to leave holes when keeping unmodified. Enabling auto extending can give the user the flexibility to reserved some space for later filling and does not change current functions logics.

greghogan · 2017-03-31T15:41:22Z

@wenlong88 you are right that the position can currently be moved beyond written data. This still feels a bit like feature creep as we've added to the PR. Do you need to expand the array is this manner? How did you run across this bug? I'm not disagreeing with the change but would like to be conservative and understand the why.

wenlong88 · 2017-04-01T02:52:32Z

@greghogan I come across this bug when we are trying to write some objects to a byte array: at first, I wrote a 0 as initial size and then the serialized object, after that, I seek back to the start position to correct the size and then seek to the end of the written data to continue to write other object. When the array can auto expanding, I can just reserve some hole at first and then seek back and fill the hole.

wenlong88 · 2017-04-07T05:55:13Z

hi, @greghogan , do you have any new comment about the changes?

Fix off-by-one error in 'setPosition' and expand array when seeking outside the original bounds. This closes apache#3595

greghogan · 2017-04-07T20:14:16Z

@wenlong88 thanks for the contribution (and the ping)! Merging ...

Fix off-by-one error in 'setPosition' and expand array when seeking outside the original bounds. This closes apache#3595

wenlong.lwl added 2 commits March 22, 2017 14:29

Fix bug in ByteArrayOutputStreamWithPos

905e513

fix typo

09cc96a

greghogan reviewed Mar 22, 2017

View reviewed changes

use explicit initial buffer size

69fb3c3

greghogan reviewed Mar 23, 2017

View reviewed changes

address comments

d7cf66e

fix typo

cad4845

add check and test in ByteArrayInputStream

0e4db9e

greghogan reviewed Mar 29, 2017

View reviewed changes

address comments

0f62595

wenlong.lwl added 2 commits March 30, 2017 13:27

update test in ByteArrayInputStreamWithPosTest

312b578

update test in ByteArrayOutputStreamWithPosTest

cf9f0b4

greghogan pushed a commit to greghogan/flink that referenced this pull request Apr 7, 2017

[FLINK-6162] [core] Fix bug in ByteArrayOutputStreamWithPos

0168364

Fix off-by-one error in 'setPosition' and expand array when seeking outside the original bounds. This closes apache#3595

asfgit closed this in af34e5b Apr 8, 2017

p16i pushed a commit to p16i/flink that referenced this pull request Apr 16, 2017

[FLINK-6162] [core] Fix bug in ByteArrayOutputStreamWithPos

4eb7f7f

Fix off-by-one error in 'setPosition' and expand array when seeking outside the original bounds. This closes apache#3595

rmetzger added the component=<none> label Mar 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-6162]Fix bug in ByteArrayOutputStreamWithPos#setPosition #3595

[FLINK-6162]Fix bug in ByteArrayOutputStreamWithPos#setPosition #3595

wenlong88 commented Mar 22, 2017 •

edited

greghogan left a comment

greghogan Mar 22, 2017

wenlong88 commented Mar 23, 2017

greghogan left a comment

greghogan Mar 23, 2017

greghogan Mar 23, 2017

wenlong88 commented Mar 24, 2017

StefanRRichter commented Mar 24, 2017

wenlong88 commented Mar 28, 2017

greghogan left a comment

greghogan Mar 28, 2017

greghogan Mar 28, 2017

greghogan Mar 28, 2017

wenlong88 commented Mar 30, 2017

greghogan commented Mar 30, 2017

wenlong88 commented Mar 31, 2017

greghogan commented Mar 31, 2017

wenlong88 commented Apr 1, 2017

wenlong88 commented Apr 7, 2017

greghogan commented Apr 7, 2017

[FLINK-6162]Fix bug in ByteArrayOutputStreamWithPos#setPosition #3595

[FLINK-6162]Fix bug in ByteArrayOutputStreamWithPos#setPosition #3595

Conversation

wenlong88 commented Mar 22, 2017 • edited

greghogan left a comment

Choose a reason for hiding this comment

greghogan Mar 22, 2017

Choose a reason for hiding this comment

wenlong88 commented Mar 23, 2017

greghogan left a comment

Choose a reason for hiding this comment

greghogan Mar 23, 2017

Choose a reason for hiding this comment

greghogan Mar 23, 2017

Choose a reason for hiding this comment

wenlong88 commented Mar 24, 2017

StefanRRichter commented Mar 24, 2017

wenlong88 commented Mar 28, 2017

greghogan left a comment

Choose a reason for hiding this comment

greghogan Mar 28, 2017

Choose a reason for hiding this comment

greghogan Mar 28, 2017

Choose a reason for hiding this comment

greghogan Mar 28, 2017

Choose a reason for hiding this comment

wenlong88 commented Mar 30, 2017

greghogan commented Mar 30, 2017

wenlong88 commented Mar 31, 2017

greghogan commented Mar 31, 2017

wenlong88 commented Apr 1, 2017

wenlong88 commented Apr 7, 2017

greghogan commented Apr 7, 2017

wenlong88 commented Mar 22, 2017 •

edited