JDK-8261167: print_process_memory_info add a close call after fopen #2409

MBaesken · 2021-02-04T16:54:57Z

Hello, there is an fopen call opening "proc/self/status" in print_process_memory_info in os_linux.cpp. This call should also have a corresponding fclose-call.

See also the Sonar finding ("Opened stream never closed. Potential resource leak") :
https://sonarcloud.io/project/issues?id=shipilev_jdk&open=AXck8BNvBBG2CXpcnaeA&resolved=false&severities=BLOCKER&types=BUG

Progress

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

Issue

JDK-8261167: print_process_memory_info add a close call after fopen

Reviewers

Thomas Stuefe (@tstuefe - Reviewer) ⚠️ Review applies to a03fa5f
David Holmes (@dholmes-ora - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2409/head:pull/2409
$ git checkout pull/2409

bridgekeeper · 2021-02-04T16:55:56Z

👋 Welcome back mbaesken! 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-02-04T16:56:49Z

@MBaesken The following label will be automatically applied to this pull request:

hotspot-runtime

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

mlbridge · 2021-02-04T16:59:53Z

Webrevs

01: Full - Incremental (858ea53)
00: Full (a03fa5f)

tstuefe

LGTM

openjdk · 2021-02-04T17:10:18Z

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

8261167: print_process_memory_info add a close call after fopen

Reviewed-by: stuefe, dholmes

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

78b0d32: 8234534: Simplify CardTable code after CMS removal
ee2f205: 8260926: Trace resource exhausted events unconditionally
1e0a101: 8259862: MutableSpace's end should be atomic
d2bd499: 8163498: Many long-running security libs tests
c5bb109: 8260019: Move some Thread subtypes out of thread.hpp
08f7454: 8261190: restore original Alibaba copyright line in two files
3bb6a3d: 8261109: [macOS] Remove disabled warning for JNF in make/autoconf/flags-cflags.m4
c59e4b6: 8261106: Reduce inclusion of jniHandles.hpp
d9aefa3: 8260356: (tz) Upgrade time-zone data to tzdata2021a
bdcbfa1: 8259927: Windows jpackage installer issues
... and 1 more: https://git.openjdk.java.net/jdk/compare/c1dea39d08e3a9af03430b603e24192a02cde223...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.

dcubed-ojdk · 2021-02-04T18:13:45Z

src/hotspot/os/linux/os_linux.cpp

-         )
-    {
-      num_found ++;
+  if (f) {


Please don't use implied booleans. This should be:
if (f != NULL) {

What Dan said. Plus if you are now checking the fopen succeeded shouldn't you also print something if it failed?

Thanks,
David

hi Daniel and David, I added a NULL check, and some output.
I noticed that a number of other fopen calls miss the NULL check + output in case of failure as well, should I adjust that in another change?

Best regards, Matthias

dholmes-ora · 2021-02-04T23:29:52Z

src/hotspot/os/linux/os_linux.cpp

-         )
-    {
-      num_found ++;
+  if (f) {


What Dan said. Plus if you are now checking the fopen succeeded shouldn't you also print something if it failed?

Thanks,
David

mlbridge · 2021-02-05T08:47:42Z

Mailing list message from David Holmes on hotspot-runtime-dev:

On 5/02/2021 6:08 pm, Matthias Baesken wrote:

On Thu, 4 Feb 2021 23:29:52 GMT, David Holmes <dholmes at openjdk.org> wrote:

src/hotspot/os/linux/os_linux.cpp line 2198:

2196: FILE* f = ::fopen("/proc/self/status", "r");
2197: char buf[256];
2198: if (f) {

Please don't use implied booleans. This should be:
`if (f != NULL) {`

What Dan said. Plus if you are now checking the fopen succeeded shouldn't you also print something if it failed?

Thanks,
David

hi Daniel and David, I added a NULL check, and some output.
I noticed that a number of other fopen calls miss the NULL check + output in case of failure as well, should I adjust that in another change?

I guess so. Though the output will depend on context - some things you
won't be able to print an error message.

I'm trying to recall if there was some discussion that these /proc open
calls really can't fail hence the lack of checking ...

Cheers,
David

MBaesken · 2021-02-05T08:56:01Z

I'm trying to recall if there was some discussion that these /proc open
calls really can't fail hence the lack of checking ...

Hi David, those other calls are checked but just in the style if (fp) { ... }
without the explicit NULL check .
For example

jdk/src/hotspot/os/linux/os_linux.cpp

Line 1032 in 78b0d32

FILE *fp = fopen("/proc/self/maps", "r");

MBaesken · 2021-02-05T13:37:12Z

/integrate

openjdk · 2021-02-05T13:38:08Z

@MBaesken Since your change was applied there have been 15 commits pushed to the master branch:

48f5220: 8260369: [PPC64] Add support for JDK-8200555
224c166: 8261213: [BACKOUT] MutableSpace's end should be atomic
3495feb: 8260296: SA's dumpreplaydata fails
0791899: 8261154: Memory leak in Java_java_lang_ClassLoader_defineClass0 with long class names
78b0d32: 8234534: Simplify CardTable code after CMS removal
ee2f205: 8260926: Trace resource exhausted events unconditionally
1e0a101: 8259862: MutableSpace's end should be atomic
d2bd499: 8163498: Many long-running security libs tests
c5bb109: 8260019: Move some Thread subtypes out of thread.hpp
08f7454: 8261190: restore original Alibaba copyright line in two files
... and 5 more: https://git.openjdk.java.net/jdk/compare/c1dea39d08e3a9af03430b603e24192a02cde223...master

Your commit was automatically rebased without conflicts.

Pushed as commit 43ae0cf.

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

JDK-8261167

a03fa5f

openjdk bot added the rfr Pull request is ready for review label Feb 4, 2021

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Feb 4, 2021

tstuefe approved these changes Feb 4, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 4, 2021

dcubed-ojdk suggested changes Feb 4, 2021

View reviewed changes

dholmes-ora suggested changes Feb 4, 2021

View reviewed changes

Add explicit NULL check and output in failure case

858ea53

dholmes-ora approved these changes Feb 5, 2021

View reviewed changes

openjdk bot closed this Feb 5, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 5, 2021

JDK-8261167: print_process_memory_info add a close call after fopen #2409

JDK-8261167: print_process_memory_info add a close call after fopen #2409

MBaesken commented Feb 4, 2021 •

edited by openjdk bot

bridgekeeper bot commented Feb 4, 2021

openjdk bot commented Feb 4, 2021

mlbridge bot commented Feb 4, 2021 •

edited

tstuefe left a comment

openjdk bot commented Feb 4, 2021 •

edited

dcubed-ojdk Feb 4, 2021

dholmes-ora Feb 4, 2021

MBaesken Feb 5, 2021

dholmes-ora Feb 4, 2021

mlbridge bot commented Feb 5, 2021

MBaesken commented Feb 5, 2021

MBaesken commented Feb 5, 2021

openjdk bot commented Feb 5, 2021

JDK-8261167: print_process_memory_info add a close call after fopen #2409

JDK-8261167: print_process_memory_info add a close call after fopen #2409

Conversation

MBaesken commented Feb 4, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Feb 4, 2021

openjdk bot commented Feb 4, 2021

mlbridge bot commented Feb 4, 2021 • edited

Webrevs

tstuefe left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 4, 2021 • edited

dcubed-ojdk Feb 4, 2021

Choose a reason for hiding this comment

dholmes-ora Feb 4, 2021

Choose a reason for hiding this comment

MBaesken Feb 5, 2021

Choose a reason for hiding this comment

dholmes-ora Feb 4, 2021

Choose a reason for hiding this comment

mlbridge bot commented Feb 5, 2021

MBaesken commented Feb 5, 2021

MBaesken commented Feb 5, 2021

openjdk bot commented Feb 5, 2021

MBaesken commented Feb 4, 2021 •

edited by openjdk bot

mlbridge bot commented Feb 4, 2021 •

edited

openjdk bot commented Feb 4, 2021 •

edited