8333871: Check sysinfo return values #28317

MBaesken · 2025-11-14T10:33:07Z

In the review of JDK-8333522 it has been discussed that we might run into issues because of failing sysinfo.
However we miss checking the return values at some places, this might need adjustment.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8333871: Check sysinfo return values (Bug - P4)

Reviewers

Alan Bateman (@AlanBateman - Reviewer) 🔄 Re-review required (review applies to e9d2b1b4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28317

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-11-14T10:35:09Z

👋 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 · 2025-11-14T10:36:17Z

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

openjdk · 2025-11-14T10:37:15Z

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

core-libs
hotspot-runtime

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 · 2025-11-14T10:40:45Z

Webrevs

src/java.base/unix/native/libjava/java_props_md.c

AlanBateman · 2025-11-14T12:08:05Z

src/java.base/unix/native/libjava/java_props_md.c

+        if (ret == 0) {
+            sprops.cpu_isalist = strdup(list);
+        } else {
+            sprops.cpu_isalist = NULL;


Would you mind checking the code that puts this in a system property can tolerate NULL? I don't suppose sysinfo will fail here (unless you found a case?) but it would be good to confirm that it won't blow up somewhere else.

We do PUTPROP here

jdk/src/java.base/share/native/libjava/System.c

Line 164 in 5d65c23

PUTPROP(propArray, _sun_cpu_isalist_NDX, sprops->cpu_isalist);

with 3rd parameter == NULL in this case.
PUTPROP checks for this so I do not see issues with this

jdk/src/java.base/share/native/libjava/System.c

Line 68 in 5d65c23

#define PUTPROP(array, prop_index, val) \

AlanBateman · 2025-11-16T17:26:33Z

/reviewers 2

openjdk · 2025-11-16T17:27:58Z

@AlanBateman
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

AlanBateman

Thanks for checking that cpu_isalist == NULL is okay, the update to java_props_md.c looks okay to me.

dholmes-ora · 2025-11-17T01:02:42Z

I think this is completely unnecessary - sysinfo on Linux can't really fail, the only error possible is EFAULT if you pass a bad pointer for the struct - so don't do that.

Replacing numbers with words in the logging could break external parsing scripts.

MBaesken · 2025-11-18T09:20:55Z

I think this is completely unnecessary - sysinfo on Linux can't really fail, the only error possible is EFAULT if you pass a bad pointer for the struct - so don't do that.

Replacing numbers with words in the logging could break external parsing scripts.

Hi David , not sure how likely those failures are. Back then in the review of JDK-8333522 we discussed the checking might be needed and that's why the JBS issue was created.
Checking other projects for potential sysinfo failures it seems that the 'bad pointer' issue you referred to is indeed the most likely one.
But if it is really 'completely unnecessary' we should probably kick out the checks we do at a number of places in the codebase ?
https://github.com/search?q=repo%3Aopenjdk%2Fjdk+%22sysinfo%28%22&type=code

MBaesken · 2025-11-18T10:03:36Z

If you search github, you find also a number of places where EPERM is checked after sysinfo, not sure if this is really happening these days on our platforms ?

AlanBateman · 2025-11-18T11:01:59Z

If you search github, you find also a number of places where EPERM is checked after sysinfo, not sure if this is really happening these days on our platforms ?

seccomp comes up periodically where random syscalls fail with EPERM as they aren't permitted by the filter. I don't know if this is what you found in your search but the possibility of seccomp or interposer libraries creates the possibility of syscall failing, or failing with errors that aren't documented for the syscall.

dholmes-ora · 2025-11-19T07:54:52Z

the possibility of seccomp or interposer libraries creates the possibility of syscall failing, or failing with errors that aren't documented for the syscall.

That certainly complicates things and could potentially invalidate any code where we check for the expected/specified set of errors. I grudgingly withdraw my objection.

dholmes-ora · 2025-11-19T07:56:31Z

src/hotspot/os/linux/os_linux.cpp

+  } else {
+    st->print_cr("OS uptime could not be retrieved.");


I think an assert would be preferable here. This code can't be parsed so I think it better not to inject it.

dholmes-ora · 2025-11-19T07:57:15Z

src/hotspot/os/linux/os_linux.cpp

+  } else {
+    st->print(", swap could not be determined");
+  }


Again probably better to assert and say nothing, or use 0 as a placeholder.

Hi David, I added asserts where you suggested.

openjdk · 2025-11-19T10:12:38Z

@MBaesken this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8333871
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

src/hotspot/os/linux/os_linux.cpp

JDK-8333871

2d574f7

openjdk bot changed the title ~~JDK-8333871: check sysinfo return values~~ 8333871: check sysinfo return values Nov 14, 2025

openjdk bot added hotspot-runtime hotspot-runtime-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Nov 14, 2025

openjdk bot added the rfr Pull request is ready for review label Nov 14, 2025

jaikiran reviewed Nov 14, 2025

View reviewed changes

src/java.base/unix/native/libjava/java_props_md.c Outdated Show resolved Hide resolved

0 - terminate not needed

e9d2b1b

AlanBateman reviewed Nov 14, 2025

View reviewed changes

AlanBateman approved these changes Nov 16, 2025

View reviewed changes

dholmes-ora reviewed Nov 19, 2025

View reviewed changes

use more asserts

6284ef2

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Nov 19, 2025

MBaesken changed the title ~~8333871: check sysinfo return values~~ 8333871: Check sysinfo return values Nov 19, 2025

MBaesken added 2 commits November 19, 2025 13:30

Merge remote-tracking branch 'origin/master' into JDK-8333871

2f4c7c9

Merge master

e498891

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Nov 19, 2025

dholmes-ora reviewed Nov 20, 2025

View reviewed changes

src/hotspot/os/linux/os_linux.cpp Outdated Show resolved Hide resolved

Better assert messages

22a2f6a

8333871: Check sysinfo return values #28317

Are you sure you want to change the base?

8333871: Check sysinfo return values #28317

Uh oh!

Conversation

MBaesken commented Nov 14, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Nov 14, 2025

Uh oh!

openjdk bot commented Nov 14, 2025

Uh oh!

openjdk bot commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Uh oh!

AlanBateman Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

MBaesken Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

AlanBateman commented Nov 16, 2025

Uh oh!

openjdk bot commented Nov 16, 2025

Uh oh!

AlanBateman left a comment

Choose a reason for hiding this comment

Uh oh!

dholmes-ora commented Nov 17, 2025

Uh oh!

MBaesken commented Nov 18, 2025

Uh oh!

MBaesken commented Nov 18, 2025

Uh oh!

AlanBateman commented Nov 18, 2025

Uh oh!

dholmes-ora commented Nov 19, 2025

Uh oh!

dholmes-ora Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

dholmes-ora Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

MBaesken Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Nov 19, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

MBaesken commented Nov 14, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Nov 14, 2025 •

edited

Loading

mlbridge bot commented Nov 14, 2025 •

edited

Loading