8299665: /proc/self/stat parsing in libmanagement broken by execname with spaces

[kevinjwalls]

Code that parses /proc/self/stat may be broken if the executable name contains spaces.

We have code that does this correctly, and code that does not.
UnixOperatingSystem.c is Linux-specific OS code, and has vread_statdata which correctly uses strrchr to find the last ) and parses from there.

OperatingSystemImpl.c the generic, not Linux-specific code, is not OK, as Java_com_sun_management_internal_OperatingSystemImpl_getCommittedVirtualMemorySize0 has a problematic fscanf call.
But the problematic fscanf call is Linux-specific: we use ifdef to create OS-specific implementations.

Need to change the fscanf of /proc/self/stat to do the same buffered read as read_ticks in UnixOperatingSystem.c (the Linux-specific code), with strrchr to read past the ) of the execname.

Moving the actually-Linux-specific getCommittedVirtualMemorySize0 to be with the other Linux-specific code means it can just use the existing method for accessing /proc/self/stat.

It does however call throw_internal_error, which is then not accessible. So that function can be shared in the libmanagement_ext.h/.c for both uses.

The call to throw_internal_error is working: if I change the code to fail, it reports a java.lang.InternalError as it would before.

Existing tests still pass:
i.e. test/jdk/com/sun/management/OperatingSystemMXBean/GetCommittedVirtualMemorySize.java

---

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

Issue

• JDK-8299665: /proc/self/stat parsing in libmanagement broken by execname with spaces

Reviewers

• Serguei Spitsyn (@sspitsyn - Reviewer)
• Alex Menkov (@alexmenkov - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14107/head:pull/14107
$ git checkout pull/14107

Update a local copy of the PR:
$ git checkout pull/14107
$ git pull https://git.openjdk.org/jdk.git pull/14107/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14107

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14107.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back kevinw! 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]

@kevinjwalls The following labels will be automatically applied to this pull request:

• jmx
• serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 00: Full (4099d5ef)

[sspitsyn]

How do you test the issue is fixed now?
Is there any test case which fails without your fix and passes with it?

[kevinjwalls]

How do you test the issue is fixed now? Is there any test case which fails without your fix and passes with it?

Hi - I tested manually, renaming the java executable to something with a space, then query the OperatingSystem MBean, and see something like: CommittedVirtualMemorySize = (java.lang.Long) 36202319872 (which is the same/similar to another run
when not renamed). The problem is seeing a completely wrong value, like: CommittedVirtualMemorySize = (java.lang.Long) 0

These are lines like:

$ cat /proc/9496/stat
9496 (java) S 625 9495 607 34823 9635 1077936192 12024 0 0 0 156 22 0 0 20 0 28 0 2172294643 36260237312 30463 18446744073709551615 93928094162944 93928094166632 140724289571696 0 0 0 4 0 16800975 0 0 0 -1 11 0 0 0 0 0 93928094174456 93928094175248 93928124657664 140724289577708 140724289577894 140724289577894 140724289581032 0

That's a different run, where 36260237312 was the value we wanted. If "java" becomes "this is java" (with 2 spaces), then we read field 21, not field 23 from the line of data and show zero.

It's quite a niche thing to do so I didn't add a test that copied the java binary under test to a new name with a space in it...
The code that does the strrchr to scan from the last ) and then parses the remainder has been with us for some time. 8-)

[sspitsyn]

Thank you for explanation! Looks good.

[openjdk]

@kevinjwalls This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8299665: /proc/self/stat parsing in libmanagement broken by execname with spaces

Reviewed-by: sspitsyn, amenkov

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 45 new commits pushed to the master branch:

• 1451ac1: 8307652: sealed class hierarchy graph doesn't distinguish non-sealed classes
• 207fbcb: 8299414: JVMTI FollowReferences should support references from VirtualThread stack
• b44fa36: 8308040: Evaluate new public types in non-public classes
• ac89e30: 8308291: compiler/jvmci/meta/ProfilingInfoTest.java fails with -XX:TieredStopAtLevel=1
• bacf652: 8297887: Update Siphash
• 89f2d45: 8308237: add JDWP and JDI virtual thread support for ThreadReference.PopFrames
• d10467e: 8307738: JFR: EventStream.openRepository() drops events
• 736b90d: 8308310: HttpClient: Avoid logging or locking from within synchronized blocks
• 7764f46: 8308761: New test TestHFA needs adaptation for JDK-8308276
• d7245f7: 8305578: X11GraphicsDevice.pGetBounds() is slow in remote X11 sessions
• ... and 35 more: https://git.openjdk.org/jdk/compare/878162b362f64e5f434f8ad340514c9f94a2b4ea...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[kevinjwalls]

Thanks @sspitsyn and @alexmenkov for the reviews!

[kevinjwalls]

/integrate

[openjdk]

Going to push as commit 17ef8a4.
Since your change was applied there have been 82 commits pushed to the master branch:

• c494770: 8306812: Open source several AWT Miscellaneous tests
• d3b9b36: 8308009: Generational ZGC: OOM before clearing all SoftReferences
• bac02b6: 8305189: C2 failed "assert(_outcnt==1) failed: not unique"
• 62537d2: 8299974: Replace NULL with nullptr in share/adlc/
• f09345b: 8308931: Problemlist compiler/jvmci/TestUncaughtErrorInCompileMethod.java
• 7c072db: 8308844: ProblemList gc/z/TestHighUsage.java with Generational ZGC on windows x64
• 4becb7b: 8306137: Open source several AWT ScrollPane related tests
• 199b1bf: 8308583: SIGSEGV in GraphKit::gen_checkcast
• 46c4da7: 8159023: Engineering notation of DecimalFormat does not work as documented
• ee321c7: 8308907: ProblemList java/awt/Toolkit/GetScreenInsetsCustomGC/GetScreenInsetsCustomGC.java on linux-x64
• ... and 72 more: https://git.openjdk.org/jdk/compare/878162b362f64e5f434f8ad340514c9f94a2b4ea...master

Your commit was automatically rebased without conflicts.

[openjdk]

@kevinjwalls Pushed as commit 17ef8a4.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.