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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion surefire-booter/src/main/java/org/apache/maven/surefire/booter/SystemUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ public static boolean endsWithJavaPath( String jvmExecPath )
public static File toJdkHomeFromJvmExec( String jvmExecutable )
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

{
File bin = new File( jvmExecutable ).getAbsoluteFile().getParentFile();
if ( "bin".equals( bin.getName() ) )
if ( bin != null && "bin".equals( bin.getName() ) )
{
File parent = bin.getParentFile();
if ( "jre".equals( parent.getName() ) )
Expand Down
9 changes: 9 additions & 0 deletions surefire-booter/src/test/java/org/apache/maven/surefire/booter/SystemUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,15 @@ public void incorrectJdkPath()
assertThat( SystemUtils.isJava9AtLeast( incorrect.getAbsolutePath() ) ).isFalse();
}

@Test
public void incorrectJdkPathShouldNotNPE()
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 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" );

{
File jre = new File( "/opt/jdk" );
File jdk = jre.getParentFile();
Copy link
Contributor

Choose a reason for hiding this comment

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

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();

Copy link
Author

Choose a reason for hiding this comment

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

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.

File incorrect = jdk.getParentFile();
assertThat( SystemUtils.isJava9AtLeast( incorrect.getAbsolutePath() ) ).isFalse();
}

@Test
public void shouldHaveJavaPath()
{
Expand Down