RFR: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause [v2]
Jaikiran Pai
jpai at openjdk.java.net
Sun May 15 02:40:39 UTC 2022
On Wed, 11 May 2022 15:45:40 GMT, Christoph Langer <clanger at openjdk.org> wrote:
>> After https://bugs.openjdk.java.net/browse/JDK-8251329, javac throws errors when the classpath
>> contains jar files with . or .. in its name. The error message, however, does not help to find
>> the culprit. This could be improved.
>
> Christoph Langer has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
>
> - Merge branch 'master' into jdk-8286444
> - Separate zips change from this PR
> - JDK-8286444
>
> Improve error message in javac compilation when jar files with . or .. are in classpath
src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java line 567:
> 565: this.fileSystem = jarFSProvider.newFileSystem(archivePath, env);
> 566: } catch (ZipException ze) {
> 567: throw new IOException("ZipException opening \"" + archivePath + "\": " + ze.getMessage(), ze);
Hello Christoph, I'm sorry I'm a bit late to this discussion. One of the concerns raised in this PR is that we might end up leaking the file path of the jar being reported for error. The original example that you had in the JBS is something like this:
java -cp invalid.jar Hello.java
which after this change prints:
ZipException opening "invalid.jar": ZIP file can't be opened as a file system because an entry has a '.' or '..' element in its name
This is fine, IMO.
However, consider a slightly modified case. Let's consider a case where you have a different `foo.jar` in that current directory and that `foo.jar` has a `Class-Path` entry which looks like:
Manifest-Version: 1.0
Class-Path: invalid.jar
Created-By: 19-internal (N/A)
So `foo.jar` is pointing to `invalid.jar`. Now let's run the following command by passing `foo.jar` as `-cp` value (which then brings in `invalid.jar` through the `Class-Path` entry in the Manifest file):
java -cp foo.jar Hello.java
With this PR, this now returns:
ZipException opening "/private/tmp/8286444/invalid.jar": ZIP file can't be opened as a file system because an entry has a '.' or '..' element in its name
Notice that it has ended up leaking the file path of the jar that was pulled in through the `MANIFEST.MF` file. I think this is something that isn't allowed from a security point of view.
Perhaps an additional change needs to be done on top of this PR to prevent this, something like:
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java
index 1336fb74916..7c40afb62af 100644
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java
@@ -564,7 +564,7 @@ public class JavacFileManager extends BaseFileManager implements StandardJavaFil
try {
this.fileSystem = jarFSProvider.newFileSystem(archivePath, env);
} catch (ZipException ze) {
- throw new IOException("ZipException opening "" + archivePath + "": " + ze.getMessage(), ze);
+ throw new IOException("ZipException opening "" + archivePath.getFileName() + "": " + ze.getMessage(), ze);
}
} else {
this.fileSystem = FileSystems.newFileSystem(archivePath, (ClassLoader)null);
This then just prints the name of the jar in both cases:
java -cp invalid.jar Hello.java
ZipException opening "invalid.jar": ZIP file can't be opened as a file system because an entry has a '.' or '..' element in its name
java -cp foo.jar Hello.java
ZipException opening "invalid.jar": ZIP file can't be opened as a file system because an entry has a '.' or '..' element in its name
-------------
PR: https://git.openjdk.java.net/jdk/pull/8616
More information about the compiler-dev
mailing list