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

Jump to bottom

Trim file type bits from mode header #77151

Merged
merged 5 commits into from
Oct 20, 2022
Merged

Trim file type bits from mode header #77151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
db5f423
Trim file type bits from mode header
am11 Oct 18, 2022
04bf3d0
Define and use ValidUnixFileModes
am11 Oct 18, 2022
cec85bf
Rev System.Formats.Tar.TestData version
am11 Oct 18, 2022
6af0213
Move ValidUnixFileModes to shared helpers
am11 Oct 18, 2022
1e719fc
Merge branch 'main' into feature/libs/System.Formats.Tar/docker-image…
am11 Oct 19, 2022
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
6 changes: 4 additions & 2 deletions src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,10 +138,12 @@ public string LinkName
/// <remarks>The value in this field has no effect on Windows platforms.</remarks>
public UnixFileMode Mode
{
get => (UnixFileMode)_header._mode;
// Some paths do not use the setter, and we want to return valid UnixFileMode.
// This mask only keeps the least significant 12 bits.
Comment on lines +141 to +142
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if not using the setter everywhere is correct. it seems somewhat inconsistent and we could've thrown earlier.

Copy link
Member Author

@am11 am11 Oct 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Good point, I am not sure if this is intentional. I didn't wanted to go for larger refactoring at this point. There is also an existing helper:

runtime/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHelpers.Unix.cs

Line 55 in 88aea31

internal static void CreateDirectory(string fullPath, UnixFileMode? mode, SortedDictionary<string, UnixFileMode>? pendingModes)
which takes care of permissions but for some reason we don't use it in the path OP was hitting the exception.

get => (UnixFileMode)(_header._mode & (int)TarHelpers.ValidUnixFileModes);
set
{
if ((int)value is < 0 or > 4095) // 4095 in decimal is 7777 in octal
if ((value & ~TarHelpers.ValidUnixFileModes) != 0) // throw on invalid UnixFileModes
Copy link
Member

Choose a reason for hiding this comment

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

nit: There's another reference to 4095 on the tests which we could change to a const UnixFileMode ValidUnixFileModes.

runtime/src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.File.Base.Unix.cs

Line 38 in ccd9d16

UnixFileMode expectedMode = (UnixFileMode)(status.Mode & 4095); // First 12 bits

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently we don't have access to the helpers in the tests.

Copy link
Member

Choose a reason for hiding this comment

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

I was suggesting copying the const to the tests, not making the one on src accessible to tests. Is just a nit, so you can disregard it.

{
throw new ArgumentOutOfRangeException(nameof(value));
}
Expand Down
14 changes: 14 additions & 0 deletions src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,20 @@ internal static partial class TarHelpers
internal const int MaxBufferLength = 4096;
internal const long MaxSizeLength = (1L << 33) - 1; // Max value of 11 octal digits = 2^33 - 1 or 8 Gb.

internal const UnixFileMode ValidUnixFileModes =
UnixFileMode.UserRead |
UnixFileMode.UserWrite |
UnixFileMode.UserExecute |
UnixFileMode.GroupRead |
UnixFileMode.GroupWrite |
UnixFileMode.GroupExecute |
UnixFileMode.OtherRead |
UnixFileMode.OtherWrite |
UnixFileMode.OtherExecute |
UnixFileMode.StickyBit |
UnixFileMode.SetGroup |
UnixFileMode.SetUser;

// Default mode for TarEntry created for a file-type.
private const UnixFileMode DefaultFileMode =
UnixFileMode.UserRead | UnixFileMode.UserWrite |
Expand Down
3 changes: 2 additions & 1 deletion src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarWriter.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ private TarEntry ConstructEntryForWriting(string fullPath, string entryName, Fil
entry._header._aTime = TarHelpers.GetDateTimeOffsetFromSecondsSinceEpoch(status.ATime);
entry._header._cTime = TarHelpers.GetDateTimeOffsetFromSecondsSinceEpoch(status.CTime);

entry._header._mode = status.Mode & 4095; // First 12 bits
// This mask only keeps the least significant 12 bits valid for UnixFileModes
entry._header._mode = status.Mode & (int)TarHelpers.ValidUnixFileModes;

// Uid and UName
entry._header._uid = (int)status.Uid;
Expand Down
13 changes: 13 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 @@ -97,6 +97,19 @@ public async Task ExtractEntry_ManySubfolderSegments_NoPrecedingDirectoryEntries
}
}

[Fact]
public async Task ExtractEntry_DockerImageTarWithFileTypeInDirectoriesInMode_SuccessfullyExtracts_Async()
Copy link
Member

Choose a reason for hiding this comment

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

I would also include a version of this test using a compressed archive since that's what the customer reported. Also, add the sync test. But this is up to you, not a big deal tbh.

Copy link
Member Author

Choose a reason for hiding this comment

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

That will require setting up token to fetch gzipped tar archive from docker api and posting a PR like dotnet/runtime-assets#279. I don't have access to those tokens (and don't think it is super important for 7.0 minimal fix).

{
using (TempDirectory root = new TempDirectory())
{
await using MemoryStream archiveStream = GetTarMemoryStream(CompressionMethod.Uncompressed, "golang_tar", "docker-hello-world");
await TarFile.ExtractToDirectoryAsync(archiveStream, root.Path, overwriteFiles: true);

Assert.True(File.Exists(Path.Join(root.Path, "manifest.json")));
Assert.True(File.Exists(Path.Join(root.Path, "repositories")));
}
}

[Theory]
[InlineData(TarEntryType.SymbolicLink)]
[InlineData(TarEntryType.HardLink)]
Expand Down