Skip to content

SkipBlockAlignmentPadding must be executed for all entries #74396

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

Merged
merged 1 commit into from
Aug 23, 2022
Merged

SkipBlockAlignmentPadding must be executed for all entries #74396

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 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 @@ -225,10 +225,10 @@ internal void AdvanceDataStreamIfNeeded()
{
long bytesToSkip = _previouslyReadEntry._header._size - dataStream.Position;
TarHelpers.AdvanceStream(_archiveStream, bytesToSkip);
TarHelpers.SkipBlockAlignmentPadding(_archiveStream, _previouslyReadEntry._header._size);
dataStream.HasReachedEnd = true; // Now the pointer is beyond the limit, so any read attempts should throw
}
}
TarHelpers.SkipBlockAlignmentPadding(_archiveStream, _previouslyReadEntry._header._size);
}
}

Expand Down Expand Up @@ -267,10 +267,10 @@ internal async ValueTask AdvanceDataStreamIfNeededAsync(CancellationToken cancel
{
long bytesToSkip = _previouslyReadEntry._header._size - dataStream.Position;
await TarHelpers.AdvanceStreamAsync(_archiveStream, bytesToSkip, cancellationToken).ConfigureAwait(false);
await TarHelpers.SkipBlockAlignmentPaddingAsync(_archiveStream, _previouslyReadEntry._header._size, cancellationToken).ConfigureAwait(false);
dataStream.HasReachedEnd = true; // Now the pointer is beyond the limit, so any read attempts should throw
}
}
await TarHelpers.SkipBlockAlignmentPaddingAsync(_archiveStream, _previouslyReadEntry._header._size, cancellationToken).ConfigureAwait(false);
}
}

Expand Down
32 changes: 32 additions & 0 deletions src/libraries/System.Formats.Tar/tests/TarFile/TarFile.ExtractToDirectory.Stream.Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Collections.Generic;
using System.IO;
using System.IO.Compression;
using System.Linq;
using Xunit;

Expand Down Expand Up @@ -145,5 +146,36 @@ private void Extract_LinkEntry_TargetInsideDirectory_Internal(TarEntryType entry

Assert.Equal(2, Directory.GetFileSystemEntries(baseDir).Count());
}

[Theory]
[InlineData(512)]
[InlineData(512 + 1)]
[InlineData(512 + 512 - 1)]
public void Extract_UnseekableStream_BlockAlignmentPadding_DoesNotAffectNextEntries(int contentSize)
{
byte[] fileContents = new byte[contentSize];
Array.Fill<byte>(fileContents, 0x1);

using var archive = new MemoryStream();
using (var compressor = new GZipStream(archive, CompressionMode.Compress, leaveOpen: true))
{
using var writer = new TarWriter(compressor);
var entry1 = new PaxTarEntry(TarEntryType.RegularFile, "file");
entry1.DataStream = new MemoryStream(fileContents);
writer.WriteEntry(entry1);

var entry2 = new PaxTarEntry(TarEntryType.RegularFile, "next-file");
writer.WriteEntry(entry2);
}

archive.Position = 0;
using var decompressor = new GZipStream(archive, CompressionMode.Decompress);
using var reader = new TarReader(decompressor);

using TempDirectory destination = new TempDirectory();
TarFile.ExtractToDirectory(decompressor, destination.Path, overwriteFiles: true);

Assert.Equal(2, Directory.GetFileSystemEntries(destination.Path, "*", SearchOption.AllDirectories).Count());
}
}
}
32 changes: 32 additions & 0 deletions ...ibraries/System.Formats.Tar/tests/TarFile/TarFile.ExtractToDirectoryAsync.Stream.Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Collections.Generic;
using System.IO;
using System.IO.Compression;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
Expand Down Expand Up @@ -174,5 +175,36 @@ private async Task Extract_LinkEntry_TargetInsideDirectory_Internal_Async(TarEnt
}
}
}

[Theory]
[InlineData(512)]
[InlineData(512 + 1)]
[InlineData(512 + 512 - 1)]
public async Task Extract_UnseekableStream_BlockAlignmentPadding_DoesNotAffectNextEntries_Async(int contentSize)
{
byte[] fileContents = new byte[contentSize];
Array.Fill<byte>(fileContents, 0x1);

using var archive = new MemoryStream();
using (var compressor = new GZipStream(archive, CompressionMode.Compress, leaveOpen: true))
{
using var writer = new TarWriter(compressor);
var entry1 = new PaxTarEntry(TarEntryType.RegularFile, "file");
entry1.DataStream = new MemoryStream(fileContents);
await writer.WriteEntryAsync(entry1);

var entry2 = new PaxTarEntry(TarEntryType.RegularFile, "next-file");
await writer.WriteEntryAsync(entry2);
}

archive.Position = 0;
using var decompressor = new GZipStream(archive, CompressionMode.Decompress);
using var reader = new TarReader(decompressor);

using TempDirectory destination = new TempDirectory();
await TarFile.ExtractToDirectoryAsync(decompressor, destination.Path, overwriteFiles: true);

Assert.Equal(2, Directory.GetFileSystemEntries(destination.Path, "*", SearchOption.AllDirectories).Count());
}
}
}
38 changes: 38 additions & 0 deletions src/libraries/System.Formats.Tar/tests/TarReader/TarReader.GetNextEntry.Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,5 +250,43 @@ public void GetNextEntry_UnseekableArchive_ReplaceDataStream_ExcludeFromDisposin
Assert.Equal("Substituted", streamReader.ReadLine());
}
}

[Theory]
[InlineData(512)]
[InlineData(512 + 1)]
[InlineData(512 + 512 - 1)]
public void BlockAlignmentPadding_DoesNotAffectNextEntries(int contentSize)
{
byte[] fileContents = new byte[contentSize];
Array.Fill<byte>(fileContents, 0x1);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is good. A 0x1 char is not a valid char in any of the header fields, so if the archive is not reading entries from the correct position, this char would cause an exception when collecting the header fields.


using var archive = new MemoryStream();
using (var writer = new TarWriter(archive, leaveOpen: true))
{
var entry1 = new PaxTarEntry(TarEntryType.RegularFile, "file");
entry1.DataStream = new MemoryStream(fileContents);
writer.WriteEntry(entry1);

var entry2 = new PaxTarEntry(TarEntryType.RegularFile, "next-file");
writer.WriteEntry(entry2);
}

archive.Position = 0;
using var unseekable = new WrappedStream(archive, archive.CanRead, archive.CanWrite, canSeek: false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Total nit. I would've used the arguments (archive, canRead: true, canWrite: true, canSeek: false). No need to restart the CI for this.

using var reader = new TarReader(unseekable);

TarEntry e = reader.GetNextEntry();
Assert.Equal(contentSize, e.Length);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would've added a couple other asserts when checking both entries:

Assert.Equal("file", e.Name);
Assert.Equal(TarEntryType.RegularFile, e.EntryType);

The name field starts at byte 0 of the header. But I guess GetNextEntry() would throw if we aren't reading from the right position when it encounters an unexpected character.


byte[] buffer = new byte[contentSize];
while (e.DataStream.Read(buffer) > 0) ;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would've added an Assert.NotNull(e.DataStream) before the while, but this works too. The first while loop will throw if it's null.

AssertExtensions.SequenceEqual(fileContents, buffer);

e = reader.GetNextEntry();
Assert.Equal(0, e.Length);

e = reader.GetNextEntry();
Assert.Null(e);
Comment on lines +288 to +289
Copy link
Contributor

Choose a reason for hiding this comment

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

I would usually put these together. But this is fine too. No need to restart the CI for this.

Assert.Null(reader.GetNextEntry());

And the async variant:

Assert.Null(await reader.GetNextEntryAsync());

}
}
}
38 changes: 38 additions & 0 deletions src/libraries/System.Formats.Tar/tests/TarReader/TarReader.GetNextEntryAsync.Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,5 +288,43 @@ public async Task GetNextEntry_UnseekableArchive_ReplaceDataStream_ExcludeFromDi
}
}
}

[Theory]
[InlineData(512)]
[InlineData(512 + 1)]
[InlineData(512 + 512 - 1)]
public async Task BlockAlignmentPadding_DoesNotAffectNextEntries_Async(int contentSize)
{
byte[] fileContents = new byte[contentSize];
Array.Fill<byte>(fileContents, 0x1);

using var archive = new MemoryStream();
using (var writer = new TarWriter(archive, leaveOpen: true))
{
var entry1 = new PaxTarEntry(TarEntryType.RegularFile, "file");
entry1.DataStream = new MemoryStream(fileContents);
await writer.WriteEntryAsync(entry1);

var entry2 = new PaxTarEntry(TarEntryType.RegularFile, "next-file");
await writer.WriteEntryAsync(entry2);
}

archive.Position = 0;
using var unseekable = new WrappedStream(archive, archive.CanRead, archive.CanWrite, canSeek: false);
using var reader = new TarReader(unseekable);

TarEntry e = await reader.GetNextEntryAsync();
Assert.Equal(contentSize, e.Length);

byte[] buffer = new byte[contentSize];
while (e.DataStream.Read(buffer) > 0) ;
AssertExtensions.SequenceEqual(fileContents, buffer);

e = await reader.GetNextEntryAsync();
Assert.Equal(0, e.Length);

e = await reader.GetNextEntryAsync();
Assert.Null(e);
}
}
}