Prevent exception when attempting to write PaxTarEntry obtained from TarReader #75237

carlossanlop · 2022-09-08T03:08:06Z

A PaxTarEntry obtained from a TarReader will have an extended attributes dictionary filled with the fields that get collected by default.

When attempting to write such entry to a TarWriter via WriteEntry(TarEntry), we collect the default extended attribute fields and store them in the dictionary. The problem is that we were using Dictionary.Add, which throws if the key already exists, and that is not the intended behavior.

The fix is to add the key using the indexer. That way, we always write the most up-to-date value to the dictionary.

We should try to get it into 7.0 as mentioned in the issue.

ghost · 2022-09-08T03:08:22Z

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

Issue Details

Fixes #75215

A PaxTarEntry obtained from a TarReader will have an extended attributes dictionary filled with the fields that get collected by default.

When attempting to write such entry to a TarWriter via WriteEntry(TarEntry), we collect the default extended attribute fields and store them in the dictionary. The problem is that we were using Dictionary.Add, which throws if the key already exists, and that is not the intended behavior.

The fix is to add the key using the indexer. That way, we always write the most up-to-date value to the dictionary.

We should try to get it into 7.0 as mentioned in the issue.

Author:	carlossanlop
Assignees:	carlossanlop
Labels:	`area-System.IO`
Milestone:	-

src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntry.File.Tests.cs

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

jozkee · 2022-09-08T19:12:05Z

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

Do you mind elaborate further why are you skipping these archives? and how do you deal with them in other tests?

For the first two, the reason is the same as the one I wrote a few lines above.

Generally speaking, these tests are collecting the unarchived files from the filesystem. We have to skip them because either some OS variants do not support the files in these test cases, or nuget is unable to pack them from runtime-assets and unpack them unchanged into the test data.

Other tests are not consuming the unarchived files, they are consuming the .tar or .tar.gz files directly, which we can iterate without interacting with the filesystem. But this test in particular needs to be able to compare with the filesystem, not with TarReader entries.

The skipped test cases aren't that important. What's important is the copy of an entry that comes from a TarReader into a new TarWriter, particularly in the PAX format, to verify the bug is fixed in the extended attributes.

this test in particular needs to be able to compare with the filesystem, not with TarReader entries.

I left a suggestion below that can help you avoid comparing against the filesystem #75237 (comment). Could that help removing all these continues?

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

src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntry.File.Tests.cs

jozkee · 2022-09-08T19:37:22Z

src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntry.File.Tests.cs

Another way you can validate the copy is by re-iterating originArchive (along with destinationArchive) and comparing as much fields as possible against the entries in destinationReader.

Why not have both tests? :)

I don't see value in extracting the entries and then using them for comparison in this case, the reported error is when you copy a Tar to another without writing to the disk. IMO, the filesystem should not be involved at all.

But if there is a bug in the way we are reading information with the Tar APIs, then comparing the items read with the first TarReader with a second TarReader will give me the same results. The utf8 case discussed above is a good example of this.

Then there should be a separate test that validates file system entries extracted to disk against tar archive entries. Said test should not exercise the "Copy tar" scenario, just "extract to disk then compare"

src/libraries/System.Formats.Tar/tests/TarTestsBase.cs

jozkee · 2022-09-08T19:45:39Z

src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs

This behavior looks like it was intentional, is it OK that we change it from "add if it was absent" to "always set"?

This CollectExtendedAttributesFromStandardFields method does exactly what other tar tools do when creating a PAX entry:

They always insert the values of name and mtime for all entry types, regardless if they fit or not in the standard field.

They insert linkname for symlinks and hardlinks if not empty, regardless if it fits or not in the standard field.

If gname or uname are set, they get added if their byte lengths are too large for the standard field.

If size does not fit in the standard field when converted to string, it gets inserted.

If the user decides to manually change the mtime before writing it into the TarWriter, then the mtime would not be updated.

I need to add more unit tests.

src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs

stephentoub · 2022-09-19T14:10:02Z

We should try to get it into 7.0 as mentioned in the issue.

This hasn't moved in 10 days. What's the plan?

carlossanlop · 2022-09-19T18:46:00Z

This hasn't moved in 10 days. What's the plan?

I had a call with @jozkee today. We have a mutual understanding now and will address the comments today.

…entry.

… or when using conversion constructor. Disallow dictionary with reserved keys when using the pax constructor that takes a dictionary. Allow user to modify reserved fields via their properties and ensure they get updated in the dictionary upon writing entry to an archive.

carlossanlop · 2022-09-20T17:34:12Z

All the apple queues are failing with an unrelated infra problem that is being investigated right now (thanks Stephen for notifying First Responders).

##[error]Git fetch failed with exit code: 128

jozkee · 2022-09-20T21:28:53Z

src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.cs

+                {
+                    if (key is PaxEaName or PaxEaSize or PaxEaMTime or PaxEaGName or PaxEaUName)
+                    {
+                        throw new ArgumentException(string.Format(SR.TarReservedExtendedAttribute, key));


Suggested change

throw new ArgumentException(string.Format(SR.TarReservedExtendedAttribute, key));

throw new ArgumentException(SR.Format(SR.TarReservedExtendedAttribute, key));

jozkee · 2022-09-20T21:37:44Z

src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.cs

+        private string _name;
        internal int _mode;
        internal int _uid;
        internal int _gid;
-        internal long _size;
-        internal DateTimeOffset _mTime;
+        private long _size;
+        private DateTimeOffset _mTime;
        internal int _checksum;
        internal TarEntryType _typeFlag;
-        internal string? _linkName;
+        private string? _linkName;

        // POSIX and GNU shared attributes

        internal string _magic;
        internal string _version;
-        internal string? _gName;
-        internal string? _uName;
+        private string? _gName;
+        private string? _uName;
        internal int _devMajor;
        internal int _devMinor;


nit: consider grouping by and documenting the fields that are written/expected in the extended attributes.

jozkee · 2022-09-20T21:45:12Z

src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.cs

+
            _format = format;
-            _name = name;
+            Name = name;


Suggested change

Name = name;

_name = name;

This can keep using the fields as all _isPaxEa* bools are false at this point. No? The same applies for the rest of the cases.

jozkee · 2022-09-20T21:49:32Z

src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.cs

+            if (!allowReservedKeys)
+            {
+                foreach ((string key, string _) in existing)
+                {
+                    if (key is PaxEaName or PaxEaSize or PaxEaMTime or PaxEaGName or PaxEaUName)
+                    {
+                        throw new ArgumentException(string.Format(SR.TarReservedExtendedAttribute, key));
+                    }
+                }
+            }
+
            _ea = new Dictionary<string, string>(existing);


you are now iterating existing twice when if (!allowReservedKeys). Can you please change it to iterate just once on that case?

jozkee · 2022-09-20T21:57:35Z

src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs


        // Used to access the data section of this entry in an unseekable file
-        private TarReader? _readerOfOrigin;
+        internal TarReader? _readerOfOrigin;


Suggested change

internal TarReader? _readerOfOrigin;

private TarReader? _readerOfOrigin;

jozkee · 2022-09-20T22:07:58Z

src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.cs

        // fields have data, we store it to avoid data loss, but we don't yet expose it publicly.
        internal byte[]? _gnuUnusedBytes;

+        internal string Name


If you create an entry and then change Name using the setter, will ExtendedAttributes["path"] return the updated value? Seems to me that it won't.

jozkee · 2022-09-20T22:15:44Z

src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs

+            if (!_isPaxEaGNameSynced && !string.IsNullOrEmpty(GName))
            {
-                TryAddStringField(ExtendedAttributes, PaxEaGName, _gName, FieldLengths.GName);
+                TryAddStringField(ExtendedAttributes, PaxEaGName, GName, FieldLengths.GName);


TryAddStringField will only change the dictionary element if Encoding.UTF8.GetByteCount(value) > maxLength and you are signaling _isPaxEaGNameSynced regardless of that condition.

jozkee · 2022-09-20T22:15:50Z

src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs

+            if (!_isPaxEaUNameSynced && !string.IsNullOrEmpty(UName))
            {
-                TryAddStringField(ExtendedAttributes, PaxEaUName, _uName, FieldLengths.UName);
+                TryAddStringField(ExtendedAttributes, PaxEaUName, UName, FieldLengths.UName);


jozkee · 2022-09-20T22:16:51Z

src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs

            }

-            if (_size > 99_999_999)
+            Size = GetTotalDataBytesToWrite();


you were not doing this before, why is it needed now?

jozkee · 2022-09-29T21:47:01Z

Closing in favor of #76404

carlossanlop added the area-System.IO label Sep 8, 2022

carlossanlop requested review from adamsitnik, danmoseley, jozkee and stephentoub September 8, 2022 03:08

carlossanlop self-assigned this Sep 8, 2022

stephentoub reviewed Sep 8, 2022

View reviewed changes

src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntry.File.Tests.cs Outdated Show resolved Hide resolved

danmoseley reviewed Sep 8, 2022

View reviewed changes

src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.File.Base.cs Outdated Show resolved Hide resolved

jozkee reviewed Sep 8, 2022

View reviewed changes

carlossanlop added 8 commits September 20, 2022 10:14

Prevent exception when collecting extended attributes before writing …

3bbdbd0

…entry.

Add tests

a2e9d66

Proper method name

b53f9c6

Tests, sync and async

69981f9

Address minor feedback suggestions.

42cfe4f

Merge conflict fix

a27de47

Escape test chars

2fdaeb3

jozkee reviewed Sep 20, 2022

View reviewed changes

carlossanlop mentioned this pull request Sep 27, 2022

Use UTF8 encoding on Tar string fields #75902

Merged

jozkee closed this Sep 29, 2022

ghost locked as resolved and limited conversation to collaborators Oct 30, 2022

carlossanlop added area-System.Formats.Tar and removed area-System.IO labels Nov 22, 2022

carlossanlop deleted the ExtendedAttributesCollect branch July 28, 2023 15:28

	throw new ArgumentException(string.Format(SR.TarReservedExtendedAttribute, key));
	throw new ArgumentException(SR.Format(SR.TarReservedExtendedAttribute, key));

	internal TarReader? _readerOfOrigin;
	private TarReader? _readerOfOrigin;

Prevent exception when attempting to write PaxTarEntry obtained from TarReader #75237

Prevent exception when attempting to write PaxTarEntry obtained from TarReader #75237

Uh oh!

Conversation

carlossanlop commented Sep 8, 2022

Uh oh!

ghost commented Sep 8, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlossanlop Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jozkee Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stephentoub commented Sep 19, 2022

Uh oh!

carlossanlop commented Sep 19, 2022

Uh oh!

carlossanlop commented Sep 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jozkee commented Sep 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

carlossanlop Sep 8, 2022 •

edited

Loading

jozkee Sep 8, 2022 •

edited

Loading