[SUREFIRE-2056] BufferOverflowException when encoding message with null testId

[yrodiere]

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.

---

• Make sure there is a JIRA issue 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.
• Each commit in the pull request should have a meaningful subject line and body.
• 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.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• 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 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
you have to acknowledge this by using the following check-box.

• I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004

• In any other case, please file an Apache Individual Contributor License Agreement.

[Tibor17]

@yrodiere
It would not be a problem to modify the existing test.
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.

[yrodiere]

@Tibor17 I would have, but the bug only occurs when 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>:

maven-surefire/surefire-api/src/main/java/org/apache/maven/surefire/api/stream/AbstractStreamEncoder.java

Line 206 in 79db903

lengthOfData += 4 + 1 + (int) ceil( encoder.maxBytesPerChar() * s.length() ) + 1;

In the case of UTF-8 the max bytes per char is 3.0f, while the most frequent 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, after the existing testing code, I can do that.

[Tibor17]

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.

[yrodiere]

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 (so we'll have two encode() calls and assertions in one test method), or leave my PR as is?

[Tibor17]

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?

[yrodiere]

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 gets fixed in M6, SUREFIRE-1815), so I can stay on M5 for now.

[basil]

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.

[br0nstein]

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. Also, avoid calling printf with any empty string arguments.