8293806: JDK_JAVA_OPTIONS picked up twice if launcher re-executes it self

[dsamersoff]

If the user has set LD_LIBRARY_PATH to use a particular libjvm.so, options from the JDK_JAVA_OPTIONS environment variable are picked up twice.

If an option cannot be accepted twice (e.g., -agentlib), the application fails to start.

The same happens on operating systems that doesn't support $RPATH/$ORIGIN and always have to set LD_LIBRARY_PATH and re-exec the launcher.

The fix adds one additional argument - orig_argv to JLI_Launch() and use it in on relaunch in execv() call.

All relevant jtreg tests are passed.

---

Progress

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

Issue

• JDK-8293806: JDK_JAVA_OPTIONS picked up twice if launcher re-executes it self

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10430

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

Using diff file

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

[bridgekeeper]

👋 Welcome back dsamersoff! 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]

@dsamersoff 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]

Webrevs

• 00: Full (ed310b8f)

[dholmes-ora]

Sorry @dsamersoff but I don't understand how this arises - can you explain in more detail please? I'm not seeing the connection between setting LD_LIBRARY_PATH and reading an env var twice. Thanks

[dsamersoff]

Sorry @dsamersoff but I don't understand how this arises - can you explain in more detail please? I'm not seeing the connection between setting LD_LIBRARY_PATH and reading an env var twice. Thanks

Linker reads LD_LIBRARY_PATH only once, at the time of program loading, so if for some reason launcher need to update LD_LIBRARY_PATH, it creates a new environment and re-launch it self using execvp().

But the launcher is designed so that the decision to re-launch is made after all arguments from all sources have already been processed and copied to the argv array.

[dholmes-ora]

Can't we just remove JDK_JAVA_OPTIONS from the new env in that case?

[dsamersoff]

Unfortunately not.
Java application (like IDE) may rely on JDK_JAVA_OPTIONS variable to pass Java options to it's child processes.

[dholmes-ora]

Sorry Dmitry I'm still trying to get my head around the big picture here. The launcher startup process is extremely complex but also quite sophisticated so I'm a bit surprised this issue exists (and that we have not noticed it before!). I have an impression that the intent if we re-exec is that the env var options have already been processed and so the newly exec'd java is not supposed to process them again (they have already been converted to items in argv) - see JLI_AddArgsFromEnvVar and the use of the relaunch field. It seems quite redundant to have the exec'd java repeat all the arg processing that has already taken place.

I'm also struggling to understand under what conditions, exactly, we will re-exec when the data-model selection is no longer an issue.

[AlanBateman]

I think I agree with David that the JBS issue or PR needs a better description of the issue. It reads to me that this is caused by setting LD_LIBRARY_PATH to find a libjvm that is coming from somewhere else, is that right?

[jddarcy]

Years back, I worked on maintaining the launcher. Previously the launcher could re-exec to change data model (e.g. change from 32 to 64 bit on a Solaris environment that supported both options) and to pick a different version.

IIRC, both features were dropped so I would also need some additional context and explanation as it what is being attempted here.

[dsamersoff]

I updated description in the CR, https://bugs.openjdk.org/browse/JDK-8293806 but for convenience copy it here. Also, I added some debug output to the comments for CR

Under some circumstances, the launcher have to update LD_LIBRARY_PATH, but the runtime linker only considers this variable during application startup, so after updating LD_LIBRARY_PATH the launcher must re-execute it self by calling execvp().

The decision to re-launch is made after all options from all sources (JDK_JAVA_OPTIONS, @argfile) already been processed and copied to argv[] array. Thus, parameters passed through JDK_JAVA_OPTIONS variable are processed twice.

If an option cannot be accepted twice (e.g., -agentlib), the application fails to start.

We cannot just remove the JDK_JAVA_OPTIONS variable because some applications (e.g. IDEs) use it to pass user parameters through the chain of child java processes.

It happens under following conditions:

1. Under MUSL libc launcher have to set LD_LIBRARY_PATH to resolve dependency between libjava and libjvm
2. On AIX launcher have to set LD_LIBRARY_PATH because the runtime linker doesn't support $ORIGIN
3. User set LD_LIBRARY_PATH variable to some location containing libjvm.so. According to comment in java_md.c, LD_LIBRARY_PATH have to be updated to avoid recursion.

[dholmes-ora]

I'm still somewhat perplexed as the launcher appears to to try and deal with this as I said before "see JLI_AddArgsFromEnvVar and the use of the relaunch field". So something seems amiss here. If that logic is not supposed to deal with the current problem, then what exactly is it intended for?

[dsamersoff]

JLI_InitArgProcessing(jboolean hasJavaArgs, jboolean disableArgFile) {
    // No expansion for relaunch
    if (argsCount != 1) {
        relaunch = JNI_TRUE;
        stopExpansion = JNI_TRUE;
        argsCount = 1;
    } else {

The code, that set relaunch variable (see above) doesn't really work. Because at the point (below) when the function is executed the number of arguments is always 1.

main(int argc, char **argv)
{
       ...
       JLI_List args = JLI_List_new(argc + 1);
        int i = 0;

      // Add first arg, which is the app name
        JLI_List_add(args, JLI_StringDup(argv[0]));
        // Append JDK_JAVA_OPTIONS
        if (JLI_AddArgsFromEnvVar(args, JDK_JAVA_OPTIONS)) {

[dholmes-ora]

Then can we fix the use of relaunch if it isn't working?

[dsamersoff]

It's exactly what I'm trying to do - fix the broken logic around relaunch variable.

We have couple of options here:

1. Decide re-launch/not-relaunch before processing arguments.

   • it's (IMHO) the best solution, but it requires significant refactor of the launcher.

2. Pass initial argv to the secondary launcher, which is re-launched and repeat argument processing.

   • it's what was implemented in this patch.

3. Let the secondary launcher know that the arguments was already processed.

   • to do this we need to use some kind of IPC: another environment variable; additional command-line option, which we will then remove etc.
     @dholmes-ora if you think that this is the way, please advice - which method of such communication you prefer.

[dholmes-ora]

I don't think solution #2 is desirable whilst the broken re-launch logic remains in place - but I might be swayed to defer that code removal to a separate RFE.

That said solution #1 is certainly preferable as there was obviously an intent that relaunch was handled correctly.

Happy to hear other opinions.

[bridgekeeper]

@dsamersoff This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[dsamersoff]

Take the approach to re-launch as early as possible.

#11538