8362208: [8u] Buffer overflow in g1GCPhaseTimes.cpp::LineBuffer::_buffer

[sendaoYan]

Hi all,

When running hotspot/test/gc/g1/TestG1TraceEagerReclaimHumongousObjects.java on a CPU with more than 200 physical threads, the jvm will crashes. The reason is that the testcase turn on the gc log, which prints the statistics of each gc thread. If the machine has more cores, more gc threads will be turned on (143 gc threads on a machine with 224 physical threads). In the G1GCParPhasePrinter::print_time_values function (hotspot/src/share/vm/gc_implementation/g1/g1GCPhaseTimes.cpp), the relevant statistics of all gc threads are concatenated into one line, and the string concatenation content is saved in the array defined by g1GCPhaseTimes.cpp::LineBuffer::_buffer. Therefore, on machines with a large number of physical threads, it is easy for the GC log output line length to exceed the predefined buffer size. When the buffer size is exceeded, an error occurs when calling the os::vsnprintf function.
In JDK9, JDK-8150068 refactors the relevant GC log output, so buffer overflow will no longer occur. However, JDK-8150068 is a new feature, and JDK-8150068 cannot be directly backported to jdk8u. In addition, the amount of JDK-8150068 code is large, and the risk of backporting to jdk8u is also very high. Therefore, this PR changes the buffer length to 1024*3 to ensure that there will be no problems with GC log output in some scenarios, and leave a certain margin.

In addition, this PR adds a guarantee statement to ensure that an error is reported before calling os::vsnprintf when the buffer overflows, which is conducive to the rapid location of the problem

Change has been verified locally, risk is low.

Additional testing:

• jtreg tests include tier1/2/3 etc.. on linux-x64 with release build

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• JDK-8362208 needs maintainer approval

Issue

• JDK-8362208: [8u] Buffer overflow in g1GCPhaseTimes.cpp::LineBuffer::_buffer (Bug - P4)

Reviewers

• Paul Hohensee (@phohensee - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev.git pull/668/head:pull/668
$ git checkout pull/668

Update a local copy of the PR:
$ git checkout pull/668
$ git pull https://git.openjdk.org/jdk8u-dev.git pull/668/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 668

View PR using the GUI difftool:
$ git pr show -t 668

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/668.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back syan! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[mlbridge]

Webrevs

• 03: Full - Incremental (811ce78a)
• 02: Full - Incremental (55e4fe2b)
• 01: Full - Incremental (a132e0c0)
• 00: Full (a207869a)

[phohensee]

Afaik a guarantee is supposed to check for JVM-ending situations, so it seems to strong to me just for losing some logging info. Even an assert is imo too strong. I'd turn the guarantee into a debug-only warning with something like "previous LineBuffer overflow, request ignored" and have it execute just once. I'd also gate the whole method on (_cur < BUFFER_LEN) so vnsprintf isn't called unnecessarily.

[sendaoYan]

Afaik a guarantee is supposed to check for JVM-ending situations, so it seems to strong to me just for losing some logging info. Even an assert is imo too strong. I'd turn the guarantee into a debug-only warning with something like "previous LineBuffer overflow, request ignored" and have it execute just once. I'd also gate the whole method on (_cur < BUFFER_LEN) so vnsprintf isn't called unnecessarily.

@phohensee Thanks for your suggestions. The guaratee has been removed, and I add a debug only warning, and then return early when _cur > BUFFER_LEN

[sendaoYan]

GHA report several test failures:

1. Several 'jdk/security_infra' job fails, it's known CA test issue, it's unrelated to this PR.

[phohensee]

Looks like the test on line 44 should be

_cur >= BUFFER_LEN

because _cur is set to BUFFER_LEN at line 52 as the sentinal.

[sendaoYan]

Looks like the test on line 44 should be

_cur >= BUFFER_LEN

because _cur is set to BUFFER_LEN at line 52 as the sentinal.

Thanks for your correction. PR has been updated.

[phohensee]

Thanks, looks good.

[openjdk]

⚠️ @sendaoYan This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.