8381567: ProcessHandle.descendants fails with ArrayIndexOutOfBoundException for process 0#30763
8381567: ProcessHandle.descendants fails with ArrayIndexOutOfBoundException for process 0#30763Michael-Mc-Mahon wants to merge 9 commits intoopenjdk:masterfrom
Conversation
|
👋 Welcome back michaelm! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@Michael-Mc-Mahon The following labels will be automatically applied to this pull request:
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. |
Webrevs
|
|
/label remove nio |
|
@AlanBateman |
| } | ||
| } | ||
| ppid = pids[++count]; // pick up the next pid to scan for | ||
| if (++count >= size) { |
There was a problem hiding this comment.
Do the check without incrementing and increment below.
It looks odd to bump the count even though it's not used.
|
|
||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| /* |
| */ | ||
| @Test | ||
| public void test() { | ||
| int num = ProcessHandle.of(0).orElseThrow().descendants().toList().size(); |
There was a problem hiding this comment.
Should this have any assertion? E.g. assertNotEquals(0, num) / assertTrue(num > 0) (or at least a assertDoesNotThrow to clarify the intention)?
There was a problem hiding this comment.
I can wrap the invocation in assertDoesNotThrow()
| } | ||
| ppid = pids[++count]; // pick up the next pid to scan for | ||
| ppStart = starttimes[count]; // and its start time | ||
| } while (count < next); |
There was a problem hiding this comment.
Have you tried } while (count < next && count < (size -1)) to avoid the break out of the loop.
There was a problem hiding this comment.
Have you tried
} while (count < next && count < (size -1))to avoid the break out of the loop.
It has already dereferenced the invalid index at that point. It might be possible to restructure the loop by putting the assignment to ppid and ppStart at the top, but the initial value for ppid is not taken from the array, which is probably why that code is at the end of the loop.
Co-authored-by: Andrey Turbanov <turbanoff@gmail.com>
|
The purpose of the |
I wouldn't say that is desired behavior. It's just what is observed but is platform specific and undocumented. I'm not sure special casing pid == 0 in the shared code is a good idea. What if the other platforms have similar behavior, but for different pid values? I think also checking before dereferencing an out of range index value is still prudent regardless. |
Hmm, I don't think process 0 is real and am surprised to find a process zero with parent zero in the result list. If a process zero is not real then the API should return an empty list for calls to |
ProcessHandle.of(0) returns a real handle on Mac, but not on Linux or Windows. We could disallow its creation on Mac too, but existing code could be using it. The parent of |
ok, pid 0 is the kernel on MacOS. |
Yes. While you can argue it is reasonable that We could change the native code to use I'm okay with just disallowing the creation of |
|
Any other comments on the fix? |
|
I looked at a refactoring to change the sentinel value to -1 and avoid 0. |
Okay, I'll take a look. |
Hi,
This is a small fix for j.l.ProcessHandle on MacOS. Unlike other platforms, Mac returns a ProcessHandle for pid 0 whose descendants are all processes on the system. This specific scenario tickles an off by one error where the descendants method tries to access an element past the end of the array of pids. The fix is to break from the loop before accessing this element.
Thanks,
Michael
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/30763/head:pull/30763$ git checkout pull/30763Update a local copy of the PR:
$ git checkout pull/30763$ git pull https://git.openjdk.org/jdk.git pull/30763/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 30763View PR using the GUI difftool:
$ git pr show -t 30763Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/30763.diff
Using Webrev
Link to Webrev Comment