bundles get silently ignored from being imported if compressed in a non-standard way

[gireeshpunathil]

I have a user who reported that bundles were missing from target platform definitions in later versions of eclipse. Digging deeper, we found the root cause as PDE was silently ignoring archives that it could not open. There are few extra bytes at the end of the zip header of the bundle which were tolerated by Java versions upto 11, but threw in 17 thus:

java.util.zip.ZipException: Invalid CEN header (invalid extra data field size for tag: 0xef00 at 84350)
        at java.base/java.util.zip.ZipFile$Source.zerror(ZipFile.java:1762)
        at java.base/java.util.zip.ZipFile$Source.checkExtraFields(ZipFile.java:1282)
        at java.base/java.util.zip.ZipFile$Source.checkAndAddEntry(ZipFile.java:1229)
        at java.base/java.util.zip.ZipFile$Source.initCEN(ZipFile.java:1701)
        at java.base/java.util.zip.ZipFile$Source.<init>(ZipFile.java:1479)
        at java.base/java.util.zip.ZipFile$Source.get(ZipFile.java:1441)
        at java.base/java.util.zip.ZipFile$CleanableResource.<init>(ZipFile.java:718)
        at java.base/java.util.zip.ZipFile.<init>(ZipFile.java:252)
        at java.base/java.util.zip.ZipFile.<init>(ZipFile.java:181)
        at TestZip.main(TestZip.java:10)

Looking at the relevant code:

eclipse.pde/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/DirectoryBundleContainer.java

Lines 75 to 90 in d5c48d8

	if (dir.isDirectory()) {
	File site = getSite(dir);
	File[] files = site.listFiles();
	SubMonitor localMonitor = SubMonitor.convert(monitor, Messages.DirectoryBundleContainer_0, files.length);
	return Arrays.stream(files).parallel() //
	.map(file -> {
	localMonitor.split(1);
	try {
	return new TargetBundle(file);
	} catch (CoreException e) {
	// Ignore non-bundle files
	return null;
	}
	}).filter(Objects::nonNull) //
	.toArray(TargetBundle[]::new);
	}

• from the comment surrounding the code it looks like we treated this bundle as a non-bundle
• can we tolerate the ZIP issue and bring back the old behaviour?
• if not, can we at least log this instead of ignoring?

[laeubi]

@gireeshpunathil I fear we can't do much without breaking others even though the code might be improved to be a bit smarter, the idea seem to be that such a directory can contain other files as well and these should be ignored.

But this seems (your stack trace is incomplete in this regards) to boil down to org.eclipse.pde.internal.core.util.ManifestUtils.loadManifest(File) where we deliver the causing exception and some status code, so maybe the catch part can be enhanced to emit an error if the file ends with .jar and the causing exception is a IOException ...

[gireeshpunathil]

java.util.zip.ZipException: zip END header not found
	at java.base/java.util.zip.ZipFile$Source.findEND(ZipFile.java:1633)
	at java.base/java.util.zip.ZipFile$Source.initCEN(ZipFile.java:1641)
	at java.base/java.util.zip.ZipFile$Source.<init>(ZipFile.java:1479)
	at java.base/java.util.zip.ZipFile$Source.get(ZipFile.java:1441)
	at java.base/java.util.zip.ZipFile$CleanableResource.<init>(ZipFile.java:718)
	at java.base/java.util.zip.ZipFile.<init>(ZipFile.java:252)
	at java.base/java.util.zip.ZipFile.<init>(ZipFile.java:181)
	at org.eclipse.pde.internal.core.util.ManifestUtils.loadManifest(ManifestUtils.java:108)
	at org.eclipse.pde.core.target.TargetBundle.initialize(TargetBundle.java:175)
	at org.eclipse.pde.core.target.TargetBundle.<init>(TargetBundle.java:92)
	at org.eclipse.pde.internal.core.target.DirectoryBundleContainer.lambda$0(DirectoryBundleContainer.java:83)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
	at java.base/java.util.Spliterators$ArraySpliterator.forEachRemaining(Spliterators.java:992)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:522)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:512)
	at java.base/java.util.stream.Nodes$CollectorTask.doLeaf(Nodes.java:2183)
	at java.base/java.util.stream.Nodes$CollectorTask.doLeaf(Nodes.java:2149)
	at java.base/java.util.stream.AbstractTask.compute(AbstractTask.java:327)
	at java.base/java.util.concurrent.CountedCompleter.exec(CountedCompleter.java:754)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:373)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1182)
	at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1655)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1622)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:165)
java.util.zip.ZipException: Invalid CEN header (invalid extra data field size for tag: 0xef00 at 84350)
	at java.base/java.util.zip.ZipFile$Source.zerror(ZipFile.java:1762)
	at java.base/java.util.zip.ZipFile$Source.checkExtraFields(ZipFile.java:1282)
	at java.base/java.util.zip.ZipFile$Source.checkAndAddEntry(ZipFile.java:1229)
	at java.base/java.util.zip.ZipFile$Source.initCEN(ZipFile.java:1701)
	at java.base/java.util.zip.ZipFile$Source.<init>(ZipFile.java:1479)
	at java.base/java.util.zip.ZipFile$Source.get(ZipFile.java:1441)
	at java.base/java.util.zip.ZipFile$CleanableResource.<init>(ZipFile.java:718)
	at java.base/java.util.zip.ZipFile.<init>(ZipFile.java:252)
	at java.base/java.util.zip.ZipFile.<init>(ZipFile.java:181)
	at org.eclipse.pde.internal.core.util.ManifestUtils.loadManifest(ManifestUtils.java:108)
	at org.eclipse.pde.core.target.TargetBundle.initialize(TargetBundle.java:175)
	at org.eclipse.pde.core.target.TargetBundle.<init>(TargetBundle.java:92)
	at org.eclipse.pde.internal.core.target.DirectoryBundleContainer.lambda$0(DirectoryBundleContainer.java:83)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
	at java.base/java.util.Spliterators$ArraySpliterator.forEachRemaining(Spliterators.java:992)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:522)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:512)
	at java.base/java.util.stream.Nodes$CollectorTask.doLeaf(Nodes.java:2183)
	at java.base/java.util.stream.Nodes$CollectorTask.doLeaf(Nodes.java:2149)
	at java.base/java.util.stream.AbstractTask.compute(AbstractTask.java:327)
	at java.base/java.util.concurrent.CountedCompleter.exec(CountedCompleter.java:754)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:373)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1182)
	at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1655)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1622)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:16

thanks @laeubi . In this case, this is the full stack trace that we get. So expanding the exception makes sense to me. Funny enough, we seem to be throwing CoreException if the location is not a directory, but not when some files cannot be parsed inside. Will throwing be acceptable (the file ends with .jar but parsing causes an exception) ? or will it break any API contract? the function is i) protected, ii) already declared to throw Exception, and is iii) already throwing in some code paths.

[laeubi]

Its hard to tell, I think at least an ILog.error() should be considered. Directory locations are mostly legacy use and legacy code tend to do strange stuff and people don't like if something "suddenly" breaks.

On the other hand, the more correct solution would probably that a Warning for the resolved location is issued if not PDE would handle warnings strange:

• Warnings from target bundles create strange results #656

so probably in the scope of #656 this would be a good use-case to enhance it!