From f0c21325faf6f613821a8b542c73ac380f0feb6a Mon Sep 17 00:00:00 2001 From: Simon Spero Date: Sun, 11 Jun 2017 18:48:42 -0400 Subject: [PATCH 1/2] COMPRESS-407 Validate Block and Record Sizes Require block size >=0 that is multiple of 512 bytes. Use block size of 512 if one is not given. Require record size of 512 bytes. Deprecate constructors taking recordSize as parameter Modify tests to check for enforcement of record size == 512 Modify tests to check for correct overall length for different block sizes including PAX default, USTAR default, and unspecified. Signed-off-by: Simon Spero (cherry picked from commit d754d89) Signed-off-by: Simon Spero --- .../archivers/tar/TarArchiveOutputStream.java | 217 ++++++++++-------- .../tar/TarArchiveOutputStreamTest.java | 209 +++++++++++------ 2 files changed, 261 insertions(+), 165 deletions(-) diff --git a/src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStream.java b/src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStream.java index 79c7bca86de..864d4ad0cbd 100644 --- a/src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStream.java +++ b/src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStream.java @@ -61,18 +61,18 @@ public class TarArchiveOutputStream extends ArchiveOutputStream { /** POSIX/PAX extensions are used to store big numbers in the archive. */ public static final int BIGNUMBER_POSIX = 2; - - private long currSize; - private String currName; - private long currBytes; - private final byte[] recordBuf; - private int assemLen; - private final byte[] assemBuf; - private int longFileMode = LONGFILE_ERROR; - private int bigNumberMode = BIGNUMBER_ERROR; + private static final int RECORD_SIZE = 512; + + private long currSize; + private String currName; + private long currBytes; + private final byte[] recordBuf; + private int assemLen; + private final byte[] assemBuf; + private int longFileMode = LONGFILE_ERROR; + private int bigNumberMode = BIGNUMBER_ERROR; private int recordsWritten; private final int recordsPerBlock; - private final int recordSize; private boolean closed = false; @@ -93,12 +93,14 @@ public class TarArchiveOutputStream extends ArchiveOutputStream { private static final ZipEncoding ASCII = ZipEncodingHelper.getZipEncoding("ASCII"); + private static int BLOCK_SIZE_UNSPECIFIED = -511; + /** * Constructor for TarInputStream. * @param os the output stream to use */ public TarArchiveOutputStream(final OutputStream os) { - this(os, TarConstants.DEFAULT_BLKSIZE, TarConstants.DEFAULT_RCDSIZE); + this(os, BLOCK_SIZE_UNSPECIFIED); } /** @@ -108,59 +110,82 @@ public TarArchiveOutputStream(final OutputStream os) { * @since 1.4 */ public TarArchiveOutputStream(final OutputStream os, final String encoding) { - this(os, TarConstants.DEFAULT_BLKSIZE, TarConstants.DEFAULT_RCDSIZE, encoding); + this(os, BLOCK_SIZE_UNSPECIFIED, encoding); } /** * Constructor for TarInputStream. * @param os the output stream to use - * @param blockSize the block size to use + * @param blockSize the block size to use. Must be a multiple of 512 bytes. */ public TarArchiveOutputStream(final OutputStream os, final int blockSize) { - this(os, blockSize, TarConstants.DEFAULT_RCDSIZE); + this(os, blockSize, null); } + /** * Constructor for TarInputStream. * @param os the output stream to use * @param blockSize the block size to use - * @param encoding name of the encoding to use for file names - * @since 1.4 + * @param recordSize the record size to use. Must be 512 bytes. + * @deprecated recordSize must always be 512 bytes. An IllegalArgumentException will be thrown + * if any other value is used */ + @Deprecated public TarArchiveOutputStream(final OutputStream os, final int blockSize, - final String encoding) { - this(os, blockSize, TarConstants.DEFAULT_RCDSIZE, encoding); + final int recordSize) { + this(os, blockSize, recordSize, null); } /** * Constructor for TarInputStream. * @param os the output stream to use - * @param blockSize the block size to use - * @param recordSize the record size to use + * @param blockSize the block size to use . Must be a multiple of 512 bytes. + * @param recordSize the record size to use. Must be 512 bytes. + * @param encoding name of the encoding to use for file names + * @since 1.4 + * @deprecated recordSize must always be 512 bytes. An IllegalArgumentException will be thrown + * if any other value is used. */ - public TarArchiveOutputStream(final OutputStream os, final int blockSize, final int recordSize) { - this(os, blockSize, recordSize, null); + @Deprecated + public TarArchiveOutputStream(final OutputStream os, final int blockSize, + final int recordSize, final String encoding) { + this(os, blockSize, encoding); + if (recordSize != RECORD_SIZE) { + throw new IllegalArgumentException( + "Tar record size must always be 512 bytes. Attempt to set size of " + recordSize); + } + } /** * Constructor for TarInputStream. + * * @param os the output stream to use - * @param blockSize the block size to use - * @param recordSize the record size to use + * @param blockSize the block size to use. Must be a multiple of 512 bytes. * @param encoding name of the encoding to use for file names * @since 1.4 */ public TarArchiveOutputStream(final OutputStream os, final int blockSize, - final int recordSize, final String encoding) { + final String encoding) { + int realBlockSize; + if (BLOCK_SIZE_UNSPECIFIED == blockSize) { + realBlockSize = RECORD_SIZE; + } else { + realBlockSize = blockSize; + } + + if(realBlockSize <=0 || realBlockSize % RECORD_SIZE != 0) { + throw new IllegalArgumentException("Block size must be a multiple of 512 bytes. Attempt to use set size of " + blockSize); + } out = new CountingOutputStream(os); this.encoding = encoding; this.zipEncoding = ZipEncodingHelper.getZipEncoding(encoding); this.assemLen = 0; - this.assemBuf = new byte[recordSize]; - this.recordBuf = new byte[recordSize]; - this.recordSize = recordSize; - this.recordsPerBlock = blockSize / recordSize; + this.assemBuf = new byte[RECORD_SIZE]; + this.recordBuf = new byte[RECORD_SIZE]; + this.recordsPerBlock = realBlockSize / RECORD_SIZE; } /** @@ -208,11 +233,11 @@ public long getBytesWritten() { /** * Ends the TAR archive without closing the underlying OutputStream. - * + * * An archive consists of a series of file entries terminated by an - * end-of-archive entry, which consists of two 512 blocks of zero bytes. + * end-of-archive entry, which consists of two 512 blocks of zero bytes. * POSIX.1 requires two EOF records, like some other implementations. - * + * * @throws IOException on error */ @Override @@ -251,9 +276,11 @@ public void close() throws IOException { * Get the record size being used by this stream's TarBuffer. * * @return The TarBuffer record size. + * @deprecated */ + @Deprecated public int getRecordSize() { - return this.recordSize; + return RECORD_SIZE; } /** @@ -278,12 +305,12 @@ public void putArchiveEntry(final ArchiveEntry archiveEntry) throws IOException final Map paxHeaders = new HashMap<>(); final String entryName = entry.getName(); final boolean paxHeaderContainsPath = handleLongName(entry, entryName, paxHeaders, "path", - TarConstants.LF_GNUTYPE_LONGNAME, "file name"); + TarConstants.LF_GNUTYPE_LONGNAME, "file name"); final String linkName = entry.getLinkName(); final boolean paxHeaderContainsLinkPath = linkName != null && linkName.length() > 0 && handleLongName(entry, linkName, paxHeaders, "linkpath", - TarConstants.LF_GNUTYPE_LONGLINK, "link name"); + TarConstants.LF_GNUTYPE_LONGLINK, "link name"); if (bigNumberMode == BIGNUMBER_POSIX) { addPaxHeadersForBigNumbers(paxHeaders, entry); @@ -307,7 +334,7 @@ && handleLongName(entry, linkName, paxHeaders, "linkpath", } entry.writeEntryHeader(recordBuf, zipEncoding, - bigNumberMode == BIGNUMBER_STAR); + bigNumberMode == BIGNUMBER_STAR); writeRecord(recordBuf); currBytes = 0; @@ -336,7 +363,7 @@ public void closeArchiveEntry() throws IOException { if (finished) { throw new IOException("Stream has already been finished"); } - if (!haveUnclosedEntry){ + if (!haveUnclosedEntry) { throw new IOException("No current entry to close"); } if (assemLen > 0) { @@ -352,9 +379,9 @@ public void closeArchiveEntry() throws IOException { if (currBytes < currSize) { throw new IOException("entry '" + currName + "' closed at '" - + currBytes - + "' before the '" + currSize - + "' bytes specified in the header were written"); + + currBytes + + "' before the '" + currSize + + "' bytes specified in the header were written"); } haveUnclosedEntry = false; } @@ -380,9 +407,9 @@ public void write(final byte[] wBuf, int wOffset, int numToWrite) throws IOExcep } if (currBytes + numToWrite > currSize) { throw new IOException("request to write '" + numToWrite - + "' bytes exceeds size in header of '" - + currSize + "' bytes for entry '" - + currName + "'"); + + "' bytes exceeds size in header of '" + + currSize + "' bytes for entry '" + + currName + "'"); // // We have to deal with assembly!!! @@ -398,9 +425,9 @@ public void write(final byte[] wBuf, int wOffset, int numToWrite) throws IOExcep final int aLen = recordBuf.length - assemLen; System.arraycopy(assemBuf, 0, recordBuf, 0, - assemLen); + assemLen); System.arraycopy(wBuf, wOffset, recordBuf, - assemLen, aLen); + assemLen, aLen); writeRecord(recordBuf); currBytes += recordBuf.length; @@ -409,7 +436,7 @@ public void write(final byte[] wBuf, int wOffset, int numToWrite) throws IOExcep assemLen = 0; } else { System.arraycopy(wBuf, wOffset, assemBuf, assemLen, - numToWrite); + numToWrite); wOffset += numToWrite; assemLen += numToWrite; @@ -425,7 +452,7 @@ public void write(final byte[] wBuf, int wOffset, int numToWrite) throws IOExcep while (numToWrite > 0) { if (numToWrite < recordBuf.length) { System.arraycopy(wBuf, wOffset, assemBuf, assemLen, - numToWrite); + numToWrite); assemLen += numToWrite; @@ -447,14 +474,14 @@ public void write(final byte[] wBuf, int wOffset, int numToWrite) throws IOExcep * @since 1.4 */ void writePaxHeaders(final TarArchiveEntry entry, - final String entryName, - final Map headers) throws IOException { + final String entryName, + final Map headers) throws IOException { String name = "./PaxHeaders.X/" + stripTo7Bits(entryName); if (name.length() >= TarConstants.NAMELEN) { name = name.substring(0, TarConstants.NAMELEN - 1); } final TarArchiveEntry pex = new TarArchiveEntry(name, - TarConstants.LF_PAX_EXTENDED_HEADER_LC); + TarConstants.LF_PAX_EXTENDED_HEADER_LC); transferModTime(entry, pex); final StringWriter w = new StringWriter(); @@ -500,8 +527,8 @@ private String stripTo7Bits(final String name) { } /** - * @return true if the character could lead to problems when used - * inside a TarArchiveEntry name for a PAX header. + * @return true if the character could lead to problems when used inside a TarArchiveEntry name + * for a PAX header. */ private boolean shouldBeReplaced(final char c) { return c == 0 // would be read as Trailing null @@ -510,8 +537,8 @@ private boolean shouldBeReplaced(final char c) { } /** - * Write an EOF (end of archive) record to the tar archive. - * An EOF record consists of a record of all zeros. + * Write an EOF (end of archive) record to the tar archive. An EOF record consists of a record + * of all zeros. */ private void writeEOFRecord() throws IOException { Arrays.fill(recordBuf, (byte) 0); @@ -525,13 +552,13 @@ public void flush() throws IOException { @Override public ArchiveEntry createArchiveEntry(final File inputFile, final String entryName) - throws IOException { - if(finished) { + throws IOException { + if (finished) { throw new IOException("Stream has already been finished"); } return new TarArchiveEntry(inputFile, entryName); } - + /** * Write an archive record to the archive. * @@ -539,17 +566,17 @@ public ArchiveEntry createArchiveEntry(final File inputFile, final String entryN * @throws IOException on error */ private void writeRecord(final byte[] record) throws IOException { - if (record.length != recordSize) { + if (record.length != RECORD_SIZE) { throw new IOException("record to write has length '" - + record.length - + "' which is not the record size of '" - + recordSize + "'"); + + record.length + + "' which is not the record size of '" + + RECORD_SIZE + "'"); } out.write(record); recordsWritten++; } - + /** * Write an archive record to the archive, where the record may be * inside of a larger array buffer. The buffer must be "offset plus @@ -560,15 +587,15 @@ private void writeRecord(final byte[] record) throws IOException { * @throws IOException on error */ private void writeRecord(final byte[] buf, final int offset) throws IOException { - - if (offset + recordSize > buf.length) { + + if (offset + RECORD_SIZE > buf.length) { throw new IOException("record has length '" + buf.length - + "' with offset '" + offset - + "' which is less than the record size of '" - + recordSize + "'"); + + "' with offset '" + offset + + "' which is less than the record size of '" + + RECORD_SIZE + "'"); } - out.write(buf, offset, recordSize); + out.write(buf, offset, RECORD_SIZE); recordsWritten++; } @@ -582,28 +609,28 @@ private void padAsNeeded() throws IOException { } private void addPaxHeadersForBigNumbers(final Map paxHeaders, - final TarArchiveEntry entry) { + final TarArchiveEntry entry) { addPaxHeaderForBigNumber(paxHeaders, "size", entry.getSize(), - TarConstants.MAXSIZE); + TarConstants.MAXSIZE); addPaxHeaderForBigNumber(paxHeaders, "gid", entry.getLongGroupId(), - TarConstants.MAXID); + TarConstants.MAXID); addPaxHeaderForBigNumber(paxHeaders, "mtime", - entry.getModTime().getTime() / 1000, - TarConstants.MAXSIZE); + entry.getModTime().getTime() / 1000, + TarConstants.MAXSIZE); addPaxHeaderForBigNumber(paxHeaders, "uid", entry.getLongUserId(), - TarConstants.MAXID); + TarConstants.MAXID); // star extensions by J\u00f6rg Schilling addPaxHeaderForBigNumber(paxHeaders, "SCHILY.devmajor", - entry.getDevMajor(), TarConstants.MAXID); + entry.getDevMajor(), TarConstants.MAXID); addPaxHeaderForBigNumber(paxHeaders, "SCHILY.devminor", - entry.getDevMinor(), TarConstants.MAXID); + entry.getDevMinor(), TarConstants.MAXID); // there is no PAX header for file mode failForBigNumber("mode", entry.getMode(), TarConstants.MAXID); } private void addPaxHeaderForBigNumber(final Map paxHeaders, - final String header, final long value, - final long maxValue) { + final String header, final long value, + final long maxValue) { if (value < 0 || value > maxValue) { paxHeaders.put(header, String.valueOf(value)); } @@ -613,29 +640,32 @@ private void failForBigNumbers(final TarArchiveEntry entry) { failForBigNumber("entry size", entry.getSize(), TarConstants.MAXSIZE); failForBigNumberWithPosixMessage("group id", entry.getLongGroupId(), TarConstants.MAXID); failForBigNumber("last modification time", - entry.getModTime().getTime() / 1000, - TarConstants.MAXSIZE); + entry.getModTime().getTime() / 1000, + TarConstants.MAXSIZE); failForBigNumber("user id", entry.getLongUserId(), TarConstants.MAXID); failForBigNumber("mode", entry.getMode(), TarConstants.MAXID); failForBigNumber("major device number", entry.getDevMajor(), - TarConstants.MAXID); + TarConstants.MAXID); failForBigNumber("minor device number", entry.getDevMinor(), - TarConstants.MAXID); + TarConstants.MAXID); } private void failForBigNumber(final String field, final long value, final long maxValue) { failForBigNumber(field, value, maxValue, ""); } - private void failForBigNumberWithPosixMessage(final String field, final long value, final long maxValue) { - failForBigNumber(field, value, maxValue, " Use STAR or POSIX extensions to overcome this limit"); + private void failForBigNumberWithPosixMessage(final String field, final long value, + final long maxValue) { + failForBigNumber(field, value, maxValue, + " Use STAR or POSIX extensions to overcome this limit"); } - private void failForBigNumber(final String field, final long value, final long maxValue, final String additionalMsg) { + private void failForBigNumber(final String field, final long value, final long maxValue, + final String additionalMsg) { if (value < 0 || value > maxValue) { throw new RuntimeException(field + " '" + value //NOSONAR - + "' is too big ( > " - + maxValue + " )." + additionalMsg); + + "' is too big ( > " + + maxValue + " )." + additionalMsg); } } @@ -661,9 +691,9 @@ private void failForBigNumber(final String field, final long value, final long m * @param fieldName the name of the field * @return whether a pax header has been written. */ - private boolean handleLongName(final TarArchiveEntry entry , final String name, - final Map paxHeaders, - final String paxHeaderName, final byte linkType, final String fieldName) + private boolean handleLongName(final TarArchiveEntry entry, final String name, + final Map paxHeaders, + final String paxHeaderName, final byte linkType, final String fieldName) throws IOException { final ByteBuffer encodedName = zipEncoding.encode(name); final int len = encodedName.limit() - encodedName.position(); @@ -675,7 +705,8 @@ private boolean handleLongName(final TarArchiveEntry entry , final String name, } else if (longFileMode == LONGFILE_GNU) { // create a TarEntry for the LongLink, the contents // of which are the link's name - final TarArchiveEntry longLinkEntry = new TarArchiveEntry(TarConstants.GNU_LONGLINK, linkType); + final TarArchiveEntry longLinkEntry = new TarArchiveEntry(TarConstants.GNU_LONGLINK, + linkType); longLinkEntry.setSize(len + 1l); // +1 for NUL transferModTime(entry, longLinkEntry); @@ -685,8 +716,8 @@ private boolean handleLongName(final TarArchiveEntry entry , final String name, closeArchiveEntry(); } else if (longFileMode != LONGFILE_TRUNCATE) { throw new RuntimeException(fieldName + " '" + name //NOSONAR - + "' is too long ( > " - + TarConstants.NAMELEN + " bytes)"); + + "' is too long ( > " + + TarConstants.NAMELEN + " bytes)"); } } return false; diff --git a/src/test/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStreamTest.java b/src/test/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStreamTest.java index 85cdb66a30f..497a7640ea0 100644 --- a/src/test/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStreamTest.java +++ b/src/test/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStreamTest.java @@ -26,6 +26,7 @@ import java.io.FileInputStream; import java.io.FileOutputStream; import java.io.IOException; +import java.io.InputStream; import java.security.MessageDigest; import java.util.Calendar; import java.util.Date; @@ -100,10 +101,10 @@ public void testBigNumberStarMode() throws Exception { tos.write(new byte[10 * 1024]); final byte[] data = bos.toByteArray(); assertEquals(0x80, - data[TarConstants.NAMELEN - + TarConstants.MODELEN - + TarConstants.UIDLEN - + TarConstants.GIDLEN] & 0x80); + data[TarConstants.NAMELEN + + TarConstants.MODELEN + + TarConstants.UIDLEN + + TarConstants.GIDLEN] & 0x80); final TarArchiveInputStream tin = new TarArchiveInputStream(new ByteArrayInputStream(data)); final TarArchiveEntry e = tin.getNextTarEntry(); @@ -126,12 +127,12 @@ public void testBigNumberPosixMode() throws Exception { tos.write(new byte[10 * 1024]); final byte[] data = bos.toByteArray(); assertEquals("00000000000 ", - new String(data, - 1024 + TarConstants.NAMELEN - + TarConstants.MODELEN - + TarConstants.UIDLEN - + TarConstants.GIDLEN, 12, - CharsetNames.UTF_8)); + new String(data, + 1024 + TarConstants.NAMELEN + + TarConstants.MODELEN + + TarConstants.UIDLEN + + TarConstants.GIDLEN, 12, + CharsetNames.UTF_8)); final TarArchiveInputStream tin = new TarArchiveInputStream(new ByteArrayInputStream(data)); final TarArchiveEntry e = tin.getNextTarEntry(); @@ -148,11 +149,11 @@ public void testWriteSimplePaxHeaders() throws Exception { m.put("a", "b"); final byte[] data = writePaxHeader(m); assertEquals("00000000006 ", - new String(data, TarConstants.NAMELEN - + TarConstants.MODELEN - + TarConstants.UIDLEN - + TarConstants.GIDLEN, 12, - CharsetNames.UTF_8)); + new String(data, TarConstants.NAMELEN + + TarConstants.MODELEN + + TarConstants.UIDLEN + + TarConstants.GIDLEN, 12, + CharsetNames.UTF_8)); assertEquals("6 a=b\n", new String(data, 512, 6, CharsetNames.UTF_8)); } @@ -160,38 +161,38 @@ public void testWriteSimplePaxHeaders() throws Exception { public void testPaxHeadersWithLength99() throws Exception { final Map m = new HashMap<>(); m.put("a", - "0123456789012345678901234567890123456789" - + "01234567890123456789012345678901234567890123456789" - + "012"); + "0123456789012345678901234567890123456789" + + "01234567890123456789012345678901234567890123456789" + + "012"); final byte[] data = writePaxHeader(m); assertEquals("00000000143 ", - new String(data, TarConstants.NAMELEN - + TarConstants.MODELEN - + TarConstants.UIDLEN - + TarConstants.GIDLEN, 12, - CharsetNames.UTF_8)); + new String(data, TarConstants.NAMELEN + + TarConstants.MODELEN + + TarConstants.UIDLEN + + TarConstants.GIDLEN, 12, + CharsetNames.UTF_8)); assertEquals("99 a=0123456789012345678901234567890123456789" - + "01234567890123456789012345678901234567890123456789" - + "012\n", new String(data, 512, 99, CharsetNames.UTF_8)); + + "01234567890123456789012345678901234567890123456789" + + "012\n", new String(data, 512, 99, CharsetNames.UTF_8)); } @Test public void testPaxHeadersWithLength101() throws Exception { final Map m = new HashMap<>(); m.put("a", - "0123456789012345678901234567890123456789" - + "01234567890123456789012345678901234567890123456789" - + "0123"); + "0123456789012345678901234567890123456789" + + "01234567890123456789012345678901234567890123456789" + + "0123"); final byte[] data = writePaxHeader(m); assertEquals("00000000145 ", - new String(data, TarConstants.NAMELEN - + TarConstants.MODELEN - + TarConstants.UIDLEN - + TarConstants.GIDLEN, 12, - CharsetNames.UTF_8)); + new String(data, TarConstants.NAMELEN + + TarConstants.MODELEN + + TarConstants.UIDLEN + + TarConstants.GIDLEN, 12, + CharsetNames.UTF_8)); assertEquals("101 a=0123456789012345678901234567890123456789" - + "01234567890123456789012345678901234567890123456789" - + "0123\n", new String(data, 512, 101, CharsetNames.UTF_8)); + + "01234567890123456789012345678901234567890123456789" + + "0123\n", new String(data, 512, 101, CharsetNames.UTF_8)); } private byte[] writePaxHeader(final Map m) throws Exception { @@ -226,7 +227,7 @@ public void testWriteLongFileNamePosixMode() throws Exception { tos.closeArchiveEntry(); final byte[] data = bos.toByteArray(); assertEquals("160 path=" + n + "\n", - new String(data, 512, 160, CharsetNames.UTF_8)); + new String(data, 512, 160, CharsetNames.UTF_8)); final TarArchiveInputStream tin = new TarArchiveInputStream(new ByteArrayInputStream(data)); final TarArchiveEntry e = tin.getNextTarEntry(); @@ -248,11 +249,11 @@ public void testOldEntryStarMode() throws Exception { tos.write(new byte[10 * 1024]); final byte[] data = bos.toByteArray(); assertEquals((byte) 0xff, - data[TarConstants.NAMELEN - + TarConstants.MODELEN - + TarConstants.UIDLEN - + TarConstants.GIDLEN - + TarConstants.SIZELEN]); + data[TarConstants.NAMELEN + + TarConstants.MODELEN + + TarConstants.UIDLEN + + TarConstants.GIDLEN + + TarConstants.SIZELEN]); final TarArchiveInputStream tin = new TarArchiveInputStream(new ByteArrayInputStream(data)); final TarArchiveEntry e = tin.getNextTarEntry(); @@ -279,13 +280,13 @@ public void testOldEntryPosixMode() throws Exception { tos.write(new byte[10 * 1024]); final byte[] data = bos.toByteArray(); assertEquals("00000000000 ", - new String(data, - 1024 + TarConstants.NAMELEN - + TarConstants.MODELEN - + TarConstants.UIDLEN - + TarConstants.GIDLEN - + TarConstants.SIZELEN, 12, - CharsetNames.UTF_8)); + new String(data, + 1024 + TarConstants.NAMELEN + + TarConstants.MODELEN + + TarConstants.UIDLEN + + TarConstants.GIDLEN + + TarConstants.SIZELEN, 12, + CharsetNames.UTF_8)); final TarArchiveInputStream tin = new TarArchiveInputStream(new ByteArrayInputStream(data)); final TarArchiveEntry e = tin.getNextTarEntry(); @@ -328,7 +329,7 @@ public void testWriteNonAsciiPathNamePaxHeader() throws Exception { tos.close(); final byte[] data = bos.toByteArray(); assertEquals("11 path=" + n + "\n", - new String(data, 512, 11, CharsetNames.UTF_8)); + new String(data, 512, 11, CharsetNames.UTF_8)); final TarArchiveInputStream tin = new TarArchiveInputStream(new ByteArrayInputStream(data)); final TarArchiveEntry e = tin.getNextTarEntry(); @@ -351,7 +352,7 @@ public void testWriteNonAsciiLinkPathNamePaxHeader() throws Exception { tos.close(); final byte[] data = bos.toByteArray(); assertEquals("15 linkpath=" + n + "\n", - new String(data, 512, 15, CharsetNames.UTF_8)); + new String(data, 512, 15, CharsetNames.UTF_8)); final TarArchiveInputStream tin = new TarArchiveInputStream(new ByteArrayInputStream(data)); final TarArchiveEntry e = tin.getNextTarEntry(); @@ -399,8 +400,8 @@ private void testRoundtripWith67CharFileName(final int mode) throws Exception { @Test public void testWriteLongDirectoryNameErrorMode() throws Exception { final String n = "01234567890123456789012345678901234567890123456789" - + "01234567890123456789012345678901234567890123456789" - + "01234567890123456789012345678901234567890123456789/"; + + "01234567890123456789012345678901234567890123456789" + + "01234567890123456789012345678901234567890123456789/"; try { final TarArchiveEntry t = new TarArchiveEntry(n); @@ -524,8 +525,8 @@ public void testWriteNonAsciiNameWithUnfortunateNamePosixMode() throws Exception @Test public void testWriteLongLinkNameErrorMode() throws Exception { final String linkname = "01234567890123456789012345678901234567890123456789" - + "01234567890123456789012345678901234567890123456789" - + "01234567890123456789012345678901234567890123456789/test"; + + "01234567890123456789012345678901234567890123456789" + + "01234567890123456789012345678901234567890123456789/test"; final TarArchiveEntry entry = new TarArchiveEntry("test", TarConstants.LF_SYMLINK); entry.setLinkName(linkname); @@ -548,7 +549,7 @@ public void testWriteLongLinkNameTruncateMode() throws Exception { final String linkname = "01234567890123456789012345678901234567890123456789" + "01234567890123456789012345678901234567890123456789" + "01234567890123456789012345678901234567890123456789/"; - final TarArchiveEntry entry = new TarArchiveEntry("test" , TarConstants.LF_SYMLINK); + final TarArchiveEntry entry = new TarArchiveEntry("test", TarConstants.LF_SYMLINK); entry.setLinkName(linkname); final ByteArrayOutputStream bos = new ByteArrayOutputStream(); @@ -607,31 +608,92 @@ private void testWriteLongLinkName(final int mode) throws Exception { tin.close(); } + @SuppressWarnings("deprecation") + @Test public void testRecordSize() throws IOException { + try { + TarArchiveOutputStream tos = + new TarArchiveOutputStream(new ByteArrayOutputStream(),512,511); + fail("should have rejected recordSize of 511"); + } catch(IllegalArgumentException e) { + // expected; + } + try { + TarArchiveOutputStream tos = + new TarArchiveOutputStream(new ByteArrayOutputStream(),512,511,null); + fail("should have rejected recordSize of 511"); + } catch(IllegalArgumentException e) { + // expected; + } + try (TarArchiveOutputStream tos = new TarArchiveOutputStream(new ByteArrayOutputStream(), + 512, 512)) { + assertEquals("recordSize",512,tos.getRecordSize()); + } + try (TarArchiveOutputStream tos = new TarArchiveOutputStream(new ByteArrayOutputStream(), + 512, 512, null)) { + assertEquals("recordSize",512,tos.getRecordSize()); + } + } @Test - public void testPadsOutputToFullBlockLength() throws Exception { + public void testBlockSizes() throws Exception { + String fileName = "/test1.xml"; + byte[] contents = getResourceContents(fileName); + testPadding(TarConstants.DEFAULT_BLKSIZE, fileName, contents); // USTAR / pre-pax + testPadding(5120, fileName, contents); // PAX default + testPadding(1<<15, fileName, contents); //PAX max + testPadding(-2, fileName, contents); // don't specify a block size -> use minimum length + try { + testPadding(511, fileName, contents); // don't specify a block size -> use minimum length + fail("should have thrown an illegal argument exception"); + } catch (IllegalArgumentException e) { + //expected + } + try { + testPadding(0, fileName, contents); // don't specify a block size -> use minimum length + fail("should have thrown an illegal argument exception"); + } catch (IllegalArgumentException e) { + //expected + } + } + + private void testPadding(int blockSize, String fileName, byte[] contents) throws IOException { final File f = File.createTempFile("commons-compress-padding", ".tar"); f.deleteOnExit(); final FileOutputStream fos = new FileOutputStream(f); - final TarArchiveOutputStream tos = new TarArchiveOutputStream(fos); - final File file1 = getFile("test1.xml"); - final TarArchiveEntry sEntry = new TarArchiveEntry(file1, file1.getName()); + final TarArchiveOutputStream tos; + if(blockSize != -2) { + tos = new TarArchiveOutputStream(fos, blockSize); + } else { + blockSize = 512; + tos = new TarArchiveOutputStream(fos); + } + TarArchiveEntry sEntry; + sEntry = new TarArchiveEntry(fileName); + sEntry.setSize(contents.length); tos.putArchiveEntry(sEntry); - final FileInputStream in = new FileInputStream(file1); - IOUtils.copy(in, tos); - in.close(); + tos.write(contents); tos.closeArchiveEntry(); tos.close(); - // test1.xml is small enough to fit into the default block size - assertEquals(TarConstants.DEFAULT_BLKSIZE, f.length()); + int fileRecordsSize = (int)Math.ceil((double) contents.length / 512)*512; + final int headerSize = 512; + final int endOfArchiveSize = 1024; + int unpaddedSize = headerSize + fileRecordsSize + endOfArchiveSize; + int paddedSize = (int)Math.ceil((double)unpaddedSize/blockSize)*blockSize; + assertEquals(paddedSize, f.length()); + } + + private byte[] getResourceContents(String name) throws IOException { + ByteArrayOutputStream bos; + try (InputStream resourceAsStream = getClass().getResourceAsStream(name)) { + bos = new ByteArrayOutputStream(); + IOUtils.copy(resourceAsStream, bos); + } + return bos.toByteArray(); } /** - * When using long file names the longLinkEntry included the - * current timestamp as the Entry modification date. This was - * never exposed to the client but it caused identical archives to + * When using long file names the longLinkEntry included the current timestamp as the Entry + * modification date. This was never exposed to the client but it caused identical archives to * have different MD5 hashes. - * - * @throws Exception */ @Test public void testLongNameMd5Hash() throws Exception { @@ -655,15 +717,18 @@ public void testLongNameMd5Hash() throws Exception { // do I still have the correct modification date? // let a second elapse so we don't get the current time Thread.sleep(1000); - final TarArchiveInputStream tarIn = new TarArchiveInputStream(new ByteArrayInputStream(archive2)); + final TarArchiveInputStream tarIn = new TarArchiveInputStream( + new ByteArrayInputStream(archive2)); final ArchiveEntry nextEntry = tarIn.getNextEntry(); assertEquals(longFileName, nextEntry.getName()); // tar archive stores modification time to second granularity only (floored) - assertEquals(modificationDate.getTime() / 1000, nextEntry.getLastModifiedDate().getTime() / 1000); + assertEquals(modificationDate.getTime() / 1000, + nextEntry.getLastModifiedDate().getTime() / 1000); tarIn.close(); } - private static byte[] createTarArchiveContainingOneDirectory(final String fname, final Date modificationDate) throws IOException { + private static byte[] createTarArchiveContainingOneDirectory(final String fname, + final Date modificationDate) throws IOException { final ByteArrayOutputStream baos = new ByteArrayOutputStream(); final TarArchiveOutputStream tarOut = new TarArchiveOutputStream(baos, 1024); tarOut.setLongFileMode(TarArchiveOutputStream.LONGFILE_GNU); From 368cb1b7520f78803772b5eff4b28622432479d8 Mon Sep 17 00:00:00 2001 From: Simon Spero Date: Thu, 22 Jun 2017 20:50:13 -0400 Subject: [PATCH 2/2] COMPRESS-407 Validate Block sizes. Detect invalid record sizes. Allow them through if a system property is set. Signed-off-by: Simon Spero --- .../archivers/tar/TarArchiveOutputStream.java | 48 ++++++++++++------- .../tar/TarArchiveOutputStreamTest.java | 5 ++ 2 files changed, 36 insertions(+), 17 deletions(-) diff --git a/src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStream.java b/src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStream.java index 864d4ad0cbd..6fd451cac76 100644 --- a/src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStream.java +++ b/src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStream.java @@ -63,6 +63,10 @@ public class TarArchiveOutputStream extends ArchiveOutputStream { public static final int BIGNUMBER_POSIX = 2; private static final int RECORD_SIZE = 512; + // property to set to allow invalid record sizes to be used. + private static final String ALLOW_INVALID_RECORD_SIZES = + "org.apache.commons.compress.archives.tar.allowInvalidRecordSizes"; + private long currSize; private String currName; private long currBytes; @@ -95,6 +99,7 @@ public class TarArchiveOutputStream extends ArchiveOutputStream { private static int BLOCK_SIZE_UNSPECIFIED = -511; + private int recordSize=RECORD_SIZE; /** * Constructor for TarInputStream. * @param os the output stream to use @@ -150,11 +155,7 @@ public TarArchiveOutputStream(final OutputStream os, final int blockSize, @Deprecated public TarArchiveOutputStream(final OutputStream os, final int blockSize, final int recordSize, final String encoding) { - this(os, blockSize, encoding); - if (recordSize != RECORD_SIZE) { - throw new IllegalArgumentException( - "Tar record size must always be 512 bytes. Attempt to set size of " + recordSize); - } + this(os, blockSize, encoding,recordSize); } @@ -168,24 +169,37 @@ public TarArchiveOutputStream(final OutputStream os, final int blockSize, */ public TarArchiveOutputStream(final OutputStream os, final int blockSize, final String encoding) { + this(os,blockSize,encoding,RECORD_SIZE); + } + + private TarArchiveOutputStream(final OutputStream os, final int blockSize, + final String encoding, int recordSize) { int realBlockSize; + + if (recordSize != RECORD_SIZE && !Boolean + .valueOf(System.getProperty(ALLOW_INVALID_RECORD_SIZES, "false"))) { + throw new IllegalArgumentException( + "Tar record size must always be 512 bytes. Attempt to set size of " + recordSize); + } + this.recordSize = recordSize; + if (BLOCK_SIZE_UNSPECIFIED == blockSize) { - realBlockSize = RECORD_SIZE; + realBlockSize = recordSize; } else { realBlockSize = blockSize; } - if(realBlockSize <=0 || realBlockSize % RECORD_SIZE != 0) { - throw new IllegalArgumentException("Block size must be a multiple of 512 bytes. Attempt to use set size of " + blockSize); + if(realBlockSize <=0 || realBlockSize % recordSize != 0) { + throw new IllegalArgumentException("Block size must be a multiple of recordSize . Attempt to set size of " + blockSize); } out = new CountingOutputStream(os); this.encoding = encoding; this.zipEncoding = ZipEncodingHelper.getZipEncoding(encoding); this.assemLen = 0; - this.assemBuf = new byte[RECORD_SIZE]; - this.recordBuf = new byte[RECORD_SIZE]; - this.recordsPerBlock = realBlockSize / RECORD_SIZE; + this.assemBuf = new byte[recordSize]; + this.recordBuf = new byte[recordSize]; + this.recordsPerBlock = realBlockSize / recordSize; } /** @@ -280,7 +294,7 @@ public void close() throws IOException { */ @Deprecated public int getRecordSize() { - return RECORD_SIZE; + return recordSize; } /** @@ -566,11 +580,11 @@ public ArchiveEntry createArchiveEntry(final File inputFile, final String entryN * @throws IOException on error */ private void writeRecord(final byte[] record) throws IOException { - if (record.length != RECORD_SIZE) { + if (record.length != recordSize) { throw new IOException("record to write has length '" + record.length + "' which is not the record size of '" - + RECORD_SIZE + "'"); + + recordSize + "'"); } out.write(record); @@ -588,14 +602,14 @@ private void writeRecord(final byte[] record) throws IOException { */ private void writeRecord(final byte[] buf, final int offset) throws IOException { - if (offset + RECORD_SIZE > buf.length) { + if (offset + recordSize > buf.length) { throw new IOException("record has length '" + buf.length + "' with offset '" + offset + "' which is less than the record size of '" - + RECORD_SIZE + "'"); + + recordSize + "'"); } - out.write(buf, offset, RECORD_SIZE); + out.write(buf, offset, recordSize); recordsWritten++; } diff --git a/src/test/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStreamTest.java b/src/test/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStreamTest.java index 497a7640ea0..eeb07722d03 100644 --- a/src/test/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStreamTest.java +++ b/src/test/java/org/apache/commons/compress/archivers/tar/TarArchiveOutputStreamTest.java @@ -624,6 +624,11 @@ private void testWriteLongLinkName(final int mode) throws Exception { } catch(IllegalArgumentException e) { // expected; } + System.setProperty("org.apache.commons.compress.archives.tar.allowInvalidRecordSizes","true"); + try (TarArchiveOutputStream tos = new TarArchiveOutputStream(new ByteArrayOutputStream(), + 511, 511, null)) { + } + System.getProperties().remove("org.apache.commons.compress.archives.tar.allowInvalidRecordSizes"); try (TarArchiveOutputStream tos = new TarArchiveOutputStream(new ByteArrayOutputStream(), 512, 512)) { assertEquals("recordSize",512,tos.getRecordSize());