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

8286694: Incorrect argument processing in java launcher #8694

Closed
wants to merge 3 commits into from

Conversation

YaSuenag
Copy link
Member

@YaSuenag YaSuenag commented May 13, 2022

GCC 12 reports as following:

/home/ysuenaga/github-forked/jdk/src/java.base/share/native/libjli/java.c:1629:35: error: the comparison will always evaluate as 'false' for the pointer operand in 'arg + 2' must not be NULL [-Werror=address]
 1629 | *nargv++ = ((arg + 2) == NULL) ? NULL : JLI_StringDup(arg + 2);
      |

The code is here:

1625 /* Copy the VM arguments (i.e. prefixed with -J) */
1626 for (i = 0; i < jargc; i++) {
1627     const char *arg = jargv[i];
1628     if (arg[0] == '-' && arg[1] == 'J') {
1629         *nargv++ = ((arg + 2) == NULL) ? NULL : JLI_StringDup(a rg + 2);
1630     }
1631 }

It seems to be expected that NULL is set to new array for arguments if invalid parameter ( -J only) is passed.
However NULL in new array for arguments (nargv) means stop condition for argument processing in below:

1214 while ((arg = *argv) != 0 && *arg == '-') {

jargv in L1627 means builtin arguments which is defined by JAVA_ARGS or EXTRA_JAVA_ARGS macro. So it should be ignored and reported it in laucher trace log.


Progress

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

Issue

  • JDK-8286694: Incorrect argument processing in java launcher

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8694

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 13, 2022

👋 Welcome back ysuenaga! 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
Copy link

@openjdk openjdk bot commented May 13, 2022

@YaSuenag 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 label May 13, 2022
@YaSuenag YaSuenag marked this pull request as ready for review May 13, 2022
@openjdk openjdk bot added the rfr label May 13, 2022
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 13, 2022

Webrevs

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented May 16, 2022

Simply considering the compiler warning shouldn't (arg+2) == NULL actually be arg[2] == '\0' ? as we are looking to see if this arg is only three characters: '-', 'J' and '\0'

@YaSuenag
Copy link
Member Author

@YaSuenag YaSuenag commented May 16, 2022

Simply considering the compiler warning shouldn't (arg+2) == NULL actually be arg[2] == '\0' ? as we are looking to see if this arg is only three characters: '-', 'J' and '\0'

Yeah, I thought that first, but I think better of changing this code because the result (element of nargv) will be used as loop condition (see description of this PR).

In addition, subsequent codes consider this case (-J only) to be an error as following:

1633     for (i = 0; i < argc; i++) {
1634         char *arg = argv[i];
1635         if (arg[0] == '-' && arg[1] == 'J') {
1636             if (arg[2] == '\0') {
1637                 JLI_ReportErrorMessage(ARG_ERROR3);
1638                 exit(1);
1639             }
1640             *nargv++ = arg + 2;
1641         }
1642     }

However jargv is builtin arguments, not user specified arguments as I wrote the description of this PR. So I think we can ignore it, but it would be nice if we can see it in trace logs.

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented May 16, 2022

I don't recall what "builtin arguments" means. Can you tell me how we would actually hit this conditions such that we string dup and the null string? Or are you saying this is actually an unreachable case: the check for NULL is always false and the actual string is never null anyway?

if (JLI_IsTraceLauncher()) {
printf("Empty option: jargv[%d]\n", i);
}
Copy link
Member

@dholmes-ora dholmes-ora May 16, 2022

Choose a reason for hiding this comment

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

I'd say this is an error as well - but a developer error rather than end-user.

@YaSuenag
Copy link
Member Author

@YaSuenag YaSuenag commented May 17, 2022

@dholmes-ora
As I wrote in the description, jargv comes from JAVA_ARGS or EXTRA_JAVA_ARGS - end user cannot modify them. So I think they should be reported as developer error.
OTOH JDK developer cannot know invalid options if they do not set trace mode to launcher. So I want to discuss how we should handle invalid parameter(s) in jargv.

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented May 17, 2022

You forced me to look at this in depth. The values are set via the build system. In the build system it is impossible to get just -J because the -J is only added as a prefix for an existing string. So basically this is impossible code to reach unless you manually override the build system definition. So as far as I can see this is a classic case where we want an assert.

if (arg[0] == '-' && arg[1] == 'J') {
    assert(arg[2] != '\0', "Invalid JAVA_ARGS or EXTRA_JAVA_ARGS defined by build");
    *nargv++ = JLI_StringDup(arg + 2);
}

@YaSuenag
Copy link
Member Author

@YaSuenag YaSuenag commented May 17, 2022

@dholmes-ora Thanks for your comment!

We cannot use assert(cond, message) at here because it is not HotSpot code. In imageFile.cpp for libjimage. It uses assert(cond && message), so I use this style in new commit, and the warning has gone.

I can change to "normal" assert usage at here if it is strange.

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented May 18, 2022

@YaSuenag I know this is not hotspot code. Please see existing use of assert in related code i.e. src/java.base/share/native/libjli/args.c

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented May 18, 2022

Sorry I see what you mean, it is just an assert(cond) not assert(cond, message), but that is fine.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Looks fine to me.

Thanks.

@@ -1626,7 +1628,8 @@ TranslateApplicationArgs(int jargc, const char **jargv, int *pargc, char ***parg
for (i = 0; i < jargc; i++) {
const char *arg = jargv[i];
if (arg[0] == '-' && arg[1] == 'J') {
*nargv++ = ((arg + 2) == NULL) ? NULL : JLI_StringDup(arg + 2);
assert(arg[2] != '\0' && "Invalid JAVA_ARGS or EXTRA_JAVA_ARGS defined by build");
Copy link
Member

@dholmes-ora dholmes-ora May 18, 2022

Choose a reason for hiding this comment

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

Interesting trick.

Copy link
Member

@tstuefe tstuefe May 18, 2022

Choose a reason for hiding this comment

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

Since we pass NDEBUG, this assert is disabled in release builds. This is the supposed behavior, right? That we don't do anything on release builds?

Copy link
Member

@dholmes-ora dholmes-ora May 18, 2022

Choose a reason for hiding this comment

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

Right - that is what I would expect from an assert.

@openjdk
Copy link

@openjdk openjdk bot commented May 18, 2022

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

8286694: Incorrect argument processing in java launcher

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

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 label May 18, 2022
@YaSuenag
Copy link
Member Author

@YaSuenag YaSuenag commented May 19, 2022

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented May 19, 2022

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

  • 2a2d54e: 8286984: (ch) Problem list java/nio/channels/FileChannel/LargeMapTest.java on Windows
  • a617709: 8281268: Resolve duplication of test ClassTransformer class
  • b523c88: 8286122: [macos]: App bundle cannot upload to Mac App Store due to info.plist embedded in java exe
  • 8323787: 8255439: System Tray icons get corrupted when windows scaling changes
  • cd5bfe7: 8286814: Shenandoah: RedefineRunningMethods.java test failed with Loom
  • b5a3d28: 8285097: Duplicate XML keys in XPATHErrorResources.java and XSLTErrorResources.java
  • 6b9c152: 8286366: (cs) Charset.put can use putIfAbsent instead of containsKey+put
  • 9becf7d: 8283705: Make javax.sound.midi.Track a final class
  • a03438c: 8285397: JNI exception pending in CUPSfuncs.c:250
  • 9ab29b6: 8286869: unify os::dir_is_empty across posix platforms
  • ... and 27 more: https://git.openjdk.java.net/jdk/compare/e68024c2d28d634ebfde7f2fdcc35f5d7b07d704...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated label May 19, 2022
@openjdk openjdk bot closed this May 19, 2022
@openjdk openjdk bot removed ready rfr labels May 19, 2022
@openjdk
Copy link

@openjdk openjdk bot commented May 19, 2022

@YaSuenag Pushed as commit 26c7c92.

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

@YaSuenag YaSuenag deleted the JDK-8286694 branch May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs integrated
3 participants