Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8282219: jdk/java/lang/ProcessBuilder/Basic.java fails on AIX #7574

Closed
wants to merge 5 commits into from

Conversation

takiguc
Copy link

@takiguc takiguc commented Feb 22, 2022

Run jtreg:jdk/java/lang/ProcessBuilder/Basic.java on AIX.
The test was failed by:
Incorrect handling of envstrings containing NULs

According to my investigation, this issue was happened after following change was applied.
JDK-8272600: (test) Use native "sleep" in Basic.java

test.nativepath value was added into AIX's LIBPATH during running this testcase.
On AIX, test.nativepath value should be removed from LIBPATH value before comparing the values.


Progress

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

Issue

  • JDK-8282219: jdk/java/lang/ProcessBuilder/Basic.java fails on AIX

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7574

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 22, 2022

👋 Welcome back itakiguchi! 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 openjdk bot added the rfr Pull request is ready for review label Feb 22, 2022
@openjdk
Copy link

openjdk bot commented Feb 22, 2022

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

  • core-libs

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.

@openjdk openjdk bot added the core-libs core-libs-dev@openjdk.org label Feb 22, 2022
@mlbridge
Copy link

mlbridge bot commented Feb 22, 2022

Webrevs

@@ -1875,7 +1888,7 @@ public void doIt(Map<String,String> environ) {
}
Process p = Runtime.getRuntime().exec(cmdp, envp);
String expected = Windows.is() ? "=C:=\\,=ExitValue=3,SystemRoot="+systemRoot+"," : "=C:=\\,";
expected = AIX.is() ? expected + "LIBPATH="+libpath+",": expected;
expected = AIX.is() ? expected + "LIBPATH="+removeTestNativepathString(libpath)+",": expected;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can line 1878 (and the fix) be moved into the removeAixExpectedVars method.

1883:            if (AIX.is()) {
1884:                commandOutput = removeAixExpectedVars(commandOutput);
1885:           }

The replaceAll used in that method should be just as effective as the more expensive new method.

@takiguc
Copy link
Author

takiguc commented Feb 22, 2022

@RogerRiggs
I appreciate your good suggestion.
Since libpath is just used on AIX and it's static final String.
I created expectedLibpath for expected result.
About removeAll method, it requires regular expression string.
I think if directory name has ., unexpected situation may be happened.

Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my curiosity, how is AIX different from other Linux in that the test.nativepath is not/should not be in LIBPATH?

String commandOutput = commandOutput(p);
if (MacOSX.is()) {
commandOutput = removeMacExpectedVars(commandOutput);
}
if (AIX.is()) {
commandOutput = removeAixExpectedVars(commandOutput);
expected = expected + "LIBPATH="+expectedLibpath+",";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add spaces around operators +, that is the convention and in this file.

@@ -77,6 +78,21 @@

/* used for AIX only */
static final String libpath = System.getenv("LIBPATH");
static final String expectedLibpath;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check the usage of libpath at line 1362. How is this case different from the other two at 1878 and 1943?
I would move the change to the expected value to move into the block that is already testing AIX.is at line 1367.

I would prefer to have a single static with the libpath expected by the 3 places it is used in the test.

@backwaterred
Copy link
Contributor

backwaterred commented Feb 22, 2022

Looks you and I are thinking similarly. Your issue was opened an hour or so after I did my search for duplicates.

#7581


I took a slightly different approach by setting LIBPATH in the environment vars passed into the exec method.

@takiguc
Copy link
Author

takiguc commented Feb 23, 2022

@RogerRiggs
Many thanks. that's good point.
Only 1st part has test.nativepath because of following code.

if (AIX.is()) {
    pb.environment().put("LIBPATH", libpath);
}

On current condition, parent (main) process have test.nativepath setting into LIBPATH environment, but child process does not require test.nativepath setting.
So just libpath value should not have test.nativepath related entry.
And above code does not require on current condition and this test said "Pass Empty environment to child".
So it should be removed.

#7581 is exactly same issue.
Please choose the appropriate one.

@backwaterred
Copy link
Contributor

backwaterred commented Feb 23, 2022

Hi @takiguc 👋, thanks for your changes. I closed my PR in favour of yours; we definitely don't need two PRs for this issue :-)

One comment on the approach you took. I considered modifying the static libpath variable as well, but what really swayed me away from choosing that route is the Windows tests. On Windows, the situation is analogous to AIX in that a static systemroot variable is set by querying the parent environment, but it is explicitly passed to the child process[es] when they are created. This ensures that the systemroot returned by the child process is the same as the expected value. Admittedly it's a bit of a nit-pick, but I think having the Windows version of the test do one thing and AIX version do another make it harder to understand what is going on. It's for that reason that I took the approach that I did in my PR.

String libpathString = System.getenv("LIBPATH");
if (AIX.is()) {
String nativepath = System.getProperty("test.nativepath");
if (nativepath != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this null check should be modified. There are two issues at play.

  • If nativepath is null it will be caught by the filter below. So it is safe to pass it past this point in either case (null or non-null).
  • If libpathString is null there will be a NullPointerException when split gets called on it below.

I believe there are at least two reasonable choices here. The first is to change nativepath -> libpathString in this null check. This option guards against calling String.split on a null value. Alternatively, you could trust that since this code doesn't get called unless AIX.is() returns true, that it's safe to remove this check altogether. This second option is only viable if we know LIBPATH is always set on AIX.

@takiguc
Copy link
Author

takiguc commented Feb 23, 2022

@backwaterred
I applied change. Please check this one ?

@RogerRiggs
Copy link
Contributor

Which version of AIX is this failing on? I notice that for AIX 5.3, the usage of LIBPATH is being migrated to LD_LIBRARY_PATH with LIBPATH as a fallback.

Jtreg when setting up the test with the "/native" on the @run line adds test.nativepath to LIBPATH.
What I'm puzzled by is the original case in which test.nativepath was appended to LIBPATH and passed to the child and yet the child returns the value of LIBPATH without test.nativepath.
It may not be material to fixing the errors, but does make me curious about what's really going on.

@RogerRiggs
Copy link
Contributor

The changes seem ok, modifying both the LIBPATH passed in the environment and the expected return value.

Since both of these tests are testing a different condition, the presence/accuracy of LIBPATH is not the point of the test.

@takiguc
Copy link
Author

takiguc commented Feb 24, 2022

@RogerRiggs
I'm using AIX 7.1.
AIX 5.3 and 6.1 were already in EOL.
OK, another way, LIBPATH=... can be removed from result.
Also we can use #7581 .

@tstuefe
Copy link
Member

tstuefe commented Feb 24, 2022

I admit I'm confused about this issue.

When exactly is jtreg modifying LIBPATH of the parent process? Between initializing the final static libpath variable and when we build the "expected" String at line 1361? Or after we built the expected String, when the child is spawned?

If its the former, then the issue is that libpath is just outdated when we get around to use it? In that case, why not just re-aquiring LIBPATH when building up expected?

Sorry, I just try to understand the issue. Thanks!

@takiguc
Copy link
Author

takiguc commented Feb 24, 2022

In my investigation, this issue happened after JDK-8272600 was applied.
JDK-8272600 (test) Use native "sleep" in Basic.java

It added /native on @run tag.

- @run main/othervm/timeout=300 -Djava.security.manager=allow Basic
- @run main/othervm/timeout=300 -Djava.security.manager=allow -Djdk.lang.Process.launchMechanism=fork Basic
+ @run main/othervm/native/timeout=300 -Djava.security.manager=allow Basic
+ @run main/othervm/native/timeout=300 -Djava.security.manager=allow -Djdk.lang.Process.launchMechanism=fork Basic

For this change, test.nativepath setting added into LIBPATH environment variable on parent (main) process.

Older AIX might require parent's LIBPATH setting against child process on some of cases.
But it seems the current AIX does not require LIBPATH setting.
#7581 applied parent LIBPATH setting against child process.
Please choose the appropriate one.

@RogerRiggs
Copy link
Contributor

Jtreg constructs the environment before launching the Agent to run the test.
https://github.com/openjdk/jtreg/blob/163ae223409f789cb733d32ed8d33686e14a81d9/src/share/classes/com/sun/javatest/regtest/exec/MainAction.java#L415

For AIX, it appends the test.nativepath to LIBPATH.
https://github.com/openjdk/jtreg/blob/163ae223409f789cb733d32ed8d33686e14a81d9/src/share/classes/com/sun/javatest/regtest/exec/Action.java#L146

As far as I can tell, the test does not modify the environment and originally passes LIBPATH without modification.

@backwaterred
Copy link
Contributor

If its the former, then the issue is that libpath is just outdated when we get around to use it? In that case, why not just re-aquiring LIBPATH when building up expected?

^This was my thought at first as well :-) but in my investigation, the libpath in the parent didn't change during the run.

I think Roger's solution matches more with my understanding of the failure. I noticed that jtreg adds the extra path to the library when invoking javac to compile the test.

/home/hotspot/openjdk/jdk-mainline/build/aix-ppc64-server-fastdebug/images/jdk/bin/javac \\
        -J-Xmx768m \\
        -J-XX:MaxRAMPercentage=3.125 \\
        -J-Djava.io.tmpdir=/home/hotspot/openjdk/jdk-mainline/build/aix-ppc64-server-fastdebug/test-support/jtreg_test_jdk_tier1/tmp \\
        -J-ea \\
        -J-esa \\
 (1) -> -J-Djava.library.path=/home/hotspot/openjdk/jdk-mainline/build/aix-ppc64-server-fastdebug/images/test/jdk/jtreg/native \\
        -J-Dtest.vm.opts='-Xmx768m -XX:MaxRAMPercentage=3.125 -Djava.io.tmpdir=/home/hotspot/openjdk/jdk-mainline/build/aix-ppc64-server-fastdebug/test-support/jtreg_test_jdk_tier1/tmp -ea -esa' \\
...

The path at (1) above is the extra part of LIBPATH that causes the test to fail on my machine.

So it would seem that javac adds the path to jtreg/native as part of the libpath for the parent process, but this path is not part of the default set of library paths for all created processes. For the child, it needs to be put in or taken out explicitly.

@RogerRiggs
Copy link
Contributor

Javac is compiling the source to a .class file. The contents of the java.library.path do not affect the class file generated.
None of the code of the class is executed during compilation.

@takiguc
Copy link
Author

takiguc commented Feb 24, 2022

Thanks @RogerRiggs
I could understand why this issue was happened.

You said:

As far as I can tell, the test does not modify the environment and originally passes LIBPATH without modification.

I'd like to confirm one thing.
My patch did not pass LIBPATH environment variable to child process.
You mean the testcase should pass parent LIBPATH to child process ?
If so, it's better to use #7581 .
(Please use bugid 8282219)

@backwaterred
Copy link
Contributor

backwaterred commented Feb 24, 2022

Javac is compiling the source to a .class file. The contents of the java.library.path do not affect the class file generated. None of the code of the class is executed during compilation.

Yup. Not the best snippet to include. nativepath is set while calling the jvm.

    /home/hotspot/openjdk/jdk-mainline/build/aix-ppc64-server-fastdebug/images/jdk/bin/java \\
...
 (2) -> -Dtest.nativepath=/home/hotspot/openjdk/jdk-mainline/build/aix-ppc64-server-fastdebug/images/test/jdk/jtreg/native \\
...

@RogerRiggs
Copy link
Contributor

The piece I was missing is that the HotSpot AIX specific code, computes and sets LIBPATH based on
the path to the java executable. This computed LIBPATH value is set in the environment unless
it is overridden by an existing LIBPATH in the environment.

The test cases that failed, do not supply a value for LIBPATH so the launcher provided value is inserted
and thereafter reported as the environment from the child process.

Since both of these tests are testing a different condition not related to LIBPATH,
the presence/accuracy of LIBPATH is not the point of the test.

The current solution to remove test.nativepath works and is ok by me.

(The alternative used by #7581, to include LIBPATH in the environment when launching might be slightly more robust to future changes.)

Thanks for the followup.

@tstuefe
Copy link
Member

tstuefe commented Feb 25, 2022

Hi Roger,

The piece I was missing is that the HotSpot AIX specific code, computes and sets LIBPATH based on the path to the java executable. This computed LIBPATH value is set in the environment unless it is overridden by an existing LIBPATH in the environment.

Ah, thank you for this missing piece! Now I remember. We modify LIBPATH in the java launcher itself. Since the test executable here is also java we call setenv(LIBPATH,..) in the child process. On AIX we do this unconditionally, that may be the difference to other platforms.

The point of this test was not to test System.getenv() but that Runtime.exec passes the env vector correctly, right? Then a maybe simpler alternative would have been to spawn sh -c env, or a simple little C program printing its env vector, instead of java. Less side effects. Test may be a bit quicker too, since child startup would be faster (probably does not save much in the big scheme of things).

Thanks, Thomas

@takiguc
Copy link
Author

takiguc commented Feb 25, 2022

Thanks @RogerRiggs and @tstuefe .
I appreciate your deeper investigations.
Could you give me some advice on what to do next?

@tstuefe
Copy link
Member

tstuefe commented Feb 25, 2022

Thanks @RogerRiggs and @tstuefe . I appreciate your deeper investigations. Could you give me some advice on what to do next?

Hi @takiguc,

My preference would be to spawn sh -c env instead of java, since it removes all need for second-guessing what the java child does with the environment (java modifies the environment and also conceivably System.getenv() may lie to us at some point in the future since it seems a convenient way to filter environment variables for java programs). That is if my reasoning was correct in the first place.

But I do not have a strong preference and defer to Roger and Tyler.

Cheers, Thomas

@takiguc
Copy link
Author

takiguc commented Feb 25, 2022

Hello @RogerRiggs , @backwaterred .
Could you give me your suggestion, please ?

@RogerRiggs
Copy link
Contributor

RogerRiggs commented Feb 25, 2022

My preference is to pass the unmodified LIBPATH in the environment in each of the three cases.
The expected checks already expect the unmodified LIBPATH so the only change is in the setup of the environment to be passed to the child.

Already covered at line 1366:

Insert at line 1873 in the if/then/else:

            } else if (AIX.is()) {
                envp = new String[] {"=ExitValue=3", "=C:=\\", "LIBPATH="+libpath};
            } else {

(Or assign it to a local as is done for windows, your preference).

And at line 1921'ish:

           } else if (AIX.is()) {
                envp = new String[] {"LC_ALL=C\u0000\u0000", // Yuck!
                        "FO\u0000=B\u0000R", "LIBPATH="+libpath};

I looked using sh -c env but I think it would be more work to change the way the output is compared.
The output of env is different and has some other values that get inserted.

@mlbridge
Copy link

mlbridge bot commented Feb 25, 2022

Mailing list message from Joe Darcy on core-libs-dev:

How about excluding the test from running on AIX?

-Joe

On 2/25/2022 7:07 AM, Roger Riggs wrote:

@backwaterred
Copy link
Contributor

I think it's good to keep the test on AIX since we have good solutions to the failure.

I added +1 to Roger's suggestion above. Looks like it should work well.

@mlbridge
Copy link

mlbridge bot commented Feb 25, 2022

Mailing list message from Thomas Stüfe on core-libs-dev:

IMHO this is not hard to fix (we already have multiple proposals) and we'd
should have this test on AIX too.

Cheers, Thomas

On Fri, Feb 25, 2022 at 6:23 PM Joe Darcy <joe.darcy at oracle.com> wrote:

How about excluding the test from running on AIX?

-Joe

On 2/25/2022 7:07 AM, Roger Riggs wrote:

On Wed, 23 Feb 2022 18:49:22 GMT, Ichiroh Takiguchi <
itakiguchi at openjdk.org> wrote:

Run jtreg:jdk/java/lang/ProcessBuilder/Basic.java on AIX.
The test was failed by:
Incorrect handling of envstrings containing NULs

According to my investigation, this issue was happened after following
change was applied.
JDK-8272600: (test) Use native "sleep" in Basic.java

test.nativepath value was added into AIX's LIBPATH during running this
testcase.
On AIX, test.nativepath value should be removed from LIBPATH value
before comparing the values.
Ichiroh Takiguchi has updated the pull request incrementally with one
additional commit since the last revision:

Add null check
My preference is to pass the unmodified LIBPATH in the environment in
each of the three cases.
The expected checks already expect the unmodified LIBPATH so the only
change is in the setup of the environment to be passed to the child.

Already covered at line 1366:

Insert at line 1873 in the if/then/else:

         \} else if \(AIX\.is\(\)\) \{
             envp \= new String\[\] \{\"\=ExitValue\=3\"\, \"\=C\:\=\\\"\,

"LIBPATH="+libpath};

         \} else \{

(Or assign it to a local as is done for windows, your preference).

And at line 1921'ish:
``` } else if (AIX.is()) {
envp = new String[] {"LC_ALL=C\u0000\u0000", // Yuck!
"FO\u0000=B\u0000R", "LIBPATH="+libpath};

I looked using `sh -c env` but I think it would be more work to change
the way the output is compared.
The output of `env` is different and has some other values that get
inserted.

@takiguc
Copy link
Author

takiguc commented Feb 25, 2022

Many thanks, @RogerRiggs .
I tried your suggested code, it worked fine on my AIX system.
Could you review this change again ?

@openjdk
Copy link

openjdk bot commented Feb 25, 2022

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

8282219: jdk/java/lang/ProcessBuilder/Basic.java fails on AIX

Reviewed-by: rriggs

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

  • 441e485: 8281739: JFR: Use message with Objects.requireNonNull
  • e96c599: 8271232: JFR: Scrub recording data
  • 735e86b: 8282345: handle latest VS2022 in abstract_vm_version
  • b96b743: 8281015: Further simplify NMT backend
  • 9471f24: 8280684: JfrRecorderService failes with guarantee(num_written > 0) when no space left on device.
  • 3efd6aa: 8282347: AARCH64: Untaken branch in has_negatives stub
  • cd36be4: 8206187: javax/management/remote/mandatory/connection/DefaultAgentFilterTest.java fails with Port already in use
  • bf19fc6: 8280357: user.home = "?" when running with systemd DynamicUser=true
  • b6843a1: 8005885: enhance PrintCodeCache to print more data
  • 23995f8: 8281525: Enable Zc:strictStrings flag in Visual Studio build
  • ... and 41 more: https://git.openjdk.java.net/jdk/compare/e0b49629e95c98aabe8b75ec2f7528e7fb6dcffc...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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Feb 25, 2022
@takiguc
Copy link
Author

takiguc commented Feb 26, 2022

/integrate

@openjdk
Copy link

openjdk bot commented Feb 26, 2022

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

  • fb8bf81: 8279995: jpackage --add-launcher option should allow overriding description
  • 441e485: 8281739: JFR: Use message with Objects.requireNonNull
  • e96c599: 8271232: JFR: Scrub recording data
  • 735e86b: 8282345: handle latest VS2022 in abstract_vm_version
  • b96b743: 8281015: Further simplify NMT backend
  • 9471f24: 8280684: JfrRecorderService failes with guarantee(num_written > 0) when no space left on device.
  • 3efd6aa: 8282347: AARCH64: Untaken branch in has_negatives stub
  • cd36be4: 8206187: javax/management/remote/mandatory/connection/DefaultAgentFilterTest.java fails with Port already in use
  • bf19fc6: 8280357: user.home = "?" when running with systemd DynamicUser=true
  • b6843a1: 8005885: enhance PrintCodeCache to print more data
  • ... and 42 more: https://git.openjdk.java.net/jdk/compare/e0b49629e95c98aabe8b75ec2f7528e7fb6dcffc...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Feb 26, 2022
@openjdk openjdk bot closed this Feb 26, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 26, 2022
@openjdk
Copy link

openjdk bot commented Feb 26, 2022

@takiguc Pushed as commit c5c6058.

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

@takiguc
Copy link
Author

takiguc commented Feb 28, 2022

/backport jdk17u-dev

@openjdk
Copy link

openjdk bot commented Feb 28, 2022

@takiguc Unknown command backport - for a list of valid commands use /help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated
4 participants