Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix some corner cases in TarReader #74329

Merged
merged 1 commit into from
Aug 22, 2022
Merged

Fix some corner cases in TarReader #74329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions eng/Version.Details.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,9 +134,9 @@
<Uri>https://github.com/dotnet/runtime-assets</Uri>
<Sha>77acd39a813579e1e9b12cd98466787e7f90e059</Sha>
</Dependency>
<Dependency Name="System.Formats.Tar.TestData" Version="7.0.0-beta.22409.1">
<Dependency Name="System.Formats.Tar.TestData" Version="7.0.0-beta.22421.2">
<Uri>https://github.com/dotnet/runtime-assets</Uri>
<Sha>77acd39a813579e1e9b12cd98466787e7f90e059</Sha>
<Sha>9d8fad5f0614bee808083308a3729084b681f7e7</Sha>
</Dependency>
<Dependency Name="System.IO.Compression.TestData" Version="7.0.0-beta.22409.1">
<Uri>https://github.com/dotnet/runtime-assets</Uri>
Expand Down
2 changes: 1 addition & 1 deletion eng/Versions.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@
<SystemRuntimeNumericsTestDataVersion>7.0.0-beta.22409.1</SystemRuntimeNumericsTestDataVersion>
<SystemComponentModelTypeConverterTestDataVersion>7.0.0-beta.22409.1</SystemComponentModelTypeConverterTestDataVersion>
<SystemDrawingCommonTestDataVersion>7.0.0-beta.22409.1</SystemDrawingCommonTestDataVersion>
<SystemFormatsTarTestDataVersion>7.0.0-beta.22409.1</SystemFormatsTarTestDataVersion>
<SystemFormatsTarTestDataVersion>7.0.0-beta.22421.2</SystemFormatsTarTestDataVersion>
<SystemIOCompressionTestDataVersion>7.0.0-beta.22409.1</SystemIOCompressionTestDataVersion>
<SystemIOPackagingTestDataVersion>7.0.0-beta.22409.1</SystemIOPackagingTestDataVersion>
<SystemNetTestDataVersion>7.0.0-beta.22409.1</SystemNetTestDataVersion>
Expand Down
26 changes: 22 additions & 4 deletions src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Read.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,18 +461,36 @@ private void ReadVersionAttribute(Span<byte> buffer)
// The POSIX formats have a 6 byte Magic "ustar\0", followed by a 2 byte Version "00"
if (!version.SequenceEqual(UstarVersionBytes))
{
throw new FormatException(string.Format(SR.TarPosixFormatExpected, _name));
// Check for gnu version header for mixed case
if (!version.SequenceEqual(GnuVersionBytes))
{
throw new FormatException(string.Format(SR.TarPosixFormatExpected, _name));
}

_version = GnuVersion;
}
else
{
_version = UstarVersion;
}
_version = UstarVersion;
break;

case TarEntryFormat.Gnu:
// The GNU format has a Magic+Version 8 byte string "ustar \0"
if (!version.SequenceEqual(GnuVersionBytes))
{
throw new FormatException(string.Format(SR.TarGnuFormatExpected, _name));
// Check for ustar or pax version header for mixed case
if (!version.SequenceEqual(UstarVersionBytes))
{
throw new FormatException(string.Format(SR.TarGnuFormatExpected, _name));
}

_version = UstarVersion;
}
else
{
_version = GnuVersion;
}
_version = GnuVersion;
break;

default:
Expand Down
10 changes: 10 additions & 0 deletions src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,11 @@ public async ValueTask DisposeAsync()
TarEntryFormat.V7 or TarEntryFormat.Unknown or _ => new V7TarEntry(header, this),
};

if (_archiveStream.CanSeek && _archiveStream.Length == _archiveStream.Position)
{
_reachedEndMarkers = true;
}

_previouslyReadEntry = entry;
PreserveDataStreamForDisposalIfNeeded(entry);
return entry;
Expand Down Expand Up @@ -291,6 +296,11 @@ internal async ValueTask AdvanceDataStreamIfNeededAsync(CancellationToken cancel
TarEntryFormat.V7 or TarEntryFormat.Unknown or _ => new V7TarEntry(header, this),
};

if (_archiveStream.CanSeek && _archiveStream.Length == _archiveStream.Position)
{
_reachedEndMarkers = true;
}

_previouslyReadEntry = entry;
PreserveDataStreamForDisposalIfNeeded(entry);
return entry;
Expand Down
26 changes: 26 additions & 0 deletions src/libraries/System.Formats.Tar/tests/TarReader/TarReader.File.Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,5 +124,31 @@ public void Read_Archive_LongFileName_Over100_Under255(TarEntryFormat format, Te
[InlineData(TarEntryFormat.Gnu, TestTarFormat.oldgnu)]
public void Read_Archive_LongPath_Over255(TarEntryFormat format, TestTarFormat testFormat) =>
Read_Archive_LongPath_Over255_Internal(format, testFormat);

[Fact]
public void Read_NodeTarArchives_Successfully()
{
string nodeTarPath = Path.Join(Directory.GetCurrentDirectory(), "tar", "node-tar");
foreach (string file in Directory.EnumerateFiles(nodeTarPath, "*.tar", SearchOption.AllDirectories))
{
using FileStream sourceStream = File.Open(file, FileMode.Open, FileAccess.Read, FileShare.Read);
using var reader = new TarReader(sourceStream);

TarEntry? entry = null;
while (true)
{
Exception ex = Record.Exception(() => entry = reader.GetNextEntry());
Copy link
Member

Choose a reason for hiding this comment

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

First time I see Record.Exception being used.

What advantage does it have over simply letting this throw?

Copy link
Member Author

@am11 am11 Aug 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The intent of Record.Exception() followed by Assert.Null() is to be explicit about asserting that 'the method call is not throwing with this fix' (which were throwing without the fix). It also avoids seemingly unused statements to be optimized out by the JIT.

Copy link
Member

Choose a reason for hiding this comment

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

If you're consuming or asserting something about entry, Name, and Type the jit cannot remove them, and you're implicitly testing that they don't throw.

Eg you can assert that Name is never null, and that if type is Directory then length is zero.

Copy link
Member

Choose a reason for hiding this comment

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

Eg you could do more or less what the test code did and then you won't need to record the exception explicitly

var ms = new MemoryStream();               
Assert.NotEmpty(entry.Name);                     
Assert.True(Enum.IsDefined(entry.EntryType));               
Assert.True(Enum.IsDefined(entry.Format));         
if (entry.EntryType == TarEntryType.Directory)
    continue;
var ds = entry.DataStream;
                       
if (ds != null && ds.Length > 0)
{                            
    ds.CopyTo(ms);
}

Assert.Null(ex);

if (entry is null) break;

ex = Record.Exception(() => entry.Name);
Assert.Null(ex);

ex = Record.Exception(() => entry.Length);
Assert.Null(ex);
}
Copy link
Member

Choose a reason for hiding this comment

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

Read content into a MemoryStream perhaps, as I did in my test code?

}
}
}
}