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/04 10:22:04 UTC
[GitHub] [maven-surefire] yrodiere opened a new pull request, #506: SUREFIRE-2056 BufferOverflowException when encoding message with null testId
yrodiere opened a new pull request, #506:
URL: https://github.com/apache/maven-surefire/pull/506
See https://issues.apache.org/jira/browse/SUREFIRE-2056 for an analysis of the problem.
This brings the buffer size back in line with what `encodeHeader` actually does, thereby avoiding a buffer overflow.
-----
- [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.
- [ ] 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 #506: SUREFIRE-2056 BufferOverflowException when encoding message with null testId
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on PR #506:
URL: https://github.com/apache/maven-surefire/pull/506#issuecomment-1090011910
no, leave it as it is, this makes sense. I will merge it.
Can you tell me if this is a blocker issue for the Quarkus?
--
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 #506: SUREFIRE-2056 BufferOverflowException when encoding message with null testId
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on PR #506:
URL: https://github.com/apache/maven-surefire/pull/506#issuecomment-1088109075
@yrodiere
It would not be a problem to modify the [existing test](https://github.com/apache/maven-surefire/blob/4e0a769227fa185ffbfdf6bf68b67294868b8851/surefire-booter/src/test/java/org/apache/maven/surefire/booter/spi/EventChannelEncoderTest.java#L1194).
We have too many tests in this class. Some are similar and eligible for ParameterizedTests. Adding more would complicate the refactoring. Thus it is better to modify the existing one for 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 #506: SUREFIRE-2056 BufferOverflowException when encoding message with null testId
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on PR #506:
URL: https://github.com/apache/maven-surefire/pull/506#issuecomment-1090001986
> I suspect the reason we don't get the bug with a non-empty message is that we over-allocate space in the buffer for the message: `length * <maximum possible size of UTF-8 characters>`
@yrodiere
yes, I know that the non-empty string may compensate this bug. If this bugs knocks too much, we would have to do somehing about a new release.
This method is acctually the minimum guaranteed buffer size. An exact value would have a performance impact and so it would invalidate whole idea.
--
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] yrodiere commented on pull request #506: SUREFIRE-2056 BufferOverflowException when encoding message with null testId
Posted by GitBox <gi...@apache.org>.
yrodiere commented on PR #506:
URL: https://github.com/apache/maven-surefire/pull/506#issuecomment-1088329572
@Tibor17 I would have, but the bug only occurs the message is an empty string, unlike the existing test, and I assumed this would be too big of a change? Not testing the nominal case with both a message and a test run ID seemed unwise, but what do I know :)
I suspect the reason we don't get the bug with a non-empty message is that we over-allocate space in the buffer for the message: length * <maximum possible size of UTF-8 characters>:
https://github.com/apache/maven-surefire/blob/79db90338fb474f91c76991388a35bc412ee1d46/surefire-api/src/main/java/org/apache/maven/surefire/api/stream/AbstractStreamEncoder.java#L206
In the case of UTF-8 the max bytes per char is `3.0f`, while most characters are just 1 byte, so this leaves a few extra bytes in the buffer that can be used for something else, and this over-allocation ends up "compensating" the bug.
I adjusted the test name and added a comment to make this clear.
Of course if you just wanted me to keep the testing code and shove it into the existing test method, I can do that.
--
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 #506: SUREFIRE-2056 BufferOverflowException when encoding message with null testId
Posted by GitBox <gi...@apache.org>.
Tibor17 merged PR #506:
URL: https://github.com/apache/maven-surefire/pull/506
--
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] yrodiere commented on pull request #506: SUREFIRE-2056 BufferOverflowException when encoding message with null testId
Posted by GitBox <gi...@apache.org>.
yrodiere commented on PR #506:
URL: https://github.com/apache/maven-surefire/pull/506#issuecomment-1090010194
Right, I wasn't proposing to change this over-allocation. Just explaining why a different test makes sense.
So... Do you want me to change the existing test to use an empty message (and not test non-empty messages anymore), or just shove the testing code I added into the existing test method, after the existing testing code, or leave my PR as is?
--
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] yrodiere commented on pull request #506: SUREFIRE-2056 BufferOverflowException when encoding message with null testId
Posted by GitBox <gi...@apache.org>.
yrodiere commented on PR #506:
URL: https://github.com/apache/maven-surefire/pull/506#issuecomment-1090029800
Thanks.
> Can you tell me if this is a blocker issue for the Quarkus?
I personally don't know. Hey @famod, is this bug blocking Quarkus in any way?
Personally I encountered this bug on another project, https://github.com/hibernate/hibernate-search. This bug is certainly preventing me from upgrading to M6, but M5 works fine for me (or rather, I managed to workaround a problem that get fixed in M6, [SUREFIRE-1815](https://issues.apache.org/jira/browse/SUREFIRE-1815)), so I can stay on M5 for now.
--
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] basil commented on pull request #506: [SUREFIRE-2056] BufferOverflowException when encoding message with null testId
Posted by GitBox <gi...@apache.org>.
basil commented on PR #506:
URL: https://github.com/apache/maven-surefire/pull/506#issuecomment-1107958413
I have hit this issue upgrading various Jenkins plugins to M6, especially when the tests write to standard out. I have been able to work around it by avoiding the use of standard out from tests, but it would be nice to have a fix released. I would hate to have to revert the whole Jenkins plugin ecosystem back to M5.
--
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] br0nstein commented on pull request #506: [SUREFIRE-2056] BufferOverflowException when encoding message with null testId
Posted by GitBox <gi...@apache.org>.
br0nstein commented on PR #506:
URL: https://github.com/apache/maven-surefire/pull/506#issuecomment-1131207467
I have been able to work around the issue by gating instances of System.out.print(str) that my tests run behind a check that str is non-empty. And replacing all instances of System.out.println() / System.out.println("") with System.out.print('\n'). You could also print System.lineSeparator() to be sure to use Windows line separators on Windows.
--
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