8268361: Fix the infinite loop in next_line #4378

UncleNine · 2021-06-06T14:47:31Z

If the /proc/stat mount point is changed in container environment, the while loop may lead to 100% cpu usage.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8268361: Fix the infinite loop in next_line

Reviewers

David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to e4f90ebda68f576c35c1f24e35bb9cecf8c561da
Severin Gehwolf (@jerboaa - Reviewer)
Serguei Spitsyn (@sspitsyn - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4378/head:pull/4378
$ git checkout pull/4378

Update a local copy of the PR:
$ git checkout pull/4378
$ git pull https://git.openjdk.java.net/jdk pull/4378/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4378

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4378.diff

… in container environment, the while loop may lead to 100% cpu usage.

bridgekeeper · 2021-06-06T14:48:43Z

👋 Welcome back UncleNine! 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 · 2021-06-06T14:50:26Z

@UncleNine 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.

y1yang0 · 2021-06-08T01:57:16Z

I have encountered a similar problem before, I've filed a new issue https://bugs.openjdk.java.net/browse/JDK-8268361 for this. You can change your title to "8268361: Fix the infinite loop in next_line", then OpenJDK bot would associate your PR with the corresponding JBS issue.

Best regards,
Yang

mlbridge · 2021-06-08T03:14:26Z

Webrevs

dholmes-ora

Hi,

This fixes the potential infinite loop, but is it sufficient?

Thanks,
David

dholmes-ora · 2021-06-08T06:35:59Z

src/jdk.management/linux/native/libmanagement_ext/UnixOperatingSystem.c

+    int c;
+    do {
+        c = fgetc(f);
+    } while(c != '\n' && c != EOF);


Style nit: please add space before (

It is not obvious to me that the caller of next_line will handle the fact that we have hit EOF?

In my case, it happened in the container environment.
the /proc filesystem of the container is provided by lxcfs, but a lxcfs bug may make the /proc/stat mount point change, then the file descriptor is different and fgetc function returns an EOF on error, But c != '\n' is true and it leads to the infinit loop.
Below are our flamegraph in the production , it happends on serveral frameworks（micrometer, elasticsearch..）which use the api "sun/management/OperatingSystemImpl.getSystemCpuLoad"

I use top -Hp $PID command and the thread show 100% cpu usage

@UncleNine We should handle the case in the get_totalticks() function - which seems to be the only user of next_line() - when next_line() returns EOF as David said. One way would be to return the 'c' character read in next_line and if it's EOF, return -2 in get_totalticks().

all right, i have updated the code

jerboaa · 2021-06-08T08:39:21Z

If the /proc/stat mount point is changed in container environment, the while loop may lead to 100% cpu usage.

In what way have you observed /proc/stat being changed which manifests in 100% cpu usage?

UncleNine · 2021-06-08T16:28:34Z

If the /proc/stat mount point is changed in container environment, the while loop may lead to 100% cpu usage.

In what way have you observed /proc/stat being changed which manifests in 100% cpu usage?

In my case, it happened in the container environment.
the /proc filesystem of the container is provided by lxcfs, but a lxcfs bug may make the /proc/stat mount point change.
It was caused by a lxcfs-3.1.2 segment fault's exit

jerboaa

Looks fine to me. Please fix the jcheck (whitespace) issue.

sspitsyn

It looks okay to me.
Thanks,
Serguei

dholmes-ora

This seems better to me now.

Thanks,
David

openjdk · 2021-06-10T12:23:41Z

@UncleNine 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:

8268361: Fix the infinite loop in next_line

Reviewed-by: dholmes, sgehwolf, sspitsyn

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 113 new commits pushed to the master branch:

b018c45: 8267630: Start of release updates for JDK 18
7400789: 8268542: serviceability/logging/TestFullNames.java tests only 1st test case
a95e64c: 8268443: ParallelGC Full GC should use parallel WeakProcessor
0a6c7d8: 8267424: CTW: C1 fails with "State must not be null"
e11f70a: 8003216: Add JFR event indicating explicit System.gc() call
2b41459: 8267073: Race between Card Redirtying and Freeing Collection Set regions results in missing remembered set entries with G1
7cd5a6e: 8268537: (Temporary) Disable ParallelRefProcEnabled for Parallel GC
f716711: 8265271: JFR: Allow use of .jfc options when starting JFR
f677163: 8266130: convert Thread-SMR stress tests from counter based to time based
6c552a7: 8268544: some runtime/sealedClasses tests should be run in driver mode
... and 103 more: https://git.openjdk.java.net/jdk/compare/52d88ee1d1e0f6b9927db03a2b0bff75e4be03a2...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dholmes-ora, @jerboaa, @sspitsyn) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

jerboaa

LGTM. Thanks for your patience!

UncleNine · 2021-06-10T16:17:33Z

/integrate

openjdk · 2021-06-10T16:27:38Z

@UncleNine
Your change (at version f568406) is now ready to be sponsored by a Committer.

jerboaa · 2021-06-10T16:39:27Z

/sponsor

openjdk · 2021-06-10T16:40:34Z

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

b018c45: 8267630: Start of release updates for JDK 18
7400789: 8268542: serviceability/logging/TestFullNames.java tests only 1st test case
a95e64c: 8268443: ParallelGC Full GC should use parallel WeakProcessor
0a6c7d8: 8267424: CTW: C1 fails with "State must not be null"
e11f70a: 8003216: Add JFR event indicating explicit System.gc() call
2b41459: 8267073: Race between Card Redirtying and Freeing Collection Set regions results in missing remembered set entries with G1
7cd5a6e: 8268537: (Temporary) Disable ParallelRefProcEnabled for Parallel GC
f716711: 8265271: JFR: Allow use of .jfc options when starting JFR
f677163: 8266130: convert Thread-SMR stress tests from counter based to time based
6c552a7: 8268544: some runtime/sealedClasses tests should be run in driver mode
... and 103 more: https://git.openjdk.java.net/jdk/compare/52d88ee1d1e0f6b9927db03a2b0bff75e4be03a2...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-06-10T16:40:53Z

@jerboaa @UncleNine Pushed as commit 7267227.

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

dholmes-ora · 2021-06-11T05:54:31Z

@UncleNine please do not force push commits to an open PR as it makes it difficult for reviewers to track the changes. The PR can contain as many commits as you like as it will all be squashed to a single clean commit when integrating. Thanks.

tstuefe · 2021-06-11T06:10:22Z

Would it not be better to read the whole content of /proc/stat with a single read() call instead of line by line? I don't think proc fs guarantees any kind of consistency with separate reads.

UncleNine · 2021-06-14T14:00:14Z

@UncleNine please do not force push commits to an open PR as it makes it difficult for reviewers to track the changes. The PR can contain as many commits as you like as it will all be squashed to a single clean commit when integrating. Thanks.

Sorry,

UncleNine · 2021-06-14T14:03:38Z

@UncleNine please do not force push commits to an open PR as it makes it difficult for reviewers to track the changes. The PR can contain as many commits as you like as it will all be squashed to a single clean commit when integrating. Thanks.

Sorry,

UncleNine · 2021-06-14T14:10:26Z

Would it not be better to read the whole content of /proc/stat with a single read() call instead of line by line? I don't think proc fs guarantees any kind of consistency with separate reads.

yes, so fgetc's return value should need to be checked

Fix the dead loop.\nSummary: If the /proc/stat mount point is changed…

ba53f91

… in container environment, the while loop may lead to 100% cpu usage.

openjdk bot added serviceability serviceability-dev@openjdk.org jmx jmx-dev@openjdk.org labels Jun 6, 2021

UncleNine changed the title ~~Fix the dead loop~~ 8268361: Fix the infinite loop in next_line Jun 8, 2021

openjdk bot added the rfr Pull request is ready for review label Jun 8, 2021

dholmes-ora reviewed Jun 8, 2021

View reviewed changes

8268361: Fix the infinite loop in next_line

c055e97

openjdk bot removed the rfr Pull request is ready for review label Jun 9, 2021

jerboaa reviewed Jun 9, 2021

View reviewed changes

sspitsyn approved these changes Jun 9, 2021

View reviewed changes

8268361: Fix the infinite loop in next_line

f568406

dholmes-ora approved these changes Jun 10, 2021

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 10, 2021

sspitsyn approved these changes Jun 10, 2021

View reviewed changes

jerboaa approved these changes Jun 10, 2021

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Jun 10, 2021

openjdk bot closed this Jun 10, 2021

openjdk bot added the integrated Pull request has been integrated label Jun 10, 2021

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jun 10, 2021

UncleNine deleted the myjdk branch June 11, 2021 01:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8268361: Fix the infinite loop in next_line #4378

8268361: Fix the infinite loop in next_line #4378

UncleNine commented Jun 6, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jun 6, 2021

openjdk bot commented Jun 6, 2021

y1yang0 commented Jun 8, 2021

mlbridge bot commented Jun 8, 2021 •

edited

Loading

dholmes-ora left a comment

dholmes-ora Jun 8, 2021

dholmes-ora Jun 8, 2021

UncleNine Jun 8, 2021

UncleNine Jun 8, 2021

jerboaa Jun 9, 2021

UncleNine Jun 9, 2021

jerboaa commented Jun 8, 2021

UncleNine commented Jun 8, 2021

jerboaa left a comment

sspitsyn left a comment

dholmes-ora left a comment

openjdk bot commented Jun 10, 2021 •

edited

Loading

jerboaa left a comment

UncleNine commented Jun 10, 2021

openjdk bot commented Jun 10, 2021

jerboaa commented Jun 10, 2021

openjdk bot commented Jun 10, 2021

openjdk bot commented Jun 10, 2021

dholmes-ora commented Jun 11, 2021

tstuefe commented Jun 11, 2021

UncleNine commented Jun 14, 2021

UncleNine commented Jun 14, 2021

UncleNine commented Jun 14, 2021

8268361: Fix the infinite loop in next_line #4378

8268361: Fix the infinite loop in next_line #4378

Conversation

UncleNine commented Jun 6, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jun 6, 2021

openjdk bot commented Jun 6, 2021

y1yang0 commented Jun 8, 2021

mlbridge bot commented Jun 8, 2021 • edited Loading

Webrevs

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora Jun 8, 2021

Choose a reason for hiding this comment

dholmes-ora Jun 8, 2021

Choose a reason for hiding this comment

UncleNine Jun 8, 2021

Choose a reason for hiding this comment

UncleNine Jun 8, 2021

Choose a reason for hiding this comment

jerboaa Jun 9, 2021

Choose a reason for hiding this comment

UncleNine Jun 9, 2021

Choose a reason for hiding this comment

jerboaa commented Jun 8, 2021

UncleNine commented Jun 8, 2021

jerboaa left a comment

Choose a reason for hiding this comment

sspitsyn left a comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

openjdk bot commented Jun 10, 2021 • edited Loading

jerboaa left a comment

Choose a reason for hiding this comment

UncleNine commented Jun 10, 2021

openjdk bot commented Jun 10, 2021

jerboaa commented Jun 10, 2021

openjdk bot commented Jun 10, 2021

openjdk bot commented Jun 10, 2021

dholmes-ora commented Jun 11, 2021

tstuefe commented Jun 11, 2021

UncleNine commented Jun 14, 2021

UncleNine commented Jun 14, 2021

UncleNine commented Jun 14, 2021

UncleNine commented Jun 6, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 8, 2021 •

edited

Loading

openjdk bot commented Jun 10, 2021 •

edited

Loading