Tar APIs pending feedback to address

[carlossanlop]

Initial PR changes were introduced here: #67883

The following is pending feedback I need to address (not urgent, for 8.0, mostly just additional testing coverage):

Perf improvements:

• Address stackalloc and stream-related suggestions by @stephentoub .
• Improve code that retrieves the extended attributes from the data section. Use IndexOf and related APIs suggested by Stephen.
• After IndexOfAnyExcept gets merged, consider using it: Add initial version of {Last}IndexOfAnyExcept #67941
• Improve the code from Archiving.Utils.Windows.cs Implement Tar APIs #67883 (comment)

Additional test coverage:

• Make sure that when archiving an executable, then extracting it, the executable mode bit gets properly preserved.
• Add test that reads an archive containing an unsupported entry type (no writing).
• Add test that ensures a hidden file can be used to create an entry from file.
• Verify these GNU fields are written in the data stream: AllGnuUnused = Offset + LongNames + Unused + Sparse + IsExtended + RealSize.
• Add test that ensures that a GNU archive (generated with tar tool) containing unused GNU bytes (sparse, etc) get preserved when written to another GNU archive.
• Add test to extract entries to disk one by one.
• Add test with entries containing relative segments that would cause extraction to happen outside of destination directory.
• Add test that opens one file concurrently, to prevent Allow sharing of input tar file for read #68361 from happening again.
• Add test that verifies that adding a Windows path with '' separators changes them to '/'.
• Consider enabling CA1305: Implement Tar APIs #67883 (comment)
• Add to the runtime-assets script an unarchived test with both a longlink and a longpath.
• Find out how (if possible) to add a file as a hardlink, because otherwise, it can only be created directly as an entry, not by reading it from the filesystem.
• Share WrappedStream in Common as suggested by @danmoseley .

Done:

• Address documentation suggestions, make sure all exceptions and remarks are properly filled out.
• Make sure I have full tests for individual extraction of all data types.
• Add test that advances DataStream a bit before extracting into filesystem, verify data was written starting from that position.
• Add Ustar checksum test.
• Add Pax checksum test.
• Add Gnu checksum test.
• Think of a better way to verify DateTimeOffsets in tests.
  • Ensure extended attribute timestamps atime and ctime are converted to/from decimal, not double.
• Implement async APIs: Implement Tar async APIs #70574
• Add test that reads an archive from a file with both a longlink and a longpath.
• Address chmod suggestions by @tmds : Tar, Zip: respect umask when creating files. #71647 Tar: for directories, use a default mask that has the 'x' bit. #71760
• Archives created in MacOS fail to get extracted if entries start with .\. Tar entries should be able to handle paths that begin with "./", including the root path itself #70516
• Fix failing tests around timestamps (most likely due to access time being modified, and expected value is now different). Fix Tar timestamp conversion from/to string and DateTimeOffset #71038
  • File tests: #69474, #70060, test disabled with Disable TarWriter_WriteEntry_File_Tests.Add_File in Linux #69997
  • Symbolic link tests might also have the same issue.
• Verify extended attributes in all PAX tests.
• Investigate why retrieving the devmajor and devminor with syscalls are not matching the expected values. https://github.com/dotnet/runtime/pull/67883/files#r852463720
  • Re-enable the tests that verify the values Add TAR tests that verify ExtractToFile works with special files #71988
• Add tests that verify readerOfOrigin still works as expected when using the new conversion constructor to transform an entry from one format to another. Add TarEntry conversion constructors #70325
• Allow multiple Global Extended Attributes entries.
  • Proposal: [API Proposal]: Allow multiple Global Extended Attributes entries in Tar archives with PAX format #69935
  • PR: Implement Tar Global Extended Attributes API changes #70869
• Follow up on Format detection in the reader when an archive has entries of multiple formats. SharpCompress and 7-zip handle this, we should too.
  • Proposal: [API Proposal]: TarReader should not assume same format for all entries in a tar file #69544
  • PR to rename enum: Rename TarFormat to TarEntryFormat #70313
• Add Tar conversion constructors Add TarEntry conversion constructors #70325
• Add simple tests to verify uncompressing a tar.gz works. PR: Do not read position when tar archive stream is unseekable #70178
• Fix hardlink test failure on Android/iOS/tvOS due to possible elevation requirement. Issue: Extract_HardLinkEntry_TargetInsideDirectory test from System.Formats.Tar.Tests fails on Android/iOS/tvOS #68360 - Fix: [Draft] Fix hard link creation in tar tests in Android, iOS, tvOS #69416
• Find all the stream.Read calls and make sure we read all that was expected (use similar logic to what's being done in ReadOrThrow). - This was addressed by @eerhardt in Add Stream ReadAtLeast and ReadExactly #69272
• Discuss if the TarFile extraction methods should extract something and throw at the end. - It was decided that we should throw immediately. We don't know if the archive is malformed after the error happens.
• Remove src csproj nullable enable after @eerhardt merges his PR.
• Implement p/invokes that retrieve UName and GName from uid and gid.
• Enable csproj in $(NetCoreAppCurrent)-Browser.
• Need to add the assembly to NetCoreAppLibrary.props to ensure it gets shipped as part of shared framework.
• Add a symlink test that has a link with an absolute path to a target.
• Remove errno from makedev, major and minor syscalls. They don't set it.
• Discuss if we want to merge DevMajor and DevMinor into a single public API. [API Proposal]: Merge TarEntry's DeviceMajor and DeviceMinor into a single API #68974

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

---

I received feedback in my first Tar PR #67883 that I would like to address in a follow-up PR.

• Address chmod suggestions by @tmds .
• Address stackalloc and stream-related suggestions by @stephentoub .
• Implement UName and GName from the current process' gid and uid, maybe use GetNameFromUid and (new) GetNameFromGid.
• Make sure I have full tests for individual extraction of all data types.
• Add a symlink test that has a link with an absolute path to a target.
• Make sure that when archiving an executable, then extracting it, the executable mode bit gets properly preserved.
• Add test that reads an archive containing an unsupported entry type (no writing).
• Add test that advances DataStream a bit before extracting into filesystem, verify data was written starting from that position.
• Add test that ensures a hidden file can be used to create an entry from file.
• Verify these GNU fields are written in the data stream: AllGnuUnused = Offset + LongNames + Unused + Sparse + IsExtended + RealSize.
• Add test that ensures that a GNU archive (generated with tar tool) containing unused GNU bytes (sparse, etc) get preserved when written to another GNU archive.
• Add simple tests to verify uncompressing a tar.gz works.
• Add test to extract entries to disk one by one.
• Verify extended attributes in all PAX tests.
• Add Ustar checksum test.
• Add Pax checksum test.
• Add Gnu checksum test.
• Find all the stream.Read calls and make sure we read all that was expected (use similar logic to what's being done in ReadOrThrow).
• Investigate why retrieving the devmajor and devminor with syscalls are not matching the expected values.
• Add to the runtime-assets script an unarchived test with both a longlink and a longpath.
• Add test that reads an archive from a file with both a longlink and a longpath.
• Address documentation suggestions, make sure all exceptions and remarks are properly filled out.
• Remove src csproj nullable enable after @eerhardt merges his PR.
• Find out how (if possible) to add a file as a hardlink, because otherwise, it can only be created directly as an entry, not by reading it from the filesystem.
• Think of a better way to verify DateTimeOffsets in tests.
• Implement async TarFile APIs.
• Implement async TarEntry APIs.
• Implement async TarWriter APIs.
• Implement async TarReader APIs.
• Share WrappedStream in Common as suggested by @danmoseley .
• Discuss if the TarFile extraction methods should extract something and throw at the end.
• Discuss if we want to merge DevMajor and DevMinor into a single public API.

Author:	carlossanlop
Assignees:	carlossanlop
Labels:	area-System.IO, untriaged
Milestone:	-

[Bio2hazard]

On .NET 7 Preview 4,
I've been testing the Tar APIs on some tar.gz files created on our teamcity agents (in ustar format) and ran into 2 issues:

• When using TarReader over a unseekable GZipStream it manages to read the first entry, but fails on the second one with an EndOfStreamException emitted in TarHelpers.cs line 40.
• When using TarReader over a seekable MemoryStream, it fails when it encounters a long filename that the tar format splits into a separate header. It detects it as a GNU format instead of ustar and fails with Entry 'blah' was expected to be in the GNU format, but did not have the expected version data.

Hope that helps.

[carlossanlop]

Thank you for your report, @Bio2hazard ! Any chance you can share a sample tar for each one of your experiments?

[Bio2hazard]

@carlossanlop yes I was able to reproduce it without sensitive information, thankfully. Here it is:
testarchive.tar.gz

It has 3 files in it:

• testfile1.txt which should have 5 paragraphs of lorem ipsum
• testfile3.txt which has the same content as testfile1.txt
• testing-a-verylong.filename-that-cant-fit-in-a-single.tar-header-and-will-probably-fail-to-load-correctly.txt which just contains its own filename

I was trying to benchmark it, so I'll just drop the benchmarking code in here. Goal is just to get the content byte[] for every file in the archive.

using System.Formats.Tar;
using System.IO.Compression;
using BenchmarkDotNet.Attributes;

namespace Benchmarks7;

[GcServer(true)]
[GcForce(true)]
[MemoryDiagnoser]
public class ReadFromTarGz
{
    private static byte[] fileData;

    public ReadFromTarGz()
    {
	    fileData = File.ReadAllBytes(@"testarchive.tar.gz");
    }

    [Benchmark]
    public List<byte[]> WithSharpCompress()
    {
        var ret = new List<byte[]>();
        using (var fileStream = new MemoryStream(fileData))
        using (var stream = new System.IO.Compression.GZipStream(fileStream, CompressionMode.Decompress))
        using (var tarReader = SharpCompress.Readers.Tar.TarReader.Open(stream))
        {
    	    while (tarReader.MoveToNextEntry())
    	    {
    		    var entry = tarReader.Entry;
                if (entry.IsDirectory)
    		    {
    			    continue;
    		    }

    		    byte[] entryData = new byte[entry.Size];
    		    using (var fileMemoryStream = new MemoryStream(entryData))
    		    using (var entryStream = tarReader.OpenEntryStream())
    		    {
    			    entryStream.CopyTo(fileMemoryStream);
    		    }
                ret.Add(entryData);
    	    }
        }

        return ret;
    }

    [Benchmark]
    public List<byte[]> WithSystemsFormatTar_PreDecompressed()
    {
	    var ret = new List<byte[]>();
	    using var decompressedStream = new MemoryStream();

	    using (var fileStream = new MemoryStream(fileData))
	    using (var stream = new System.IO.Compression.GZipStream(fileStream, CompressionMode.Decompress))
	    {
		    stream.CopyTo(decompressedStream);
	    }

	    decompressedStream.Position = 0;

	    using (var tarReader = new System.Formats.Tar.TarReader(decompressedStream))
	    {
		    System.Formats.Tar.TarEntry entry;
		    while ((entry = tarReader.GetNextEntry()) != null)
		    {
			    if (entry.EntryType == TarEntryType.Directory)
			    {
				    continue;
			    }

			    byte[] entryData = new byte[entry.Length];
			    using (var fileMemoryStream = new MemoryStream(entryData))
			    {
				    entry.DataStream.CopyTo(fileMemoryStream);
			    }
			    ret.Add(entryData);
		    }
	    }

	    return ret;
    }

    [Benchmark]
    public List<byte[]> WithSystemsFormatTar_StreamedDecompression()
    {
	    var ret = new List<byte[]>();
	    using (var fileStream = new MemoryStream(fileData))
	    using (var stream = new System.IO.Compression.GZipStream(fileStream, CompressionMode.Decompress))
	    using (var tarReader = new System.Formats.Tar.TarReader(stream))
	    {
		    System.Formats.Tar.TarEntry entry;
		    while ((entry = tarReader.GetNextEntry()) != null)
		    {
			    if (entry.EntryType == TarEntryType.Directory)
			    {
				    continue;
			    }

			    byte[] entryData = new byte[entry.Length];
			    using (var fileMemoryStream = new MemoryStream(entryData))
			    {
				    entry.DataStream.CopyTo(fileMemoryStream);
			    }
			    ret.Add(entryData);
		    }
	    }

	    return ret;
    }

    [Benchmark]
    public List<byte[]> WithSystemsFormatTar_StreamedDecompression_WithCopyData()
    {
	    var ret = new List<byte[]>();
	    using (var fileStream = new MemoryStream(fileData))
	    using (var stream = new System.IO.Compression.GZipStream(fileStream, CompressionMode.Decompress))
	    using (var tarReader = new System.Formats.Tar.TarReader(stream))
	    {
		    System.Formats.Tar.TarEntry entry;
		    while ((entry = tarReader.GetNextEntry(true)) != null)
		    {
			    if (entry.EntryType == TarEntryType.Directory)
			    {
				    continue;
			    }

			    byte[] entryData = new byte[entry.Length];
			    using (var fileMemoryStream = new MemoryStream(entryData))
			    {
				    entry.DataStream.CopyTo(fileMemoryStream);
			    }
			    ret.Add(entryData);
		    }
	    }

	    return ret;
    }
}

• WithSharpCompress works as expected
• WithSystemsFormatTar_PreDecompressed fails with Exception (which I'll paste at the bottom of the message)
• WithSystemsFormatTar_StreamedDecompression appears to work, but only contains a single result because of the aforementioned EndOfStreamException which gets caught internally, so it never gets to the long filename file
• WithSystemsFormatTar_StreamedDecompression_WithCopyData throws the same exception as WithSystemsFormatTar_PreDecompressed

Exception:

System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
 ---> System.FormatException: Entry 'testing-a-verylong.filename-that-cant-fit-in-a-single.tar-header-and-will-probably-fail-to-load-cor' was expected to be in the GNU format, but did not have the expected version data.
   at System.Formats.Tar.TarHeader.ReadVersionAttribute(Span`1 buffer)
   at System.Formats.Tar.TarHeader.TryGetNextHeader(Stream archiveStream, Boolean copyData)
   at System.Formats.Tar.TarReader.TryProcessGnuMetadataHeader(TarHeader header, Boolean copyData, TarHeader& finalHeader)
   at System.Formats.Tar.TarReader.TryGetNextEntryHeader(TarHeader& header, Boolean copyData)
   at System.Formats.Tar.TarReader.GetNextEntry(Boolean copyData)
   at Benchmarks7.ReadFromTarGz.WithSystemsFormatTar_PreDecompressed() in C:\Development\Benchmarks7\ReadFromTarGz.cs:line 65

I hope that helps!

[carlossanlop]

Hi again @Bio2hazard ,

What tool did you use to generate the tar file inside your gz file?

I inspected the internal tar file with the Hex Editor HxD. The magic and version metadata fields of the first 2 entries indicate the archive is in ustar format, but then it contains an unexpected GNU format entry with the TarType LongLink in the 3rd position (and then a 4th entry containing the actual data, because the LongLink entry is a metadata entry). Hence why you're getting the FormatException thrown: the reader detects that the archive is malformed for containing mixed format entries, and it is expected that the exception is thrown when attempting to read the 3rd one.

Here is what the spec has to say about the magic and version fields depending on the archive format:

For POSIX archives (ustar and pax)

     magic   Contains the magic	value "ustar" followed by a NUL	byte to	indi-
	     cate that this is a POSIX standard	archive.  Full compliance re-
	     quires the	uname and gname	fields be properly set.

     version
	     Version.  This should be "00" (two	copies of the ASCII digit
	     zero) for POSIX standard archives.

And then a few lines below it says:

     Field termination is specified slightly differently by POSIX than by pre-
     vious implementations.  The magic,	uname, and gname fields	must have a
     trailing NUL. 

For GNU

 magic   The magic field holds the five characters "ustar" followed	by a
	     space.  Note that POSIX ustar archives have a trailing null.
version
	     The version field holds a space character followed	by a null.
	     Note that POSIX ustar archives use	two copies of the ASCII	digit
	     "0".

In other words:

• For POSIX format archives (ustar or pax), the magic + version fields, which are one after the other, should look like: ustar\000.
• In the GNU format, should look like: ustar \0.

I'm surprised that SharpCompress and 7-zip are able to open this malformed archive. This tells me that they are flexible when it comes to mixed format entries. The spec does not explicitly indicate the entries should all be in the same format, but I interpreted it this way considering there are metadata entries like L, K (GNU) or x, g (PAX) which precede other entries. Also, the Unix tar CLI tool does not mix format entries, and fails to write entries with long paths and long entries if you create an archive in v7 or ustar, because those formats do not support that.

Based on the above, I see one public API structure change we should make: If archives are allowed to have mixed entries as we are seeing in your example (or in other words, should not expect all entries to be of the same format), then we would have to remove the public Format property from TarReader, and we would have to move it to the base TarEntry class. That way, the user will have to check this property from each individual entry so they know to which format they should cast the returned entry.

Note: The TarWriter receives a Format in the constructor, and this is important, particularly when creating a PAX archive, because the user should be able to add global extended attributes which only show up at the beginning of the archive. So the constructor's format argument should stay. But if the user creates a V7 or a Ustar formatted archive with the writer, and suddenly decides to write an unsupported entry like a GnuTarEntry or a PaxTarEntry with a long path or a long link, we should still throw (current behavior), to prevent the user from intermixing formats, exactly like the Unix tar CLI tool does.

[carlossanlop]

@Bio2hazard I'm really curious how you generated your archive, because even if I address the above by allowing intermixing formats, the magic and version of your entries are malformed in the 3rd entry: they indicate ustar instead of in gnu format, even though the entry type is L which is exclusive to the gnu format. And once again, I'm very surprised that SharpCompress and 7-zip were kind enough to allow it 😄 .