You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@poi.apache.org by bu...@apache.org on 2018/12/21 10:50:06 UTC
[Bug 63029] New: Potentially clobbers files on close
https://bz.apache.org/bugzilla/show_bug.cgi?id=63029
Bug ID: 63029
Summary: Potentially clobbers files on close
Product: POI
Version: 4.0.x-dev
Hardware: PC
Status: NEW
Severity: critical
Priority: P2
Component: OPC
Assignee: dev@poi.apache.org
Reporter: a.doerfler@e-sign.com
Target Milestone: ---
I read an XLSX file this way:
workBookPackage = OPCPackage.open(path.toFile(),
PackageAccess.READ);
workBook = new XSSFWorkbook(workBookPackage);
I'm using this method, as the documentation says that "Creating a XSSFWorkbook
from a file-backed OPC Package has a lower memoryfootprint than an InputStream
backed one."
Now my application got interrupted, and this resulted in a zero byte XLSX file.
Unfortunately, I don't have the complete stack trace anymore, but I got a
`java.nio.channels.ClosedByInterruptException` from FileHelper.copyFile().
Looking at ZipPackage.closeImpl(), it looks like it *always*, and
unconditionally clobbers the original file, even if I had used
PackageAccess.READ to open the package.
The other issue is, that closeImpl() does not even try to use an atomic move to
make replacing the original file saver.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 63029] OPCPackage Potentially clobbers files on close()
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=63029
--- Comment #4 from Axel Dörfler <a....@e-sign.com> ---
What makes this bug even worse is that the temp file is deleted in any case;
ZipPackage.closeImpl() deletes the temp file in a finally block.
Even if an atomic move is not possible (if the temp dir is on a different file
system, for example), you can still make it more secure by renaming the
original file first, and only delete that in case everything worked out.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 63029] OPCPackage Potentially clobbers files on close()
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=63029
Andreas Beeker <ki...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Hardware|PC |All
Depends on| |59287
Referenced Bugs:
https://bz.apache.org/bugzilla/show_bug.cgi?id=59287
[Bug 59287] Provide a write() method and change semantics of close() to not
automatically write
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 63029] OPCPackage Potentially clobbers files on close()
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=63029
--- Comment #6 from Yegor Kozlov <ye...@dinom.ru> ---
a unit test to reproduce corruption :
try(OPCPackage pkg = OPCPackage.open(path.toFile(), PackageAccess.READ_WRITE))
{
// add a marshaller that will throw an exception on save
pkg.addMarshaller("poi/junit", (part, out) -> {
throw new RuntimeException("Bugzilla 63029");
});
pkg.createPart(PackagingURIHelper.createPartName("/poi/test.xml"),
"poi/junit");
} catch (RuntimeException e){
assert("Bugzilla 63029", e.getMessage());
}
// try to read the source file once again
try ( ZipFile zip = new ZipFile(path.toFile())){
// throws java.util.zip.ZipException: archive is not a ZIP archive
}
an exception while saving *may* result in a clobbered file. The size of the
corrupted data depends on how much was saved and flushed on disk: it can be
anywhere from zero-byte to the length of the original file.
a simple change to avoid corruption would be to replace the original file only
if save() succeeded. Something like this:
boolean success = false;
try {
save(tempFile);
success = true;
} finally {
// Close the current zip file, so we can overwrite it on all platforms
IOUtils.closeQuietly(this.zipArchive);
try {
// Copy the new file over the old one
if(success) {
FileHelper.copyFile(tempFile, targetFile);
}
} finally {
// Either the save operation succeed or not, we delete the temporary
file
if (!tempFile.delete()) {
LOG.log(POILogger.WARN, "The temporary file: '"
+ targetFile.getAbsolutePath()
+ "' cannot be deleted ! Make sure that no other
application use it.");
}
}
}
this would leave the origin intact in case of an exception.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 63029] OPCPackage Potentially clobbers files on close()
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=63029
Yegor Kozlov <ye...@dinom.ru> changed:
What |Removed |Added
----------------------------------------------------------------------------
Resolution|--- |FIXED
Status|NEW |RESOLVED
--- Comment #7 from Yegor Kozlov <ye...@dinom.ru> ---
Fixed in r1853454, a unit test added.
Yegor
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 63029] OPCPackage Potentially clobbers files on close()
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=63029
--- Comment #5 from Axel Dörfler <a....@e-sign.com> ---
Only my misuse of the API depends on #59287 -- the actual issue does not,
however.
Even when using the API correctly, the file may end up being corrupted at the
end.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 63029] Potentially clobbers files on close
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=63029
--- Comment #2 from PJ Fanning <fa...@yahoo.com> ---
can you provide a reproducible test case?
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 63029] OPCPackage Potentially clobbers files on close()
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=63029
Axel Dörfler <a....@e-sign.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Summary|Potentially clobbers files |OPCPackage Potentially
|on close |clobbers files on close()
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 63029] Potentially clobbers files on close
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=63029
Axel Dörfler <a....@e-sign.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Severity|critical |major
--- Comment #3 from Axel Dörfler <a....@e-sign.com> ---
Sorry, if I had the time for that, I would have delivered a patch already ;-)
But I've read into the sources a bit more; turns out that only
OPCPackage.close() calls into ZipPackage.closeImpl(), and that is *not* called
in case of PackageAccess.READ. So that part is a wrong assumption of mine.
I should have called OPCPackage.revert() instead of close() -- not the most
intuitive API choice, but my fault in the end.
The only remaining issue is that the file is lost if ZipPackage.closeImpl() is
aborted at the wrong time. It really should, if possible, use an atomic move to
replace the original file instead.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
[Bug 63029] Potentially clobbers files on close
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=63029
Axel Dörfler <a....@e-sign.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
OS| |All
--- Comment #1 from Axel Dörfler <a....@e-sign.com> ---
I accidentally copied the wrong sources; when the problem appeared, I did not
specify PackageAccess.READ in OPCPackage.open().
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org