[SUREFIRE-1939] Fix NPE in SystemUtils.toJdkHomeFromJvmExec if java home has 2 components

[CMoH]

Trivial fix for jira issue https://issues.apache.org/jira/browse/SUREFIRE-1939

[CMoH]

The MacOS failure seems unrelated.

[slawekjaranowski]

Do we have unit test for it ... ?
If not please create - it should be easy
Unit test should coverage all cases described in java doc for this method.

[CMoH]

No, I didn't feel the need to add a new unit test since the existing unit test fails with NPE under the described conditions (i.e JAVA_HOME set to a directory 2 levels up from root, /opt/openjdk-bin-11.0.11_p9 in my case).

However, I have added a system-independent unit test to showcase the situation.

[CMoH]

I feel the fix is more complete now. Thanks for the comment

[rmannibucau]

is jvmExecutable set to /opt/jdk instead of /opt/jdk/bin/java in your case? this test is written to make jdk=/opt and jre=/opt/jdk/ which I can understand you want a case with a single segment path but is not aligned on the description and only possible if the jdk is extracted at the root of the filesystem, is it your case? Also it means the jvmExecutable is not the executable so something is wrong higher in the processing and must be fixed preventing this NPE later so I would rather chase this cause instead of swallowing it.

Any inputs on these points?

[CMoH]

Well, the reason I submitted this is because unit tests fail, due to this: on my machine JAVA_HOME is /opt/openjdk-bin-11/. Unit test incorrectJdkPath() works as follows:

        @Test
        public void incorrectJdkPath()
        {
            File jre = new File( System.getProperty( "java.home" ) );
            File jdk = jre.getParentFile();
            File incorrect = jdk.getParentFile();
            assertThat( SystemUtils.isJava9AtLeast( incorrect.getAbsolutePath() ) ).isFalse();
        }

This leads to incorrect=/, which reaches toJdkHomeFromJvmExec("/"), which calls again getParent("/"), which yields null, and next getName() is called on this null. It is clear from the implementation of toJdkHomeFromJvmExec() that returning null is fine when this jvmExec is not found. I believe this is the case here.

Also, the test is about incorrect JDK paths, and such a path is an incorrect JDK path. However, one incorrect case crashes with a non-informative NPE. I would say a "cannot find jvm executable" error on the return-null path would be more helpful for users that misconfigured their systems.

[Tibor17]

@CMoH
We can accept this fix for the unit test but more important would be to back track the callers in AbstractSurefireMojo and see if this NPE may happen and may have some influence, and where if any.

[andpab]

I've backtracked the production scope callers of the SystemUtils method and the parameter can come from three possible places:

1. <jvm> mojo property
2. System.getProperty( "java.home" ) + File.separator + "bin" + File.separator + "java"
3. JavaToolChain installFolder + File.separator + "bin" + File.separator + "java"

The only one that could in theory lead to the NPE is the <jvm> property - but only if a user enters / or C:\ there. Not an actual use case.

Conclusion: This is not an actual problem in production.

[andpab]

I think the confusion here stems from the fact that the original test incorrectJdkPath() is not well-written.
It has two main problems:

1. It makes some assumptions about the system configuration that are not marked as such using JUnit assumptions.
2. It passes a directory to the SystemUtils#isJava9AtLeast method even though that method expects a Java executable as a parameter. That could be a coding error or it could be deliberate, no way to tell really.

To explain what the original test case does: The test assumes that java.home points to a jre subdirectory of a jdk and then tests that passing the parent directory of the jdk to the method under test returns false.

Not entirely sure why this particular scenario is tested. Sort of a negative test case to check there are no false positive results with slight deviations from correct input probably.

Since (2) could as well be deliberate I would suggest only fixing (1) by adding an assumption, for example like this:

        @Test
        public void incorrectJdkPath()
        {
            File jre = new File( System.getProperty( "java.home" ) );
            assumeTrue( "jre".equals( jre.getName() )
                && (new File(jre.getParentFile(), "bin" + separator + "javac").exists()
                    || new File(jre.getParentFile(), "bin" + separator + "javac.exe").exists()) );
            File jdk = jre.getParentFile();
            File incorrect = jdk.getParentFile();
            assertThat( SystemUtils.isJava9AtLeast( incorrect.getAbsolutePath() ) ).isFalse();
        }

The assumeTrue results in the test being skipped if java home points to a JRE that is not a subdir of a JDK - because clearly in that scenario the setup does not comply with what the test wants to test.

If one is convinced that (2) is a coding error rather than a deliberate choice, one could replace the second-to-last line with something like this:

            File incorrect = new File( jdk.getParentFile(), "not_java.exe" );

[andpab]

The jdk variable points to /opt. Why is it named jdk? It's only named this way because the setup was copied from the other test without understanding what that other test was trying to achieve - which is kinda understandable since it's a bit cryptic. But we should not make the whole thing even more cryptic by copying bad naming this way.

The whole setup (jre variable and jdk variable) is actually irrelevant for this new test. The only relevant variable here is incorrect which is always pointing to /. So why not simplify this test?

I would write the whole test as a one-liner and name it something like shouldFindNoJava9_WhenGivenFileSystemRootPath():

assertThat( SystemUtils.isJava9AtLeast( new File( "/" ) ) ).isFalse();

[CMoH]

Well, as far as the new test goes, see #387 (comment). Feel free to remove the extra test.

The reason for submitting this is because I had another issue, but after the initial clone of this plugin I got a NPE at the first build. I hoped fixing this would help others. From my perspective, this null-check is both conservative and more than insignificant, in short not worthy of this elaborate debate about it.

[andpab]

I think the confusion here stems from the fact that the original test incorrectJdkPath() is not well-written.
It has two main problems:

1. It makes some assumptions about the system configuration that are not marked as such using JUnit assumptions.
2. It passes a directory to the SystemUtils#isJava9AtLeast method even though that method expects a Java executable as a parameter. That could be a coding error or it could be deliberate, no way to tell really.

To explain what the original test case does: The test assumes that java.home points to a jre subdirectory of a jdk and then tests that passing the parent directory of the jdk to the method under test returns false.

Not entirely sure why this particular scenario is tested. Sort of a negative test case to check there are no false positive results with slight deviations from correct input probably.

Since (2) could as well be deliberate I would suggest only fixing (1) by adding an assumption, for example like this:

        @Test
        public void incorrectJdkPath()
        {
            File jre = new File( System.getProperty( "java.home" ) );
            assumeTrue( "jre".equals( jre.getName() )
                && (new File(jre.getParentFile(), "bin" + separator + "javac").exists()
                    || new File(jre.getParentFile(), "bin" + separator + "javac.exe").exists()) );
            File jdk = jre.getParentFile();
            File incorrect = jdk.getParentFile();
            assertThat( SystemUtils.isJava9AtLeast( incorrect.getAbsolutePath() ) ).isFalse();
        }

The assumeTrue results in the test being skipped if java home points to a JRE that is not a subdir of a JDK - because clearly in that scenario the setup does not comply with what the test wants to test.

If one is convinced that (2) is a coding error rather than a deliberate choice, one could replace the second-to-last line with something like this:

            File incorrect = new File( jdk.getParentFile(), "not_java.exe" );

[andpab]

@CMoH are you available to make the requested changes to the tests? Otherwise I can also make a new pull request.

[CMoH]

@CMoH are you available to make the requested changes to the tests? Otherwise I can also make a new pull request.

Well, this may silently disable the test without notice due to OS setup. However, as an newcomer to this project, I do not understand the established reasons behind existing tests, the policy on changing them or how deep ideas are discussed for each change. Please feel free to pursue this to your own liking.

[michael-o]

I have created a new PR addressing this: #743. @CMoH, your approach is appreciated, but it solves only the symptom, not the cause. Look at the signature:

maven-surefire/surefire-booter/src/main/java/org/apache/maven/surefire/booter/SystemUtils.java

Line 148 in e166d93

public static boolean isJava9AtLeast(String jvmExecutablePath) {

. It expects a path to the Java executable and nothing else, not a JDK/JRE/Java home. Therefore, invalid data leads to undefined behavior, you see an NPE in this case.

FTR: Ran the entire suite on HP-UX with default /opt/java11, all fine. The test is just broken for me.

[michael-o]

Superseded by #743.