[PATCH] 8218268: Javac treats Manifest Class-Path entries as Paths instead of URLs
a Hi All, This is my first contribution to openjdk, so hope to learn a lot! I first posted this to compiler-dev, but they explained that I better can discuss this first on this list. I created a fix + tests for https://bugs.openjdk.java.net/browse/JDK-8218268 (I will rewrite the test to java using langtools, but added them below to get an idea what they do). Now the manifest classpath is behaving the same in javac and java for file paths as it was in java 8. See at the end of this mail the patch. So this fixes: 1. The Java Compiler treats the entries as URLs, according to spec: https://docs.oracle.com/en/java/javase/11/docs/specs/jar/jar.html#class-path... 2. The behavior of the JVM ClassLoader and the Java Compiler is equal (for file urls) 3. The behavior is backwards compatible (it was broken in javac 9-13) Now I understood from these links that class-path attribute has become more strict, and fallback to old behaviour is provided: [1] https://bugs.openjdk.java.net/browse/JDK-8217215 [2] https://bugs.openjdk.java.net/browse/JDK-8216401 Question 1: Where can I find the tests for these changes? These fixes seems already been committed, but javac has not been taken along. In my opinion it would be best if the new behaviour can be used both in javac and java (maybe with same properties). Question 2: Do you agree? And if so where should this code be located? (I am not in java 11 modules yet) Now is 8218268 about breaking of manifest classpath attribute in javac between java 8 => 9. I would like that this is asap fixed in e.g. java 11 because I depend on this behaviour. Question 3: Is it an idea to first implement below patch and backport that, and in addition as new ticket implement the new behaviour also for javac? The current patch contains a System.err because I do not know how to (debug) log information in the jdk. I saw for java they used a property and then write to System.err. Question 4:Is this they way to do it, or is there a better way to provide debug information to users running javac? Kind Regards, Donald Kwakkel ps: Looking forward for a sponsor to adopt/work on my change! diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/file/FSInfo.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/file/FSInfo.java --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/file/FSInfo.java +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/file/FSInfo.java @@ -26,7 +26,10 @@ package com.sun.tools.javac.file; import java.io.IOException; -import java.nio.file.FileSystems; +import java.net.MalformedURLException; +import java.net.URI; +import java.net.URISyntaxException; +import java.net.URL; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.spi.FileSystemProvider; @@ -90,7 +93,6 @@ } public List<Path> getJarClassPath(Path file) throws IOException { - Path parent = file.getParent(); try (JarFile jarFile = new JarFile(file.toFile())) { Manifest man = jarFile.getManifest(); if (man == null) @@ -100,22 +102,27 @@ if (attr == null) return Collections.emptyList(); - String path = attr.getValue(Attributes.Name.CLASS_PATH); - if (path == null) + String classpath = attr.getValue(Attributes.Name.CLASS_PATH); + if (classpath == null) return Collections.emptyList(); - List<Path> list = new ArrayList<>(); + // TODO: Must use the same code as jdk.internal.loader.URLClassPath.JarLoader.parseClassPath(base, path) + StringTokenizer st = new StringTokenizer(classpath); + List<Path> paths = new ArrayList<>(st.countTokens()); + while (st.hasMoreTokens()) { + String path = st.nextToken(); + try { + // Use getCanonicalFile just as jdk.internal.loader.URLClassPath.toFileURL + URL base = file.toFile().getCanonicalFile().toURI().toURL(); + URI uri = new URL(base, path).toURI(); - for (StringTokenizer st = new StringTokenizer(path); - st.hasMoreTokens(); ) { - String elt = st.nextToken(); - Path f = FileSystems.getDefault().getPath(elt); - if (!f.isAbsolute() && parent != null) - f = parent.resolve(f).toAbsolutePath(); - list.add(f); + // Should return uri, see comment on com.sun.tools.javac.file.Locations.SearchPath.addJarClassPath + paths.add(Path.of(uri)); + } catch (MalformedURLException | URISyntaxException e) { + System.err.println("Class-Path entry: \"" + path + "\" ignored in JAR file \"" + file + "\": " + e.getMessage()); + } } - - return list; + return paths; } } diff --git a/test/langtools/tools/javac/Paths/UrlClassPath.sh b/test/langtools/tools/javac/Paths/UrlClassPath.sh new file mode 100755 --- /dev/null +++ b/test/langtools/tools/javac/Paths/UrlClassPath.sh @@ -0,0 +1,112 @@ +#!/bin/sh +# +# Copyright (c) 2011, Oracle and/or its affiliates. All rights reserved. +# DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. +# +# This code is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License version 2 only, as +# published by the Free Software Foundation. +# +# This code is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License +# version 2 for more details (a copy is included in the LICENSE file that +# accompanied this code). +# +# You should have received a copy of the GNU General Public License version +# 2 along with this work; if not, write to the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. +# +# Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA +# or visit www.oracle.com if you need additional information or have any +# questions. +# + +# @test +# @bug 8218268 +# @summary Test handling of urls (with spaces) in the Class-Path attribute in jar file manifests +# @author Donald Kwakkel +# +# @run shell UrlClassPath.sh + +# To run this test manually, simply do ./UrlClassPath.sh + +. ${TESTSRC-.}/Util.sh + +set -u + +Cleanup() { + Sys rm -rf "x y" z Hello.java Hello.class Main.java Main.class MANIFEST.MF hellocp.jar +} + +Cleanup +Sys mkdir "x y" + +#---------------------------------------------------------------- +# Create mutually referential jar files +#---------------------------------------------------------------- +cat >Hello.java <<EOF +public class Hello { + public static String hello() { + return "hello"; + } +} +EOF + +cat >Main.java <<EOF +public class Main { + public static void main(String[] args) { + System.out.println(Hello.hello()); + } +} +EOF + +Sys "$javac" Hello.java +Sys "$jar" cf "x y/hello.jar" Hello.class +Sys rm -rf Hello.class Hello.java + +createManifestJar() { + MkManifestWithClassPath "${1}" + Sys "$jar" cmf MANIFEST.MF "${2}" +} + +runJava() { + Success "$javac" ${TESTTOOLVMOPTS} -cp ".:${1}" Main.java + # Bug: jdk.net.URLClassPath.disableClassPathURLCheck meaning is inverted and should be default enabled according to spec + Success "$java" ${TESTVMOPTS} -Djdk.net.URLClassPath.disableClassPathURLCheck=false -cp ".:${1}" Main +} + +doTest() { + createManifestJar "${1}" hellocp.jar + runJava "${2}" +} + +# Test 1: Compiling and run with directly adding classpath with space +doTest "x%20y/hello.jar" "x y/hello.jar" + +# Test 2: Compiling and run with adding classpath with space via manifest +doTest "x%20y/hello.jar" "hellocp.jar" + +# Test 3: classpath with file url path +doTest "file:"`pwd`"/x%20y/hello.jar" "hellocp.jar" + +# Test 4: classpath with file uri path +doTest "file://"`pwd`"/x%20y/hello.jar" "hellocp.jar" + +# Test 5: garbage before, right jar must still be used +doTest "notexisting://garbage x%20y/hello.jar" "hellocp.jar" + +# Test 6: linked jar +Sys mkdir "z" +createManifestJar "x%20y/hello.jar" hellocp.jar +Sys ln -s ../hellocp.jar z/hellocp.jar +runJava "z/hellocp.jar" + +# Test 7: only garbage, should fail +createManifestJar "notexisting://garbage" hellocp.jar +Failure "$javac" ${TESTTOOLVMOPTS} -cp ".:hellocp.jar" Main.java +Failure "$java" ${TESTVMOPTS} -cp ".:hellocp.jar" Main.java + +Cleanup + +Bottom Line
Looking at the revised JAR specification, approved in [1], it is disappointing that the spec contains text which is specific to a JAR file being used in a classloader: |The resulting URLs are inserted into the search path of the class loader opening the context JAR immediately following the path of the context JAR. Any duplicated URLs are omitted.| That text would seem not to apply to javac and other tools that may wish to process the class files in a JAR file. -- Jon https://bugs.openjdk.java.net/browse/JDK-8217215 On 02/28/2019 08:41 AM, Donald Kwakkel wrote:
a Hi All,
This is my first contribution to openjdk, so hope to learn a lot!
I first posted this to compiler-dev, but they explained that I better can discuss this first on this list.
I created a fix + tests for https://bugs.openjdk.java.net/browse/JDK-8218268 (I will rewrite the test to java using langtools, but added them below to get an idea what they do). Now the manifest classpath is behaving the same in javac and java for file paths as it was in java 8. See at the end of this mail the patch.
So this fixes: 1. The Java Compiler treats the entries as URLs, according to spec: https://docs.oracle.com/en/java/javase/11/docs/specs/jar/jar.html#class-path... 2. The behavior of the JVM ClassLoader and the Java Compiler is equal (for file urls) 3. The behavior is backwards compatible (it was broken in javac 9-13)
Now I understood from these links that class-path attribute has become more strict, and fallback to old behaviour is provided: [1] https://bugs.openjdk.java.net/browse/JDK-8217215 [2] https://bugs.openjdk.java.net/browse/JDK-8216401
Question 1: Where can I find the tests for these changes?
These fixes seems already been committed, but javac has not been taken along. In my opinion it would be best if the new behaviour can be used both in javac and java (maybe with same properties).
Question 2: Do you agree? And if so where should this code be located? (I am not in java 11 modules yet)
Now is 8218268 about breaking of manifest classpath attribute in javac between java 8 => 9. I would like that this is asap fixed in e.g. java 11 because I depend on this behaviour.
Question 3: Is it an idea to first implement below patch and backport that, and in addition as new ticket implement the new behaviour also for javac?
The current patch contains a System.err because I do not know how to (debug) log information in the jdk. I saw for java they used a property and then write to System.err.
Question 4:Is this they way to do it, or is there a better way to provide debug information to users running javac?
Kind Regards, Donald Kwakkel
ps: Looking forward for a sponsor to adopt/work on my change!
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/file/FSInfo.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/file/FSInfo.java --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/file/FSInfo.java +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/file/FSInfo.java @@ -26,7 +26,10 @@ package com.sun.tools.javac.file;
import java.io.IOException; -import java.nio.file.FileSystems; +import java.net.MalformedURLException; +import java.net.URI; +import java.net.URISyntaxException; +import java.net.URL; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.spi.FileSystemProvider; @@ -90,7 +93,6 @@ }
public List<Path> getJarClassPath(Path file) throws IOException { - Path parent = file.getParent(); try (JarFile jarFile = new JarFile(file.toFile())) { Manifest man = jarFile.getManifest(); if (man == null) @@ -100,22 +102,27 @@ if (attr == null) return Collections.emptyList();
- String path = attr.getValue(Attributes.Name.CLASS_PATH); - if (path == null) + String classpath = attr.getValue(Attributes.Name.CLASS_PATH); + if (classpath == null) return Collections.emptyList();
- List<Path> list = new ArrayList<>(); + // TODO: Must use the same code as jdk.internal.loader.URLClassPath.JarLoader.parseClassPath(base, path) + StringTokenizer st = new StringTokenizer(classpath); + List<Path> paths = new ArrayList<>(st.countTokens()); + while (st.hasMoreTokens()) { + String path = st.nextToken(); + try { + // Use getCanonicalFile just as jdk.internal.loader.URLClassPath.toFileURL + URL base = file.toFile().getCanonicalFile().toURI().toURL(); + URI uri = new URL(base, path).toURI();
- for (StringTokenizer st = new StringTokenizer(path); - st.hasMoreTokens(); ) { - String elt = st.nextToken(); - Path f = FileSystems.getDefault().getPath(elt); - if (!f.isAbsolute() && parent != null) - f = parent.resolve(f).toAbsolutePath(); - list.add(f); + // Should return uri, see comment on com.sun.tools.javac.file.Locations.SearchPath.addJarClassPath + paths.add(Path.of(uri)); + } catch (MalformedURLException | URISyntaxException e) { + System.err.println("Class-Path entry: \"" + path + "\" ignored in JAR file \"" + file + "\": " + e.getMessage()); + } } - - return list; + return paths; } }
diff --git a/test/langtools/tools/javac/Paths/UrlClassPath.sh b/test/langtools/tools/javac/Paths/UrlClassPath.sh new file mode 100755 --- /dev/null +++ b/test/langtools/tools/javac/Paths/UrlClassPath.sh @@ -0,0 +1,112 @@ +#!/bin/sh +# +# Copyright (c) 2011, Oracle and/or its affiliates. All rights reserved. +# DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. +# +# This code is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License version 2 only, as +# published by the Free Software Foundation. +# +# This code is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License +# version 2 for more details (a copy is included in the LICENSE file that +# accompanied this code). +# +# You should have received a copy of the GNU General Public License version +# 2 along with this work; if not, write to the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. +# +# Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA +# or visit www.oracle.com if you need additional information or have any +# questions. +# + +# @test +# @bug 8218268 +# @summary Test handling of urls (with spaces) in the Class-Path attribute in jar file manifests +# @author Donald Kwakkel +# +# @run shell UrlClassPath.sh + +# To run this test manually, simply do ./UrlClassPath.sh + +. ${TESTSRC-.}/Util.sh + +set -u + +Cleanup() { + Sys rm -rf "x y" z Hello.java Hello.class Main.java Main.class MANIFEST.MF hellocp.jar +} + +Cleanup +Sys mkdir "x y" + +#---------------------------------------------------------------- +# Create mutually referential jar files +#---------------------------------------------------------------- +cat >Hello.java <<EOF +public class Hello { + public static String hello() { + return "hello"; + } +} +EOF + +cat >Main.java <<EOF +public class Main { + public static void main(String[] args) { + System.out.println(Hello.hello()); + } +} +EOF + +Sys "$javac" Hello.java +Sys "$jar" cf "x y/hello.jar" Hello.class +Sys rm -rf Hello.class Hello.java + +createManifestJar() { + MkManifestWithClassPath "${1}" + Sys "$jar" cmf MANIFEST.MF "${2}" +} + +runJava() { + Success "$javac" ${TESTTOOLVMOPTS} -cp ".:${1}" Main.java + # Bug: jdk.net.URLClassPath.disableClassPathURLCheck meaning is inverted and should be default enabled according to spec + Success "$java" ${TESTVMOPTS} -Djdk.net.URLClassPath.disableClassPathURLCheck=false -cp ".:${1}" Main +} + +doTest() { + createManifestJar "${1}" hellocp.jar + runJava "${2}" +} + +# Test 1: Compiling and run with directly adding classpath with space +doTest "x%20y/hello.jar" "x y/hello.jar" + +# Test 2: Compiling and run with adding classpath with space via manifest +doTest "x%20y/hello.jar" "hellocp.jar" + +# Test 3: classpath with file url path +doTest "file:"`pwd`"/x%20y/hello.jar" "hellocp.jar" + +# Test 4: classpath with file uri path +doTest "file://"`pwd`"/x%20y/hello.jar" "hellocp.jar" + +# Test 5: garbage before, right jar must still be used +doTest "notexisting://garbage x%20y/hello.jar" "hellocp.jar" + +# Test 6: linked jar +Sys mkdir "z" +createManifestJar "x%20y/hello.jar" hellocp.jar +Sys ln -s ../hellocp.jar z/hellocp.jar +runJava "z/hellocp.jar" + +# Test 7: only garbage, should fail +createManifestJar "notexisting://garbage" hellocp.jar +Failure "$javac" ${TESTTOOLVMOPTS} -cp ".:hellocp.jar" Main.java +Failure "$java" ${TESTVMOPTS} -cp ".:hellocp.jar" Main.java + +Cleanup + +Bottom Line
On 28/02/2019 20:58, Jonathan Gibbons wrote:
Looking at the revised JAR specification, approved in [1], it is disappointing that the spec contains text which is specific to a JAR file being used in a classloader:
|The resulting URLs are inserted into the search path of the class loader opening the context JAR immediately following the path of the context JAR. Any duplicated URLs are omitted.|
That text would seem not to apply to javac and other tools that may wish to process the class files in a JAR file. That should be fixed as it should apply at compile-time too.
-Alan
On 02/28/2019 01:06 PM, Alan Bateman wrote:
On 28/02/2019 20:58, Jonathan Gibbons wrote:
Looking at the revised JAR specification, approved in [1], it is disappointing that the spec contains text which is specific to a JAR file being used in a classloader:
|The resulting URLs are inserted into the search path of the class loader opening the context JAR immediately following the path of the context JAR. Any duplicated URLs are omitted.|
That text would seem not to apply to javac and other tools that may wish to process the class files in a JAR file. That should be fixed as it should apply at compile-time too.
-Alan
|Agreed it might be good to fix it if possible. All the preceding text is good, and can be handled by javac. The only questionable bit is the text "Any duplicated URLs are omitted" which could be moved up a bit in the spec to a new paragraph, and maybe rephrased to use "ignored" instead of "omitted". If that were done, all the stuff about class loaders could be taken as non-normative text. -- Jon |
Hi, Jon On 2/28/19 1:38 PM, Jonathan Gibbons wrote:
On 28/02/2019 20:58, Jonathan Gibbons wrote:
Looking at the revised JAR specification, approved in [1], it is disappointing that the spec contains text which is specific to a JAR file being used in a classloader:
|The resulting URLs are inserted into the search path of the class loader opening the context JAR immediately following the path of the context JAR. Any duplicated URLs are omitted.|
That text would seem not to apply to javac and other tools that may wish to process the class files in a JAR file.
|Agreed it might be good to fix it if possible. All the preceding text is good, and can be handled by javac. The only questionable bit is the text "Any duplicated URLs are omitted" which could be moved up a bit in the spec to a new paragraph, and maybe rephrased to use "ignored" instead of "omitted". If that were done, all the stuff about class loaders could be taken as non-normative text.
I've updated the spec changes in the CSR[1] to address your concerns. For reference, the new relevant passages are: "Invalid entries are ignored. Valid entries are resolved against the context JAR. If the resulting URL is invalid or refers to a resource that cannot be found, then it is ignored. Duplicate URLs are ignored. The resulting URLs are inserted into the class path, immediately following the URL of the context JAR. For example, given the following class path:" Let me know if this will work for javac, and I will proceed. Thanks, -Brent 1. https://bugs.openjdk.java.net/browse/JDK-8217215
On 02/28/2019 01:06 PM, Alan Bateman wrote:
On 28/02/2019 20:58, Jonathan Gibbons wrote:
Looking at the revised JAR specification, approved in [1], it is disappointing that the spec contains text which is specific to a JAR file being used in a classloader:
|The resulting URLs are inserted into the search path of the class loader opening the context JAR immediately following the path of the context JAR. Any duplicated URLs are omitted.|
That text would seem not to apply to javac and other tools that may wish to process the class files in a JAR file. That should be fixed as it should apply at compile-time too.
-Alan
|Agreed it might be good to fix it if possible. All the preceding text is good, and can be handled by javac. The only questionable bit is the text "Any duplicated URLs are omitted" which could be moved up a bit in the spec to a new paragraph, and maybe rephrased to use "ignored" instead of "omitted". If that were done, all the stuff about class loaders could be taken as non-normative text.
So if I am correct the answer to Question 2 is: Yes, behavior must be the same. What are the next steps to take for this? And can someone also answer my other questions?: Question 1: Where can I find the tests for these changes? Question 2: Where should the common code for this be located? Question 3: Is it an idea to first implement below patch and backport that, and in addition as new ticket implement the new behaviour also for javac? Question 4:Is this they way to do it, or is there a better way to provide debug information to users running javac?
Attached patch with tests so first the bug for java11 can be fixed and backported. Would be nice if someone can guide me how to continue with this and/or can reply on my previous questions. Op di 5 mrt. 2019 om 07:11 schreef Donald Kwakkel <dkwakkel@gmail.com>:
On 02/28/2019 01:06 PM, Alan Bateman wrote:
On 28/02/2019 20:58, Jonathan Gibbons wrote:
Looking at the revised JAR specification, approved in [1], it is disappointing that the spec contains text which is specific to a JAR file being used in a classloader:
|The resulting URLs are inserted into the search path of the class loader opening the context JAR immediately following the path of the context JAR. Any duplicated URLs are omitted.|
That text would seem not to apply to javac and other tools that may wish to process the class files in a JAR file. That should be fixed as it should apply at compile-time too.
-Alan
|Agreed it might be good to fix it if possible. All the preceding text is good, and can be handled by javac. The only questionable bit is the text "Any duplicated URLs are omitted" which could be moved up a bit in the spec to a new paragraph, and maybe rephrased to use "ignored" instead of "omitted". If that were done, all the stuff about class loaders could be taken as non-normative text.
So if I am correct the answer to Question 2 is: Yes, behavior must be the same.
What are the next steps to take for this? And can someone also answer my other questions?:
Question 1: Where can I find the tests for these changes? Question 2: Where should the common code for this be located? Question 3: Is it an idea to first implement below patch and backport that, and in addition as new ticket implement the new behaviour also for javac? Question 4:Is this they way to do it, or is there a better way to provide debug information to users running javac?
Donald, While it was reasonable to raise the spec issues on core-libs-dev, any/all questions about javac code should be addressed to compiler-dev@openjdk.java.net. That being said, * any comment labelled "// TODO" in new code seems questionable * the copyright date on the test is wrong and should probably be 2019 * the test uses tabs instead of spaces * direct writes to System.err are probably wrong -- Jon On 03/13/2019 02:38 PM, Donald Kwakkel wrote:
Attached patch with tests so first the bug for java11 can be fixed and backported. Would be nice if someone can guide me how to continue with this and/or can reply on my previous questions.
Op di 5 mrt. 2019 om 07:11 schreef Donald Kwakkel <dkwakkel@gmail.com>:
On 02/28/2019 01:06 PM, Alan Bateman wrote:
On 28/02/2019 20:58, Jonathan Gibbons wrote:
Looking at the revised JAR specification, approved in [1], it is disappointing that the spec contains text which is specific to a JAR file being used in a classloader:
|The resulting URLs are inserted into the search path of the class loader opening the context JAR immediately following the path of the context JAR. Any duplicated URLs are omitted.|
That text would seem not to apply to javac and other tools that may wish to process the class files in a JAR file. That should be fixed as it should apply at compile-time too.
-Alan |Agreed it might be good to fix it if possible. All the preceding text is good, and can be handled by javac. The only questionable bit is the text "Any duplicated URLs are omitted" which could be moved up a bit in the spec to a new paragraph, and maybe rephrased to use "ignored" instead of "omitted". If that were done, all the stuff about class loaders could be taken as non-normative text. So if I am correct the answer to Question 2 is: Yes, behavior must be the same.
What are the next steps to take for this? And can someone also answer my other questions?:
Question 1: Where can I find the tests for these changes? Question 2: Where should the common code for this be located? Question 3: Is it an idea to first implement below patch and backport that, and in addition as new ticket implement the new behaviour also for javac? Question 4:Is this they way to do it, or is there a better way to provide debug information to users running javac?
participants (4)
-
Alan Bateman
-
Brent Christian
-
Donald Kwakkel
-
Jonathan Gibbons