You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2022/04/11 07:15:57 UTC
[GitHub] [maven-surefire] zoltanmeze opened a new pull request, #518: [SUREFIRE-2058] - Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
zoltanmeze opened a new pull request, #518:
URL: https://github.com/apache/maven-surefire/pull/518
Two cases are covered here:
* Test run swallowing some of the outputs and finishing with the following warning:
```
[WARNING] Corrupted channel by directly writing to native stream in forked JVM 1.
```
* Test run getting completely stuck
Both cases are caused by the same issue, output buffer overflow in [AbstractStreamDecoder#readString(Memento, int)](https://github.com/apache/maven-surefire/blob/master/surefire-api/src/main/java/org/apache/maven/surefire/api/stream/AbstractStreamDecoder.java#L302), because output char buffer is not cleared if it's not completely full.
Added unit tests:
* AbstractStreamDecoderTest#shouldReadStringOverflowOnNewLine:
Input buffer of 1026 bytes: 1023 x 1-byte + 1 x 2-bytes encoded character + new line (LF).
* In first iteration it decodes the first 1023 bytes from the input buffer without clearing the output buffer.
* In second iteration `decodeString` returns an overflow, but it still force reads the 2-byte encoded character from input to output. `readString` ends up with 1 remaining byte (LF) on input buffer causing channel to become corrupted.
* AbstractStreamDecoderTest#shouldReadStringOverflowOn4BytesEncodedSymbol:
Input buffer of 1027 bytes: 1023 x 1 byte + 1 x 4-bytes encoded characters.
* In first iteration it decodes the first 1023 bytes from the input buffer without clearing the output buffer.
* In second iteration `decodeString` returns an overflow, without reading anything from input buffer. This is causing an infinite loop in `readString`, it never exits.
Following this checklist to help us incorporate your
contribution quickly and easily:
- [x] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/SUREFIRE) filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
- [x] Each commit in the pull request should have a meaningful subject line and body.
- [x] Format the pull request title like `[SUREFIRE-XXX] - Fixes bug in ApproximateQuantiles`,
where you replace `SUREFIRE-XXX` with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Run `mvn clean install` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [x] You have run the integration tests successfully (`mvn -Prun-its clean install`).
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [x] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #518: [SUREFIRE-2058] - Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on PR #518:
URL: https://github.com/apache/maven-surefire/pull/518#issuecomment-1096733212
@zoltanmeze
I have used only your two unit tests on master without the fix. I could reproduce the issue. Thx for the fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #518: [SUREFIRE-2058] - Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on PR #518:
URL: https://github.com/apache/maven-surefire/pull/518#issuecomment-1108371215
@zoltanmeze
Feel free to extract the last commit to a separate PR and Jira.
The Jira [SUREFIRE-2056](https://issues.apache.org/jira/browse/SUREFIRE-2056) is about another data element - testRunId.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #518: [SUREFIRE-2058] - Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on PR #518:
URL: https://github.com/apache/maven-surefire/pull/518#issuecomment-1099706229
@zoltanmeze
@walterFor
yes, that's true, the code
```
if ( runMode != null )
{
// one byte of length + one delimiter character ':' + <string> + one delimiter character ':'
lengthOfMetadata += 1 + 1 + runMode.geRunmode().length() + 1;
}
```
should become
```
// one byte of length + one delimiter character ':' + <string> + one delimiter character ':'
lengthOfMetadata += 1 + 1 + ( runMode == null ? 0 : runMode.getRunmodeBinary().length ) + 1;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] walterFor commented on pull request #518: [SUREFIRE-2058] - Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
Posted by GitBox <gi...@apache.org>.
walterFor commented on PR #518:
URL: https://github.com/apache/maven-surefire/pull/518#issuecomment-1099231382
@Tibor17
Actually runMode in [5df49](https://github.com/apache/maven-surefire/commit/8e301949173f20971c288484e5711b5496e5df49) can be set to null in line 119 [TestOutputReportEntry.java](https://github.com/apache/maven-surefire/blob/79db90338fb474f91c76991388a35bc412ee1d46/surefire-api/src/main/java/org/apache/maven/surefire/api/report/TestOutputReportEntry.java) , which will cause another BufferOverflowException. Attach stacktrace for my case:
`java.nio.BufferOverflowException
at java.nio.Buffer.nextPutIndex(Buffer.java:547)
at java.nio.HeapByteBuffer.put(HeapByteBuffer.java:172)
at org.apache.maven.surefire.api.stream.AbstractStreamEncoder.encodeString(AbstractStreamEncoder.java:127)
at org.apache.maven.surefire.api.stream.AbstractStreamEncoder.encodeStringData(AbstractStreamEncoder.java:171)
at org.apache.maven.surefire.api.stream.AbstractStreamEncoder.encode(AbstractStreamEncoder.java:157)
at org.apache.maven.surefire.booter.spi.EventChannelEncoder.encodeMessage(EventChannelEncoder.java:398)
at org.apache.maven.surefire.booter.spi.EventChannelEncoder.setOutErr(EventChannelEncoder.java:188)
at org.apache.maven.surefire.booter.spi.EventChannelEncoder.testOutput(EventChannelEncoder.java:183)
at org.apache.maven.surefire.api.booter.ForkingRunListener.writeTestOutput(ForkingRunListener.java:113)
at org.apache.maven.surefire.api.booter.ForkingRunListener.writeTestOutput(ForkingRunListener.java:44)
at org.apache.maven.surefire.common.junit4.JUnit4RunListener.writeTestOutput(JUnit4RunListener.java:235)
at org.apache.maven.surefire.api.report.ConsoleOutputCapture$ForwardingPrintStream.println(ConsoleOutputCapture.java:144)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a diff in pull request #518: [SUREFIRE-2058] - Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on code in PR #518:
URL: https://github.com/apache/maven-surefire/pull/518#discussion_r848466591
##########
surefire-api/src/test/java/org/apache/maven/surefire/api/stream/AbstractStreamDecoderTest.java:
##########
@@ -258,6 +258,56 @@ public void shouldReadString() throws Exception
.isEqualTo( "0123456789" );
}
+ @Test
+ public void shouldReadStringOverflowOnNewLine() throws Exception
+ {
+ StringBuilder s = new StringBuilder( 1025 );
+ for ( int i = 0; i < 10; i++ )
+ {
+ s.append( PATTERN1 );
+ }
+ s.append( PATTERN1, 0, 23 );
+ s.append( "\u00FA\n" ); // 2-bytes encoded character + LF
+
+ Channel channel = new Channel( s.toString().getBytes( UTF_8 ), s.length() );
+
+ Mock thread = new Mock( channel, new MockForkNodeArguments(),
+ Collections.<Segment, ForkedProcessEventType>emptyMap() );
+
+ Memento memento = thread.new Memento();
+
+ assertThat( (String) invokeMethod( thread, "readString", memento, 1026 ) )
+ .isEqualTo( s.toString() );
+
+ assertThat ( memento.getByteBuffer().remaining() )
+ .isEqualTo( 0 );
+ }
+
+ @Test
+ public void shouldReadStringOverflowOn4BytesEncodedSymbol() throws Exception
+ {
+ StringBuilder s = new StringBuilder( 1024 );
Review Comment:
I don't know. The point of the unit tests is that you provide them and I would be able to reproduce a test failure with the original code. And I would apply your fix which makes the test green again. This was the process on my side.
It;s up to you. The tests are just a kind of language you as contributor want to convince ASF about the whole point of the fix. If it is necessary to write more tests, feel free to do it but then I will undergo the test verifying failure on the original master code.
btw, I found your interesting IT
https://github.com/zoltanmeze/surefire-corrupted-channel
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] zoltanmeze commented on a diff in pull request #518: [SUREFIRE-2058] - Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
Posted by GitBox <gi...@apache.org>.
zoltanmeze commented on code in PR #518:
URL: https://github.com/apache/maven-surefire/pull/518#discussion_r847728655
##########
surefire-api/src/test/java/org/apache/maven/surefire/api/stream/AbstractStreamDecoderTest.java:
##########
@@ -258,6 +258,56 @@ public void shouldReadString() throws Exception
.isEqualTo( "0123456789" );
}
+ @Test
+ public void shouldReadStringOverflowOnNewLine() throws Exception
+ {
+ StringBuilder s = new StringBuilder( 1025 );
+ for ( int i = 0; i < 10; i++ )
+ {
+ s.append( PATTERN1 );
+ }
+ s.append( PATTERN1, 0, 23 );
+ s.append( "\u00FA\n" ); // 2-bytes encoded character + LF
+
+ Channel channel = new Channel( s.toString().getBytes( UTF_8 ), s.length() );
+
+ Mock thread = new Mock( channel, new MockForkNodeArguments(),
+ Collections.<Segment, ForkedProcessEventType>emptyMap() );
+
+ Memento memento = thread.new Memento();
+
+ assertThat( (String) invokeMethod( thread, "readString", memento, 1026 ) )
+ .isEqualTo( s.toString() );
+
+ assertThat ( memento.getByteBuffer().remaining() )
+ .isEqualTo( 0 );
+ }
+
+ @Test
+ public void shouldReadStringOverflowOn4BytesEncodedSymbol() throws Exception
+ {
+ StringBuilder s = new StringBuilder( 1024 );
Review Comment:
Just realized this should be 1025 here, 😵 (`"\uD83D\uDE35"`) is actually counted as 2 characters.
Do you want me to change this or just leave StringBuilder to increase the capacity on it's own?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a diff in pull request #518: [SUREFIRE-2058] - Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on code in PR #518:
URL: https://github.com/apache/maven-surefire/pull/518#discussion_r848513301
##########
surefire-api/src/test/java/org/apache/maven/surefire/api/stream/AbstractStreamDecoderTest.java:
##########
@@ -258,6 +258,56 @@ public void shouldReadString() throws Exception
.isEqualTo( "0123456789" );
}
+ @Test
+ public void shouldReadStringOverflowOnNewLine() throws Exception
+ {
+ StringBuilder s = new StringBuilder( 1025 );
+ for ( int i = 0; i < 10; i++ )
+ {
+ s.append( PATTERN1 );
+ }
+ s.append( PATTERN1, 0, 23 );
+ s.append( "\u00FA\n" ); // 2-bytes encoded character + LF
+
+ Channel channel = new Channel( s.toString().getBytes( UTF_8 ), s.length() );
+
+ Mock thread = new Mock( channel, new MockForkNodeArguments(),
+ Collections.<Segment, ForkedProcessEventType>emptyMap() );
+
+ Memento memento = thread.new Memento();
+
+ assertThat( (String) invokeMethod( thread, "readString", memento, 1026 ) )
+ .isEqualTo( s.toString() );
+
+ assertThat ( memento.getByteBuffer().remaining() )
+ .isEqualTo( 0 );
+ }
+
+ @Test
+ public void shouldReadStringOverflowOn4BytesEncodedSymbol() throws Exception
+ {
+ StringBuilder s = new StringBuilder( 1024 );
Review Comment:
I will verify the code with/out the fix but I think the same as you. The unit tests are quite clear.
If there is only a problem in `AbstractStreamDecoder` and nowhere else, the IT is useless.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] zoltanmeze commented on pull request #518: [SUREFIRE-2058] Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
Posted by GitBox <gi...@apache.org>.
zoltanmeze commented on PR #518:
URL: https://github.com/apache/maven-surefire/pull/518#issuecomment-1108794842
@Tibor17 Done, created a separate jira ticket and PR for BufferOverflowException on null runMode, see https://github.com/apache/maven-surefire/pull/529
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] zoltanmeze commented on a diff in pull request #518: [SUREFIRE-2058] - Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
Posted by GitBox <gi...@apache.org>.
zoltanmeze commented on code in PR #518:
URL: https://github.com/apache/maven-surefire/pull/518#discussion_r849297428
##########
surefire-api/src/test/java/org/apache/maven/surefire/api/stream/AbstractStreamDecoderTest.java:
##########
@@ -258,6 +258,56 @@ public void shouldReadString() throws Exception
.isEqualTo( "0123456789" );
}
+ @Test
+ public void shouldReadStringOverflowOnNewLine() throws Exception
+ {
+ StringBuilder s = new StringBuilder( 1025 );
+ for ( int i = 0; i < 10; i++ )
+ {
+ s.append( PATTERN1 );
+ }
+ s.append( PATTERN1, 0, 23 );
+ s.append( "\u00FA\n" ); // 2-bytes encoded character + LF
+
+ Channel channel = new Channel( s.toString().getBytes( UTF_8 ), s.length() );
+
+ Mock thread = new Mock( channel, new MockForkNodeArguments(),
+ Collections.<Segment, ForkedProcessEventType>emptyMap() );
Review Comment:
Sure. Other test methods are also using explicit type arguments, so just used the same way.
For consistency replaced in all places in this class, see [0b1c564](https://github.com/apache/maven-surefire/pull/518/commits/0b1c564779d924901c9bd3f88482fe1f33964bdb).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] zoltanmeze commented on a diff in pull request #518: [SUREFIRE-2058] - Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
Posted by GitBox <gi...@apache.org>.
zoltanmeze commented on code in PR #518:
URL: https://github.com/apache/maven-surefire/pull/518#discussion_r848495107
##########
surefire-api/src/test/java/org/apache/maven/surefire/api/stream/AbstractStreamDecoderTest.java:
##########
@@ -258,6 +258,56 @@ public void shouldReadString() throws Exception
.isEqualTo( "0123456789" );
}
+ @Test
+ public void shouldReadStringOverflowOnNewLine() throws Exception
+ {
+ StringBuilder s = new StringBuilder( 1025 );
+ for ( int i = 0; i < 10; i++ )
+ {
+ s.append( PATTERN1 );
+ }
+ s.append( PATTERN1, 0, 23 );
+ s.append( "\u00FA\n" ); // 2-bytes encoded character + LF
+
+ Channel channel = new Channel( s.toString().getBytes( UTF_8 ), s.length() );
+
+ Mock thread = new Mock( channel, new MockForkNodeArguments(),
+ Collections.<Segment, ForkedProcessEventType>emptyMap() );
+
+ Memento memento = thread.new Memento();
+
+ assertThat( (String) invokeMethod( thread, "readString", memento, 1026 ) )
+ .isEqualTo( s.toString() );
+
+ assertThat ( memento.getByteBuffer().remaining() )
+ .isEqualTo( 0 );
+ }
+
+ @Test
+ public void shouldReadStringOverflowOn4BytesEncodedSymbol() throws Exception
+ {
+ StringBuilder s = new StringBuilder( 1024 );
Review Comment:
Already changed that StringBuilder capacity from 1024 to 1025 in the morning. It doesn't change anything, StringBuilder increases the capacity internally if initial capacity is low.
Cannot think of any other tests. That first one was happening in some of my tests. Once I figured out what's making those errors and just tried some other cases.
Originally I included both of these tests as ITs, but didn't push that commit because it's actually testing the same thing and unit tests are much faster: https://github.com/apache/maven-surefire/commit/7af78437c91bd2c7245e92170b3da9a47dcf32b4
:point_up: It's same as https://github.com/zoltanmeze/surefire-corrupted-channel but without random.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #518: [SUREFIRE-2058] - Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on PR #518:
URL: https://github.com/apache/maven-surefire/pull/518#issuecomment-1096740032
@zoltanmeze
I wrote this code but I would like to remove the [loop](https://github.com/apache/maven-surefire/pull/518/commits/8615d16ea12f472b7ed1b1f0857bfd8c961066e0#diff-3d0df21c8b05874314acbad0ec8fc6778db9eebe5a08bba3bcff4f52872a4ce6R327). I have already done it and I would like to make anew PR after yours. I will invite you there. Do you want to add something into this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] zoltanmeze commented on pull request #518: [SUREFIRE-2058] - Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
Posted by GitBox <gi...@apache.org>.
zoltanmeze commented on PR #518:
URL: https://github.com/apache/maven-surefire/pull/518#issuecomment-1108203939
> @zoltanmeze I have qualified the problem with `runMode` as a bug, see the [comment](https://github.com/apache/maven-surefire/pull/518#issuecomment-1099706229). The outcome would be the same buffer overflow and the same meaning for this Jira.
@Tibor17
Done. See [511b5c4](https://github.com/apache/maven-surefire/pull/518/commits/511b5c43b01597384937649ca32e26ba633e2859). Also rebased the branch onto latest master.
I still think that BufferOverflowException on null runMode should be a separate issue/PR, it's not really related to [SUREFIRE-2058](https://issues.apache.org/jira/browse/SUREFIRE-2058). It's a bug in StreamEncoder, but [2058](https://issues.apache.org/jira/browse/SUREFIRE-2058) is a StreamDecoder issue with no exception thrown. So it should be probably linked to [SUREFIRE-2056](https://issues.apache.org/jira/browse/SUREFIRE-2056) instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #518: [SUREFIRE-2058] - Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on PR #518:
URL: https://github.com/apache/maven-surefire/pull/518#issuecomment-1095498879
Hi @zoltanmeze , we fixed buffer overflow in https://github.com/apache/maven-surefire/commit/8e301949173f20971c288484e5711b5496e5df49
This is different buffer overflow?
The changes in the algorithm must be reviewed very carefully. Let me check it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a diff in pull request #518: [SUREFIRE-2058] - Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on code in PR #518:
URL: https://github.com/apache/maven-surefire/pull/518#discussion_r848466591
##########
surefire-api/src/test/java/org/apache/maven/surefire/api/stream/AbstractStreamDecoderTest.java:
##########
@@ -258,6 +258,56 @@ public void shouldReadString() throws Exception
.isEqualTo( "0123456789" );
}
+ @Test
+ public void shouldReadStringOverflowOnNewLine() throws Exception
+ {
+ StringBuilder s = new StringBuilder( 1025 );
+ for ( int i = 0; i < 10; i++ )
+ {
+ s.append( PATTERN1 );
+ }
+ s.append( PATTERN1, 0, 23 );
+ s.append( "\u00FA\n" ); // 2-bytes encoded character + LF
+
+ Channel channel = new Channel( s.toString().getBytes( UTF_8 ), s.length() );
+
+ Mock thread = new Mock( channel, new MockForkNodeArguments(),
+ Collections.<Segment, ForkedProcessEventType>emptyMap() );
+
+ Memento memento = thread.new Memento();
+
+ assertThat( (String) invokeMethod( thread, "readString", memento, 1026 ) )
+ .isEqualTo( s.toString() );
+
+ assertThat ( memento.getByteBuffer().remaining() )
+ .isEqualTo( 0 );
+ }
+
+ @Test
+ public void shouldReadStringOverflowOn4BytesEncodedSymbol() throws Exception
+ {
+ StringBuilder s = new StringBuilder( 1024 );
Review Comment:
I don't know. The point of the unit tests is that you provide them and I would be able to reproduce a test failure with the original code. And I would apply your fix which makes the test green again. This was the process on my side.
It;s up to you. The tests are just a kind of language you as contributor want to convince ASF about the whole point of the fix. If it is necessary to write more tests, feel free to do it but then I will undergo the test with verifying failure on the original master code.
I found your interesting IT
https://github.com/zoltanmeze/surefire-corrupted-channel
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #518: [SUREFIRE-2058] - Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on PR #518:
URL: https://github.com/apache/maven-surefire/pull/518#issuecomment-1096956544
`StringBuilder s = new StringBuilder( 1025 )` extra 1000 nano seconds are not visible by humans. `StringBuilder` reallocates the internal array. The other users may think that `1025` is some business value in this test but we know that it is not. We can call the default constructor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a diff in pull request #518: [SUREFIRE-2058] - Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on code in PR #518:
URL: https://github.com/apache/maven-surefire/pull/518#discussion_r848466591
##########
surefire-api/src/test/java/org/apache/maven/surefire/api/stream/AbstractStreamDecoderTest.java:
##########
@@ -258,6 +258,56 @@ public void shouldReadString() throws Exception
.isEqualTo( "0123456789" );
}
+ @Test
+ public void shouldReadStringOverflowOnNewLine() throws Exception
+ {
+ StringBuilder s = new StringBuilder( 1025 );
+ for ( int i = 0; i < 10; i++ )
+ {
+ s.append( PATTERN1 );
+ }
+ s.append( PATTERN1, 0, 23 );
+ s.append( "\u00FA\n" ); // 2-bytes encoded character + LF
+
+ Channel channel = new Channel( s.toString().getBytes( UTF_8 ), s.length() );
+
+ Mock thread = new Mock( channel, new MockForkNodeArguments(),
+ Collections.<Segment, ForkedProcessEventType>emptyMap() );
+
+ Memento memento = thread.new Memento();
+
+ assertThat( (String) invokeMethod( thread, "readString", memento, 1026 ) )
+ .isEqualTo( s.toString() );
+
+ assertThat ( memento.getByteBuffer().remaining() )
+ .isEqualTo( 0 );
+ }
+
+ @Test
+ public void shouldReadStringOverflowOn4BytesEncodedSymbol() throws Exception
+ {
+ StringBuilder s = new StringBuilder( 1024 );
Review Comment:
I don't know. The point of the unit tests is that you provide them and I would be able to reproduce a test failure with the original code. And I would apply your fix which makes the test green again. This was the process on my side.
It;s up to you. The tests are just a kind of language you as contributor want to convince ASF about the whole point of the fix. If it is necessary to write more tests, feel free to do it but then I will undergo the test verifying failure on the original master code.
I found your interesting IT
https://github.com/zoltanmeze/surefire-corrupted-channel
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] zoltanmeze commented on a diff in pull request #518: [SUREFIRE-2058] - Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
Posted by GitBox <gi...@apache.org>.
zoltanmeze commented on code in PR #518:
URL: https://github.com/apache/maven-surefire/pull/518#discussion_r849297428
##########
surefire-api/src/test/java/org/apache/maven/surefire/api/stream/AbstractStreamDecoderTest.java:
##########
@@ -258,6 +258,56 @@ public void shouldReadString() throws Exception
.isEqualTo( "0123456789" );
}
+ @Test
+ public void shouldReadStringOverflowOnNewLine() throws Exception
+ {
+ StringBuilder s = new StringBuilder( 1025 );
+ for ( int i = 0; i < 10; i++ )
+ {
+ s.append( PATTERN1 );
+ }
+ s.append( PATTERN1, 0, 23 );
+ s.append( "\u00FA\n" ); // 2-bytes encoded character + LF
+
+ Channel channel = new Channel( s.toString().getBytes( UTF_8 ), s.length() );
+
+ Mock thread = new Mock( channel, new MockForkNodeArguments(),
+ Collections.<Segment, ForkedProcessEventType>emptyMap() );
Review Comment:
Sure. Other test methods are also using explicit type arguments, so just used the same way.
For consistency replaced in all places in this class.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] zoltanmeze commented on pull request #518: [SUREFIRE-2058] - Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
Posted by GitBox <gi...@apache.org>.
zoltanmeze commented on PR #518:
URL: https://github.com/apache/maven-surefire/pull/518#issuecomment-1099564786
@walterFor Is that somehow related to this issue? It doesn't look related. Can you create a ticket with reproducible steps?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] zoltanmeze commented on pull request #518: [SUREFIRE-2058] - Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
Posted by GitBox <gi...@apache.org>.
zoltanmeze commented on PR #518:
URL: https://github.com/apache/maven-surefire/pull/518#issuecomment-1096752891
> @zoltanmeze I wrote this code but I would like to remove the [loop](https://github.com/apache/maven-surefire/pull/518/commits/8615d16ea12f472b7ed1b1f0857bfd8c961066e0#diff-3d0df21c8b05874314acbad0ec8fc6778db9eebe5a08bba3bcff4f52872a4ce6R327). I have already done it and I would like to make anew PR after yours. I will invite you there. Do you want to add something into this PR?
@Tibor17 No, everything good.
That do-while looked suspicious to me, I don't think that condition is ever true. But didn't want to touch it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a diff in pull request #518: [SUREFIRE-2058] - Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on code in PR #518:
URL: https://github.com/apache/maven-surefire/pull/518#discussion_r848757753
##########
surefire-api/src/test/java/org/apache/maven/surefire/api/stream/AbstractStreamDecoderTest.java:
##########
@@ -258,6 +258,56 @@ public void shouldReadString() throws Exception
.isEqualTo( "0123456789" );
}
+ @Test
+ public void shouldReadStringOverflowOnNewLine() throws Exception
+ {
+ StringBuilder s = new StringBuilder( 1025 );
+ for ( int i = 0; i < 10; i++ )
+ {
+ s.append( PATTERN1 );
+ }
+ s.append( PATTERN1, 0, 23 );
+ s.append( "\u00FA\n" ); // 2-bytes encoded character + LF
+
+ Channel channel = new Channel( s.toString().getBytes( UTF_8 ), s.length() );
+
+ Mock thread = new Mock( channel, new MockForkNodeArguments(),
+ Collections.<Segment, ForkedProcessEventType>emptyMap() );
Review Comment:
Here can be static import of `emptyMap()` without Generics.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] zoltanmeze commented on pull request #518: [SUREFIRE-2058] - Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
Posted by GitBox <gi...@apache.org>.
zoltanmeze commented on PR #518:
URL: https://github.com/apache/maven-surefire/pull/518#issuecomment-1095528603
I believe it it a different issue, not throwing any `BufferOverflowException`.
`CharsetDecoder#decode` in [decodeString call (here)](https://github.com/apache/maven-surefire/blob/master/surefire-api/src/main/java/org/apache/maven/surefire/api/stream/AbstractStreamDecoder.java#L322) results in overflow when there is not enough space left in the output char buffer. And output buffer is not cleared because the `!output.hasRemaining()` condition. Clearing after each chunk ensures underflow in `decodeString`, so it has always enough space to write decoded string into the the output buffer.
You can verify this with the two included unit tests. Test method naming is probably not the best, in the first it doesn't needs to be a new line character after a 2-bytes encoded character, it can be anything else.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #518: [SUREFIRE-2058] - Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on PR #518:
URL: https://github.com/apache/maven-surefire/pull/518#issuecomment-1107656505
Hi @zoltanmeze
Would you have time to join us again and continue?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #518: [SUREFIRE-2058] Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on PR #518:
URL: https://github.com/apache/maven-surefire/pull/518#issuecomment-1110246669
@zoltanmeze
Thx for contributing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 merged pull request #518: [SUREFIRE-2058] Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
Posted by GitBox <gi...@apache.org>.
Tibor17 merged PR #518:
URL: https://github.com/apache/maven-surefire/pull/518
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #518: [SUREFIRE-2058] Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on PR #518:
URL: https://github.com/apache/maven-surefire/pull/518#issuecomment-1109146740
Let's run the CI build...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #518: [SUREFIRE-2058] - Corrupted STDOUT by directly writing to native stream in forked JVM 1 with UTF-8 console logging
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on PR #518:
URL: https://github.com/apache/maven-surefire/pull/518#issuecomment-1097426958
@zoltanmeze
I would like to continue with some changes regarding the same class and subclasses in #519 , pls have a look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org