Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -720,37 +720,30 @@ static int CountDigits(int value)
// extended attributes. They get collected and saved in that dictionary, with no restrictions.
private void CollectExtendedAttributesFromStandardFieldsIfNeeded()
{
ExtendedAttributes.Add(PaxEaName, _name);
ExtendedAttributes[PaxEaName] = _name;

if (!ExtendedAttributes.ContainsKey(PaxEaMTime))
{
ExtendedAttributes.Add(PaxEaMTime, TarHelpers.GetTimestampStringFromDateTimeOffset(_mTime));
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Just pointing out some other issues in this code:

  1. someone uses the copy ctor. passing the extended attributes from the other entry.
  2. on the new entry, you set ModificationTime.
  3. pass the new entry to TarWriter.WriteEntry.

The modification time will be neglected due to this check. @carlossanlop please advice if this is something we should address in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

please advice if this is something we should address in this PR.

Yes. We must always update the dictionary value for mtime using the value from the ModificationTime property. So the if condition should be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you recall why was added in the first place?


if (!string.IsNullOrEmpty(_gName))
{
TryAddStringField(ExtendedAttributes, PaxEaGName, _gName, FieldLengths.GName);
}

if (!string.IsNullOrEmpty(_uName))
{
TryAddStringField(ExtendedAttributes, PaxEaUName, _uName, FieldLengths.UName);
}
TryAddStringField(ExtendedAttributes, PaxEaGName, _gName, FieldLengths.GName);
TryAddStringField(ExtendedAttributes, PaxEaUName, _uName, FieldLengths.UName);

if (!string.IsNullOrEmpty(_linkName))
{
ExtendedAttributes.Add(PaxEaLinkName, _linkName);
ExtendedAttributes[PaxEaLinkName] = _linkName;
}

if (_size > 99_999_999)
{
ExtendedAttributes.Add(PaxEaSize, _size.ToString());
ExtendedAttributes[PaxEaSize] = _size.ToString();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We could add an else here, in case the data stream gets reduced below the max allowed size, in which case, we can remove the PaxEaSize value from the dictionary entry.

Copy link
Contributor

Choose a reason for hiding this comment

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

In my other PR which was replaced by this one, I had a fake stream implementation that simulated having 99_999_999 length. You can reuse that to test this.


// Adds the specified string to the dictionary if it's longer than the specified max byte length.
static void TryAddStringField(Dictionary<string, string> extendedAttributes, string key, string value, int maxLength)
static void TryAddStringField(Dictionary<string, string> extendedAttributes, string key, string? value, int maxLength)
{
if (Encoding.UTF8.GetByteCount(value) > maxLength)
if (!string.IsNullOrEmpty(value) && GetUtf8TextLength(value) > maxLength)
{
extendedAttributes.Add(key, value);
}
Expand Down
15 changes: 14 additions & 1 deletion src/libraries/System.Formats.Tar/tests/TarTestsBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,7 @@ protected TarEntryType GetTarEntryTypeForTarEntryFormat(TarEntryType entryType,
return entryType;
}

protected TarEntry InvokeTarEntryCreationConstructor(TarEntryFormat targetFormat, TarEntryType entryType, string entryName)
protected static TarEntry InvokeTarEntryCreationConstructor(TarEntryFormat targetFormat, TarEntryType entryType, string entryName)
=> targetFormat switch
{
TarEntryFormat.V7 => new V7TarEntry(entryType, entryName),
Expand Down Expand Up @@ -796,5 +796,18 @@ internal enum NameCapabilities
NameAndPrefix,
Unlimited
}

internal static void WriteTarArchiveWithOneEntry(Stream s, TarEntryFormat entryFormat, TarEntryType entryType)
{
using TarWriter writer = new(s, leaveOpen: true);

TarEntry entry = InvokeTarEntryCreationConstructor(entryFormat, entryType, "foo");
if (entryType == TarEntryType.SymbolicLink)
{
entry.LinkName = "bar";
}

writer.WriteEntry(entry);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -450,5 +450,45 @@ public void WriteEntry_TooLongGroupName_Throws(TarEntryFormat entryFormat, strin

Assert.Throws<ArgumentException>("entry", () => writer.WriteEntry(entry));
}

public static IEnumerable<object[]> WriteEntry_UsingTarEntry_FromTarReader_IntoTarWriter_TheoryData()
{
foreach (var entryFormat in new[] { TarEntryFormat.V7, TarEntryFormat.Ustar, TarEntryFormat.Pax, TarEntryFormat.Gnu })
{
foreach (var entryType in new[] { entryFormat == TarEntryFormat.V7 ? TarEntryType.V7RegularFile : TarEntryType.RegularFile, TarEntryType.Directory, TarEntryType.SymbolicLink })
{
foreach (bool unseekableStream in new[] { false, true })
{
yield return new object[] { entryFormat, entryType, unseekableStream };
}
}
}
}

[Theory]
[MemberData(nameof(WriteEntry_UsingTarEntry_FromTarReader_IntoTarWriter_TheoryData))]
public void WriteEntry_UsingTarEntry_FromTarReader_IntoTarWriter(TarEntryFormat entryFormat, TarEntryType entryType, bool unseekableStream)
{
MemoryStream msSource = new();
MemoryStream msDestination = new();

WriteTarArchiveWithOneEntry(msSource, entryFormat, entryType);
msSource.Position = 0;

Stream source = new WrappedStream(msSource, msSource.CanRead, msSource.CanWrite, canSeek: !unseekableStream);
Stream destination = new WrappedStream(msDestination, msDestination.CanRead, msDestination.CanWrite, canSeek: !unseekableStream);

using (TarReader reader = new(source))
using (TarWriter writer = new(destination))
{
TarEntry entry;
while ((entry = reader.GetNextEntry()) != null)
{
writer.WriteEntry(entry);
}
}

AssertExtensions.SequenceEqual(msSource.ToArray(), msDestination.ToArray());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -379,5 +379,34 @@ public async Task WriteEntry_TooLongGroupName_Throws_Async(TarEntryFormat entryF

await Assert.ThrowsAsync<ArgumentException>("entry", () => writer.WriteEntryAsync(entry));
}

public static IEnumerable<object[]> WriteEntry_UsingTarEntry_FromTarReader_IntoTarWriter_Async_TheoryData()
=> TarWriter_WriteEntry_Tests.WriteEntry_UsingTarEntry_FromTarReader_IntoTarWriter_TheoryData();

[Theory]
[MemberData(nameof(WriteEntry_UsingTarEntry_FromTarReader_IntoTarWriter_Async_TheoryData))]
public async Task WriteEntry_UsingTarEntry_FromTarReader_IntoTarWriter_Async(TarEntryFormat entryFormat, TarEntryType entryType, bool unseekableStream)
{
using MemoryStream msSource = new();
using MemoryStream msDestination = new();

WriteTarArchiveWithOneEntry(msSource, entryFormat, entryType);
msSource.Position = 0;

Stream source = new WrappedStream(msSource, msSource.CanRead, msSource.CanWrite, canSeek: !unseekableStream);
Stream destination = new WrappedStream(msDestination, msDestination.CanRead, msDestination.CanWrite, canSeek: !unseekableStream);

await using (TarReader reader = new(source))
await using (TarWriter writer = new(destination))
{
TarEntry entry;
while ((entry = await reader.GetNextEntryAsync()) != null)
{
await writer.WriteEntryAsync(entry);
}
}

AssertExtensions.SequenceEqual(msSource.ToArray(), msDestination.ToArray());
}
}
}