Skip to content

Override ZipOutputStream.write(int)#145

Merged
bodewig merged 1 commit intoapache:masterfrom
helfper:fix-zos-write-byte
Mar 13, 2021
Merged

Override ZipOutputStream.write(int)#145
bodewig merged 1 commit intoapache:masterfrom
helfper:fix-zos-write-byte

Conversation

@helfper
Copy link
Copy Markdown
Contributor

@helfper helfper commented Mar 13, 2021

java.util.zip.ZipOutputStream extends from java.util.zip.DeflaterOutputStream which overrides void write(int b) to redirect its calls to void write(byte[] b, int off, int len).

org.apache.tools.zip.ZipOutputStream doesn't extend the same class and currently doesn't override void write(int b). This means that calls to this method end up in FilterOutputStream.write(int b), which in turn passes them through to the underlying OutputStream. This bypasses all the logic in ZipOutputStream.write(byte[] b, int offset, int len) (deflation and whatnot) which produces corrupted ZIP files.

One example where the method void write(int b) is ultimately invoked is by calling java.util.jar.Manifest.write(zipOutputStream).

This PR fixes this by overriding the method void write(int b) in org.apache.tools.zip.ZipOutputStream in the same way as java.util.zip.DeflaterOutputStream does.

*/
@Override
public void write(int b) throws IOException {
byte[] buf = new byte[1];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could allocate a one-byte array and reuse that for all calls. Of course then we'd need to think about thread safety issues. So maybe that's not wort the trouble given that so far nobody seems to have used that method anyway :-)

Copy link
Copy Markdown
Contributor Author

@helfper helfper Mar 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I thought about that but was wondering why DeflaterOutputStream didn't do it. I did it now as this class is not thread-safe anyway, and this allocation on every write may keep the GC unnecessarily busy. I checked Apache Commons Compress since you mentioned it and it's also how it's done there: https://github.com/apache/commons-compress/blob/master/src/main/java/org/apache/commons/compress/archivers/ArchiveOutputStream.java#L52

@bodewig
Copy link
Copy Markdown
Member

bodewig commented Mar 13, 2021

Many thanks - interesting this has been there for more then a decade. Most likely this is because Ant is supposed to be the only consumer of the class and doesn't use the single byte write method itself. For general use I'd recommend to use ZipArchiveOutputStream of Apache Commons Compress which is a grandchild of Ant's zip package.

@helfper please add yourself to the contributors.xml and CONTRIBUTORS files in Ant's top level.

@helfper helfper force-pushed the fix-zos-write-byte branch from 0e857f9 to 793519b Compare March 13, 2021 12:33
@helfper
Copy link
Copy Markdown
Contributor Author

helfper commented Mar 13, 2021

@bodewig I've updated the contributors files now.

I've found the issue in https://github.com/johnrengelman/shadow which uses Ant's ZipOutputStream. Out of curiosity, why doesn't Ant use Commons Compress's ZipArchiveOutputStream?

@bodewig
Copy link
Copy Markdown
Member

bodewig commented Mar 13, 2021

Ant predates Commons Compress by several years.

Actually Ant's core doesn't have any dependencies at all, this has always been the case. Initially the XML parser had to be provided before JAXP became part of the standard class library. This is an important goal of Ant. We can bootstrap Ant with just a JDK and nothing else.

@bodewig bodewig merged commit 2f5ebf2 into apache:master Mar 13, 2021
asfgit pushed a commit that referenced this pull request Mar 13, 2021
@helfper helfper deleted the fix-zos-write-byte branch March 13, 2021 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants