Bug 60128 - File left open in ZipPackage when InvalidFormatException are raised
Summary: File left open in ZipPackage when InvalidFormatException are raised
Status: RESOLVED FIXED
Alias: None
Product: POI
Classification: Unclassified
Component: OPC (show other bugs)
Version: 3.15-dev
Hardware: PC All
: P2 critical (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-13 19:42 UTC by Matafagafo
Modified: 2016-09-14 18:42 UTC (History)
0 users



Attachments
A invalid format file (22 bytes, application/vnd.openxmlformats-officedocument.spreadsheetml.sheet)
2016-09-13 19:42 UTC, Matafagafo
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Matafagafo 2016-09-13 19:42:35 UTC
Created attachment 34243 [details]
A invalid format file

ZipPackage isn't closing opened files  when InvalidFormatExceptions are raised. 

Running the code bellow, using the provided sample file, shows the problem, while the sample code is running the file is open in operation system (you have to use some SO tool to verify this).
Some details:
* To run you have to correct the path to provided file.
* The provided file is a invalid XLSX file, was created just to generate the problem.

The Exception trace 

	at org.apache.poi.openxml4j.opc.ZipPackage.getPartsImpl(ZipPackage.java:238)
	at org.apache.poi.openxml4j.opc.OPCPackage.getParts(OPCPackage.java:726)
	at org.apache.poi.openxml4j.opc.OPCPackage.open(OPCPackage.java:228)
	at Main.main(Main.java:14)

It's happen because ZipPackage opens the file but don't close it when a Exception is raised.



The sample code:

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.util.logging.Level;
import java.util.logging.Logger;
import org.apache.poi.openxml4j.opc.OPCPackage;
import org.apache.poi.openxml4j.opc.PackageAccess;

public class Main {

	public static void main(String[] args) throws IOException {
		OPCPackage pkg = null;
		try {
			pkg = OPCPackage.open("C:/Temp/invalid file.xlsx", PackageAccess.READ);
		} catch (Exception ex) {
			Logger.getLogger(Main.class.getName()).log(Level.SEVERE, null, ex);
		} finally {
			if (pkg != null) {
				pkg.close();
			}
		}
		
		BufferedReader in = new BufferedReader(new InputStreamReader(System.in));
		String s;
		while ((s = in.readLine()) != null && s.length() != 0) {
			// Enter to end sample
		}
	}
}
Comment 1 Nick Burch 2016-09-14 12:39:22 UTC
Are you sure this is still happening on trunk?

OPCPackage has a bunch of catches in the open block, which attempt to close the faulty package before triggering the exception. I've added some unit tests in r1760693 which verify that the call to close on a faulty package does actually close the underlying stream/file. As such, I can't see how things aren't getting closed?
Comment 2 Javen O'Neal 2016-09-14 13:07:25 UTC
I found a couple spots where opened files were not closed when raising exceptions. The one mentioned in comment 0, caused by OPCPackage.open not closing the ZipPackage when pack.getParts() failed is fixed [1].
I found some other cases in the ZipPackage constructors that could fail without cleaning up their resources. [2]

I committed these changes in r1760702. They will likely be included in the POI 3.15 final release.

This commit is in desperate need of cleanup for someone who knows what they're doing with Java exception handling. This is admittedly beyond my skills.

Thanks for the test file, code, and stack trace. Got any ideas how we can check for unclosed resources in our unit tests? In TestZipPackage, we're using assertTrue(tmpFile.delete()) [3]

[1] OPCPackage.java https://svn.apache.org/viewvc/poi/trunk/src/ooxml/java/org/apache/poi/openxml4j/opc/OPCPackage.java?r1=1760702&r2=1760701&pathrev=1760702
[2] ZipPackage.java https://svn.apache.org/viewvc/poi/trunk/src/ooxml/java/org/apache/poi/openxml4j/opc/ZipPackage.java?r1=1760702&r2=1760701&pathrev=1760702
[3] TestZipPackage.java https://svn.apache.org/viewvc/poi/trunk/src/ooxml/testcases/org/apache/poi/openxml4j/opc/TestZipPackage.java?r1=1760702&r2=1760701&pathrev=1760702
Comment 3 Javen O'Neal 2016-09-14 13:17:30 UTC
Based on the line numbers in comment 0, it looks like Matafagafo is using POI 3.15 beta 2 or a trunk release between r1749795 (2016-06-22) and r1752997 (2016-07-16)
Comment 4 Matafagafo 2016-09-14 14:14:04 UTC
(In reply to Javen O'Neal from comment #3)
> Based on the line numbers in comment 0, it looks like Matafagafo is using
> POI 3.15 beta 2 or a trunk release between r1749795 (2016-06-22) and
> r1752997 (2016-07-16)

You are right, the stack is from POI 3.15 beta 2.
Sorry for the lack of info.
Comment 5 Javen O'Neal 2016-09-14 17:33:00 UTC
Changes from Throwable#addSuppress to Throwable#printStackTrace() to maintain Java 6 compatibility in r1760732.
Updated changelog in r1760734

Please reopen if the problem persists with the latest trunk build or the exception handling can be improved on.
Comment 6 Javen O'Neal 2016-09-14 17:48:40 UTC
r1760735 add test-data/openxml4j/invalid.xlsx to expected failures for integration test
Comment 7 Javen O'Neal 2016-09-14 18:42:01 UTC
r1760743 add test-data/openxml4j/invalid.xlsx to additional expected failures for integration test