Hi Chris,
Here is a new patch containing new version of the tests. The new versions
generate a the Jar on the fly as we discussed.
Let me know if there is anything else you think should be improved.
Best,
Diego
On Thu, Jun 14, 2012 at 7:32 AM, Chris Hegarty <chris.hega...@oracle.com>wrote:
> Diego,
>
> It's not too difficult to create jars on the fly using the Jar API. Here
> is a small example that I think would work nice in this case. Files created
> ( and paths are relative to the jtreg scratch, or working dir if running
> outside of jtreg ).
>
> Do you think you could use similar to create the jars for your test?
>
> createJar("a.jar", jarAList);
> createJar("b.jar", jarBList);
> .......
>
> static void createJar(String jarName, Map<String,String> contents)
> throws Exception
> {
> try (FileOutputStream aJar = new FileOutputStream(jarName);
> JarOutputStream jos = new JarOutputStream(aJar)) {
> Set<Entry<String,String>> entries = contents.entrySet();
> for (Entry<String,String> entry : entries)
> writeJarEntry(jos, entry.getKey(),
> entry.getValue().getBytes("**ASCII"));
> }
> }
>
> static void writeJarEntry(JarOutputStream jos, String name, byte[] data)
> throws Exception
> {
> JarEntry entry = new JarEntry(name);
> jos.putNextEntry(entry);
> jos.write(data);
> }
>
> static final Map<String,String> jarAList = new HashMap<>();
> static final Map<String,String> jarBList = new HashMap<>();
> static {
> jarAList.put("com/foo/**resource1.txt", "some random data");
> jarAList.put("com/bar/**resource2.txt", "some more random data!");
> jarAList.put("com/baz/**resource3.txt", "even more random
> data!!!");
> jarBList.put("x/y/resourceA.**dat", "Hello there");
> jarBList.put("x/y/resourceB.**dat", "Goodbye");
> jarBList.put("x/y/resourceC.**dat", "Hello\nfrom\nb\ndot\njar");
> }
>
> Thanks,
> -Chris.
>
>
> On 14/06/2012 03:20, Diego Belfer wrote:
>
>> Hi Chris,
>>
>> There is no way to generate a jar without directory entries using the
>> jar tool; there is no option for that. What do you think is the best way
>> to handle this ?
>> I don't want to re-implement the logic for creating a jar using the
>> JarOutputStream class.
>>
>> Do you think it is possible to add a new option to the Jar tool Main
>> class to exclude directory entries? The option does not need to be
>> exposed by the command line tool to final users if this an issue,
>> although I think it may be useful for them too.
>>
>> Best,
>> Diego
>>
>>
>> On Wed, Jun 13, 2012 at 7:12 PM, Diego Belfer <dbel...@gmail.com
>> <mailto:dbel...@gmail.com>> wrote:
>>
>> Chris,
>>
>> I was thinking of something similar. I will create the jars and
>> their contents using Java code. There is no need of creating real
>> class files, using a few resource files and some directories will be
>> enough.
>>
>> Best,
>> Diego
>>
>>
>> On Wed, Jun 13, 2012 at 6:46 PM, chris hegarty
>> <chris.hega...@oracle.com
>> <mailto:chris.hegarty@oracle.**com<chris.hega...@oracle.com>>>
>> wrote:
>>
>> Diego,
>>
>> I'm thinking that some of the trivial source files, to compile
>> and built into the jars, could be simply created and written by
>> the test itself, rather than checking them all in. If this makes
>> it cleaner. I really don't like all the file in
>> test/sun/misc/JarIndex/__**metaInfFilenames, but at least it is
>>
>> quite understandable.
>>
>> -Chris.
>>
>>
>> On 13/06/2012 20:36, Diego Belfer wrote:
>>
>> Hi Chris,
>>
>> That's right. The only non-cleanup change is the one in the
>> merge.
>>
>> Regarding the test case, I will re-write them in order to
>> generate the
>> jars on fly. I'd scanned the jdk/test folder and found a few
>> jars,
>> that's why I included them. I have seen your test case, I
>> will use it
>> as a sample.
>>
>> I had not seen your comment in the bug report. Maybe there
>> are other
>> cases which trigger the InvalidJarIndexException, but, as
>> far as I could
>> see, the validIndex method checks that at least one entry of
>> the jar
>> matches the target path found in the index. If directory
>> entries are not
>> present in the jar, stripped paths generated during the
>> merge and used
>> by the index will return jars which may not contain entries
>> for them,
>> triggering the exception.
>> When all directory entries are present, if a jar contains an
>> entry for
>> "xxx/yyy/resource.file", it will contain entries for "xxx",
>> "xxx/yyy"
>> and "xxx/yyy/resource.file".
>>
>>
>> Best,
>> Diego
>>
>>
>> On Wed, Jun 13, 2012 at 12:05 PM, Chris Hegarty
>> <chris.hega...@oracle.com
>> <mailto:chris.hegarty@oracle.**com<chris.hega...@oracle.com>
>> >
>> <mailto:chris.hegarty@oracle._**_com
>>
>> <mailto:chris.hegarty@oracle.**com <chris.hega...@oracle.com>>>>
>> wrote:
>>
>> Hi Diego,
>>
>> Thanks for picking up this bug.
>>
>> I think your changes look fine. Mainly cleanup except
>> for add ->
>> addExplicit/addMapping in merge, right? BTW the cleanup
>> makes this
>> more readable.
>>
>> Unfortunately, the tests you created require checking in
>> a binary
>> jar file. This is a real no no for the OpenJDK, we
>> really need to
>> create these jars on the fly. I did similar for
>> test/sun/misc/JarIndex/____**metaInfFilenames/, but I
>>
>> really wish I
>>
>> generated the source files for these tests rather than
>> checking in
>> so many pointless files.
>>
>> I can look at helping with writing suitable tests for this.
>>
>>
>> > That's because I was using jars containing "directory
>> entries"
>> > (I was unaware that jar files may not include them)
>>
>> Strangely I added the comment "Remove directories from
>> jar files
>> being indexed." to the workaround section of the bug.
>> You seem to be
>> seeing the opposite, right?
>>
>> -Chris.
>>
>>
>>
>> On 13/06/2012 06:11, Diego Belfer wrote:
>>
>> Hi,
>>
>> I have finally reproduced the
>> InvalidJarIndexException bug as
>> reported in
>> the ticket. I mentioned in a previous email, that
>> the only way
>> I'd found
>> for getting the error was to use an invalid index file
>> (INDEX.LIST), which
>> did not have any sense. That's because I was using
>> jars containing
>> "directory entries" (I was unaware that jar files may not
>> include them)
>>
>> After reviewing the URLClasspath$JarLoader class and
>> the
>> validIndex method,
>> I notice it is possible to get the exception for a
>> Jar file
>> which does not
>> include directory entries. In order to trigger the
>> issue, the
>> Jar must be
>> referenced by an intermediary INDEX.LIST and the
>> intermediary
>> Jar index
>> should have been merged to its parent index.
>> Although, jar tool
>> includes
>> directory entries in the generated jar files,
>> Eclipse default
>> option for
>> exporting jars does not include them (AFAIK), so
>> this might be
>> quite common.
>>
>> I have created a new PATCH which includes an
>> additional test
>> case which
>> uses the URLClassLoader to trigger the
>> InvalidIndexException.
>>
>> The patch is attached, please consider it for review.
>>
>> Best,
>> Diego Belfer [muralx]
>>
>>
>>
>> On Mon, Jun 11, 2012 at 4:47 PM, Diego
>> Belfer<dbel...@gmail.com <mailto:dbel...@gmail.com>
>> <mailto:dbel...@gmail.com <mailto:dbel...@gmail.com>>> wrote:
>>
>>
>>
>> Hi,
>>
>> Here is a patch that fixes the merge method of
>> the JarIndex.
>> This bug was
>> reported as the cause of the bug 6901992.
>> Although, I was
>> not able to
>> reproduce the BUG itself
>> (InvalidJarIndexException), I did
>> verified that
>> the method had a bug, and resources/classes
>> where not found
>> in a jarIndex
>> with merged contents.
>>
>> If you think it is possible to commit this fix
>> without actually
>> reproducing the original bug report, please
>> consider this
>> patch for review.
>>
>> Thanks,
>> Diego Belfer [muralx]
>>
>>
>>
>>
>>
>>
# HG changeset patch
# User muralx
# Date 1339820030 10800
# Node ID 7b531b09855a099fe0e9ea06ed7f77772cb62ed0
# Parent fc575c78f5d314fd8ccbdc86c8b2d7631d736960
[PATCH] 6901992 - Possible InvalidJarIndexException due to bug in
sun.misc.JarIndex.merge()
diff --git a/.hgignore b/.hgignore
--- a/.hgignore
+++ b/.hgignore
@@ -5,3 +5,4 @@
^make/netbeans/.*/dist/
^.hgtip
.DS_Store
+\.class$
diff --git a/src/share/classes/sun/misc/JarIndex.java
b/src/share/classes/sun/misc/JarIndex.java
--- a/src/share/classes/sun/misc/JarIndex.java
+++ b/src/share/classes/sun/misc/JarIndex.java
@@ -201,23 +201,20 @@
packageName = fileName;
}
- // add the mapping to indexMap
- addToList(packageName, jarName, indexMap);
-
- // add the mapping to jarMap
- addToList(jarName, packageName, jarMap);
+ addMapping(packageName, jarName);
}
/**
* Same as add(String,String) except that it doesn't strip off from the
- * last index of '/'. It just adds the filename.
+ * last index of '/'. It just adds the jarItem (filename or package)
+ * as it is received.
*/
- private void addExplicit(String fileName, String jarName) {
+ private void addMapping(String jarItem, String jarName) {
// add the mapping to indexMap
- addToList(fileName, jarName, indexMap);
+ addToList(jarItem, jarName, indexMap);
// add the mapping to jarMap
- addToList(jarName, fileName, jarMap);
+ addToList(jarName, jarItem, jarMap);
}
/**
@@ -248,18 +245,14 @@
fileName.equals(JarFile.MANIFEST_NAME))
continue;
- if (!metaInfFilenames) {
+ if (!metaInfFilenames || !fileName.startsWith("META-INF/")) {
add(fileName, currentJar);
- } else {
- if (!fileName.startsWith("META-INF/")) {
- add(fileName, currentJar);
- } else if (!entry.isDirectory()) {
+ } else if (!entry.isDirectory()) {
// Add files under META-INF explicitly so that certain
// services, like ServiceLoader, etc, can be located
// with greater accuracy. Directories can be skipped
// since each file will be added explicitly.
- addExplicit(fileName, currentJar);
- }
+ addMapping(fileName, currentJar);
}
}
@@ -324,8 +317,7 @@
jars.add(currentJar);
} else {
String name = line;
- addToList(name, currentJar, indexMap);
- addToList(currentJar, name, jarMap);
+ addMapping(name, currentJar);
}
}
@@ -354,7 +346,7 @@
if (path != null) {
jarName = path.concat(jarName);
}
- toIndex.add(packageName, jarName);
+ toIndex.addMapping(packageName, jarName);
}
}
}
diff --git a/test/sun/misc/JarIndex/JarIndexMergeForClassLoaderTest.java
b/test/sun/misc/JarIndex/JarIndexMergeForClassLoaderTest.java
new file mode 100644
--- /dev/null
+++ b/test/sun/misc/JarIndex/JarIndexMergeForClassLoaderTest.java
@@ -0,0 +1,224 @@
+/*
+ * Copyright (c) 2012, 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 6901992
+ * @summary Possible InvalidJarIndexException due to bug in
sun.misc.JarIndex.merge()
+ * Test URLClassLoader usage of the merge method when using indexes
+ * @author Diego Belfer
+ */
+import java.io.BufferedReader;
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.util.jar.JarEntry;
+import java.util.jar.JarOutputStream;
+
+
+public class JarIndexMergeForClassLoaderTest {
+ static String slash = File.separator;
+ static String testClasses = System.getProperty("test.classes");
+ static String testClassesDir = testClasses != null ? testClasses : ".";
+ static String jar;
+ static boolean debug = true;
+ static File tmpFolder = new File(testClassesDir);
+
+ static {
+ String javaHome = System.getProperty("java.home");
+ if (javaHome.endsWith("jre")) {
+ int index = javaHome.lastIndexOf(slash);
+ if (index != -1)
+ javaHome = javaHome.substring(0, index);
+ }
+
+ jar = javaHome + slash + "bin" + slash + "jar";
+ }
+
+ public static void main(String[] args) throws Exception {
+ // Create the jars file
+ File jar1 = buildJar1();
+ File jar2 = buildJar2();
+ File jar3 = buildJar3();
+
+ // Index jar files in two levels: jar1 -> jar2 -> jar3
+ createIndex(jar2.getName(), jar3.getName());
+ createIndex(jar1.getName(), jar2.getName());
+
+ // Get root jar of the URLClassLoader
+ URL url = jar1.toURI().toURL();
+
+ URLClassLoader classLoader = new URLClassLoader(new URL[] { url });
+
+ assertResource(classLoader, "com/jar1/resource.file", "jar1");
+ assertResource(classLoader, "com/test/resource1.file", "resource1");
+ assertResource(classLoader, "com/jar2/resource.file", "jar2");
+ assertResource(classLoader, "com/test/resource2.file", "resource2");
+ assertResource(classLoader, "com/test/resource3.file", "resource3");
+
+ /*
+ * The following two asserts failed before the fix of the bug 6901992
+ */
+ // Check that an existing file is found using the merged index
+ assertResource(classLoader, "com/missing/jar3/resource.file", "jar3");
+ // Check that a non existent file in directory which does not contain
+ // any file is not found and it does not throw InvalidJarIndexException
+ assertResource(classLoader, "com/missing/nofile", null);
+ }
+
+ private static File buildJar3() throws FileNotFoundException, IOException {
+ JarBuilder jar3Builder = new JarBuilder(tmpFolder, "jar3.jar");
+ jar3Builder.addResourceFile("com/test/resource3.file", "resource3");
+ jar3Builder.addResourceFile("com/missing/jar3/resource.file", "jar3");
+ return jar3Builder.build();
+ }
+
+ private static File buildJar2() throws FileNotFoundException, IOException {
+ JarBuilder jar2Builder = new JarBuilder(tmpFolder, "jar2.jar");
+ jar2Builder.addResourceFile("com/jar2/resource.file", "jar2");
+ jar2Builder.addResourceFile("com/test/resource2.file", "resource2");
+ return jar2Builder.build();
+ }
+
+ private static File buildJar1() throws FileNotFoundException, IOException {
+ JarBuilder jar1Builder = new JarBuilder(tmpFolder, "jar1.jar");
+ jar1Builder.addResourceFile("com/jar1/resource.file", "jar1");
+ jar1Builder.addResourceFile("com/test/resource1.file", "resource1");
+ return jar1Builder.build();
+ }
+
+ /* create the index */
+ static void createIndex(String parentJar, String childJar) {
+ // ProcessBuilder is used so that the current directory can be set
+ // to the directory that directly contains the jars.
+ debug("Running jar to create the index for: " + parentJar + " and "
+ + childJar);
+ ProcessBuilder pb = new ProcessBuilder(jar, "-i", parentJar, childJar);
+
+ pb.directory(tmpFolder);
+ // pd.inheritIO();
+ try {
+ Process p = pb.start();
+ if (p.waitFor() != 0)
+ throw new RuntimeException("jar indexing failed");
+
+ if (debug && p != null) {
+ debugStream(p.getInputStream());
+ debugStream(p.getErrorStream());
+ }
+ } catch (InterruptedException ie) {
+ throw new RuntimeException(ie);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ private static void debugStream(InputStream is) throws IOException {
+ BufferedReader reader = new BufferedReader(new InputStreamReader(is));
+ String line;
+ while ((line = reader.readLine()) != null) {
+ debug(line);
+ }
+ reader.close();
+ }
+
+ private static void assertResource(URLClassLoader classLoader, String file,
+ String expectedContent) throws IOException {
+ InputStream fileStream = classLoader.getResourceAsStream(file);
+
+ if (fileStream == null && expectedContent == null) {
+ return;
+ }
+ if (fileStream == null && expectedContent != null) {
+ throw new RuntimeException(
+ buildMessage(file, expectedContent, null));
+ }
+ try {
+ String actualContent = readAsString(fileStream);
+
+ if (fileStream != null && expectedContent == null) {
+ throw new RuntimeException(buildMessage(file, null,
+ actualContent));
+ }
+ if (!expectedContent.equals(actualContent)) {
+ throw new RuntimeException(buildMessage(file, expectedContent,
+ actualContent));
+ }
+ } finally {
+ fileStream.close();
+ }
+ }
+
+ private static String buildMessage(String file, String expectedContent,
+ String actualContent) {
+ return "Expected: " + expectedContent + " for: " + file + " was: "
+ + actualContent;
+ }
+
+ private static String readAsString(InputStream fileStream)
+ throws IOException {
+ byte[] buffer = new byte[1000];
+ int count = fileStream.read(buffer);
+ return new String(buffer, 0, count, "ASCII");
+ }
+
+ static void debug(Object message) {
+ if (debug) {
+ System.out.println(message);
+ }
+ }
+
+
+ /*
+ * Helper class for building jar files
+ */
+ public static class JarBuilder {
+ private JarOutputStream os;
+ private File jarFile;
+
+ public JarBuilder(File tmpFolder, String jarName)
+ throws FileNotFoundException, IOException {
+ this.jarFile = new File(tmpFolder, jarName);
+ this.os = new JarOutputStream(new FileOutputStream(jarFile));
+ }
+
+ public void addResourceFile(String pathFromRoot, String content)
+ throws IOException {
+ JarEntry entry = new JarEntry(pathFromRoot);
+ os.putNextEntry(entry);
+ os.write(content.getBytes("ASCII"));
+ os.closeEntry();
+ }
+
+ public File build() throws IOException {
+ os.close();
+ return jarFile;
+ }
+ }
+}
+
diff --git a/test/sun/misc/JarIndex/JarIndexMergeTest.java
b/test/sun/misc/JarIndex/JarIndexMergeTest.java
new file mode 100644
--- /dev/null
+++ b/test/sun/misc/JarIndex/JarIndexMergeTest.java
@@ -0,0 +1,114 @@
+/*
+ * Copyright (c) 2012, 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 6901992
+ * @summary Possible InvalidJarIndexException due to bug in
sun.misc.JarIndex.merge()
+ * @author Diego Belfer
+ */
+
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.util.LinkedList;
+import java.util.jar.JarEntry;
+import java.util.jar.JarOutputStream;
+
+import sun.misc.JarIndex;
+
+public class JarIndexMergeTest {
+ static String slash = File.separator;
+ static String testClasses = System.getProperty("test.classes");
+ static String testClassesDir = testClasses != null ? testClasses : ".";
+ static File tmpFolder = new File(testClassesDir);
+
+ public static void main(String[] args) throws Exception {
+
+ File jar1 = buildJar1();
+ File jar2 = buildJar2();
+
+ JarIndex jarIndex1 = new JarIndex(new String[] { jar1.getAbsolutePath()
});
+ JarIndex jarIndex2 = new JarIndex(new String[] { jar2.getAbsolutePath()
});
+
+ jarIndex1.merge(jarIndex2, null);
+
+ assertFileResolved(jarIndex2, "com/test1/resource1.file",
+ jar1.getAbsolutePath());
+ assertFileResolved(jarIndex2, "com/test2/resource2.file",
+ jar2.getAbsolutePath());
+ }
+
+ static void assertFileResolved(JarIndex jarIndex2, String file,
+ String jarName) {
+
+ LinkedList jarLists = jarIndex2.get(file);
+ if (jarLists == null || jarLists.size() == 0
+ || !jarName.equals(jarLists.get(0))) {
+ throw new RuntimeException(
+ "Unexpected result: the merged index must resolve file: "
+ + file);
+ }
+ }
+
+ private static File buildJar1() throws FileNotFoundException, IOException {
+ JarBuilder jar1Builder = new JarBuilder(tmpFolder, "jar1-merge.jar");
+ jar1Builder.addResourceFile("com/test1/resource1.file", "resource1");
+ return jar1Builder.build();
+ }
+
+ private static File buildJar2() throws FileNotFoundException, IOException {
+ JarBuilder jar2Builder = new JarBuilder(tmpFolder, "jar2-merge.jar");
+ jar2Builder.addResourceFile("com/test2/resource2.file", "resource2");
+ return jar2Builder.build();
+ }
+
+ /*
+ * Helper class for building jar files
+ */
+ public static class JarBuilder {
+ private JarOutputStream os;
+ private File jarFile;
+
+ public JarBuilder(File tmpFolder, String jarName)
+ throws FileNotFoundException, IOException {
+ this.jarFile = new File(tmpFolder, jarName);
+ this.os = new JarOutputStream(new FileOutputStream(jarFile));
+ }
+
+ public void addResourceFile(String pathFromRoot, String content)
+ throws IOException {
+ JarEntry entry = new JarEntry(pathFromRoot);
+ os.putNextEntry(entry);
+ os.write(content.getBytes("ASCII"));
+ os.closeEntry();
+ }
+
+ public File build() throws IOException {
+ os.close();
+ return jarFile;
+ }
+ }
+}
+
