Tar: fix handing of null terminated fields. (#117410)

* Tar: fix handing of null terminated fields.

The current implementation is trimming spaces and nulls from the end.

Instead we need to find the terminating null from the start,
and we shouldn't trim spaces at the end.

* Fix not advancing over datastream with length 1, which caused the DataOffset_RegularFile_SecondEntry test to fail.

* Skip test cases that depend on checksum being checked.

* Simplify LimitByRemaining.

* Add test.

* Refactor SetReachedEnd.

* Throw InvalidDataException when the extended header contains invalid records.

Author: tmds

src/libraries/System.Formats.Tar/src/Resources/Strings.resx

@@ -205,4 +205,7 @@
   <data name="TarStreamSeekabilityUnsupportedCombination" xml:space="preserve">
     <value>Cannot write the unseekable data stream of entry '{0}' into an unseekable archive stream.</value>
   </data>
+  <data name="ExtHeaderInvalidRecords" xml:space="preserve">
+    <value>The extended header contains invalid records.</value>
+  </data>
 </root>

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

@@ -64,23 +64,26 @@ public override long Position
 
         public override bool CanWrite => false;
 
-        internal bool HasReachedEnd
+        private long Remaining => _endInSuperStream - _positionInSuperStream;
+
+        private int LimitByRemaining(int bufferSize) => (int)Math.Min(Remaining, bufferSize);
+
+        internal ValueTask AdvanceToEndAsync(CancellationToken cancellationToken)
         {
-            get
-            {
-                if (!_hasReachedEnd && _positionInSuperStream > _endInSuperStream)
-                {
-                    _hasReachedEnd = true;
-                }
-                return _hasReachedEnd;
-            }
-            set
-            {
-                if (value) // Don't allow revert to false
-                {
-                    _hasReachedEnd = true;
-                }
-            }
+            _hasReachedEnd = true;
+
+            long remaining = Remaining;
+            _positionInSuperStream = _endInSuperStream;
+            return TarHelpers.AdvanceStreamAsync(_superStream, remaining, cancellationToken);
+        }
+
+        internal void AdvanceToEnd()
+        {
+            _hasReachedEnd = true;
+
+            long remaining = Remaining;
+            _positionInSuperStream = _endInSuperStream;
+            TarHelpers.AdvanceStream(_superStream, remaining);
         }
 
         protected void ThrowIfDisposed()
@@ -90,7 +93,7 @@ protected void ThrowIfDisposed()
 
         private void ThrowIfBeyondEndOfStream()
         {
-            if (HasReachedEnd)
+            if (_hasReachedEnd)
             {
                 throw new EndOfStreamException();
             }
@@ -107,21 +110,12 @@ public override int Read(Span<byte> destination)
             ThrowIfDisposed();
             ThrowIfBeyondEndOfStream();
 
-            // parameter validation sent to _superStream.Read
-            int origCount = destination.Length;
-            int count = destination.Length;
-
-            if (_positionInSuperStream + count > _endInSuperStream)
-            {
-                count = (int)(_endInSuperStream - _positionInSuperStream);
-            }
-
-            Debug.Assert(count >= 0);
-            Debug.Assert(count <= origCount);
+            destination = destination[..LimitByRemaining(destination.Length)];
 
-            int ret = _superStream.Read(destination.Slice(0, count));
+            int ret = _superStream.Read(destination);
 
             _positionInSuperStream += ret;
+
             return ret;
         }
 
@@ -158,14 +152,12 @@ protected async ValueTask<int> ReadAsyncCore(Memory<byte> buffer, CancellationTo
 
             cancellationToken.ThrowIfCancellationRequested();
 
-            if (_positionInSuperStream > _endInSuperStream - buffer.Length)
-            {
-                buffer = buffer.Slice(0, (int)(_endInSuperStream - _positionInSuperStream));
-            }
+            buffer = buffer[..LimitByRemaining(buffer.Length)];
 
             int ret = await _superStream.ReadAsync(buffer, cancellationToken).ConfigureAwait(false);
 
             _positionInSuperStream += ret;
+
             return ret;
         }
 

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

@@ -5,6 +5,7 @@
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
+using System.Globalization;
 using System.IO;
 using System.Text;
 using System.Threading;
@@ -384,7 +385,7 @@ private async Task ProcessDataBlockAsync(Stream archiveStream, bool copyData, Ca
 
             // Continue with the rest of the fields that require no special checks
             TarHeader header = new(initialFormat,
-                name: TarHelpers.GetTrimmedUtf8String(buffer.Slice(FieldLocations.Name, FieldLengths.Name)),
+                name: TarHelpers.ParseUtf8String(buffer.Slice(FieldLocations.Name, FieldLengths.Name)),
                 mode: TarHelpers.ParseNumeric<int>(buffer.Slice(FieldLocations.Mode, FieldLengths.Mode)),
                 mTime: ParseAsTimestamp(buffer.Slice(FieldLocations.MTime, FieldLengths.MTime)),
                 typeFlag: (TarEntryType)buffer[FieldLocations.TypeFlag])
@@ -393,7 +394,7 @@ private async Task ProcessDataBlockAsync(Stream archiveStream, bool copyData, Ca
                 _size = size,
                 _uid = TarHelpers.ParseNumeric<int>(buffer.Slice(FieldLocations.Uid, FieldLengths.Uid)),
                 _gid = TarHelpers.ParseNumeric<int>(buffer.Slice(FieldLocations.Gid, FieldLengths.Gid)),
-                _linkName = TarHelpers.GetTrimmedUtf8String(buffer.Slice(FieldLocations.LinkName, FieldLengths.LinkName))
+                _linkName = TarHelpers.ParseUtf8String(buffer.Slice(FieldLocations.LinkName, FieldLengths.LinkName))
             };
 
             if (header._format == TarEntryFormat.Unknown)
@@ -517,8 +518,8 @@ private void ReadVersionAttribute(ReadOnlySpan<byte> buffer)
         private void ReadPosixAndGnuSharedAttributes(ReadOnlySpan<byte> buffer)
         {
             // Convert the byte arrays
-            _uName = TarHelpers.GetTrimmedUtf8String(buffer.Slice(FieldLocations.UName, FieldLengths.UName));
-            _gName = TarHelpers.GetTrimmedUtf8String(buffer.Slice(FieldLocations.GName, FieldLengths.GName));
+            _uName = TarHelpers.ParseUtf8String(buffer.Slice(FieldLocations.UName, FieldLengths.UName));
+            _gName = TarHelpers.ParseUtf8String(buffer.Slice(FieldLocations.GName, FieldLengths.GName));
 
             // DevMajor and DevMinor only have values with character devices and block devices.
             // For all other typeflags, the values in these fields are irrelevant.
@@ -560,7 +561,7 @@ private static DateTimeOffset ParseAsTimestamp(ReadOnlySpan<byte> buffer)
         // Throws if a conversion to an expected data type fails.
         private void ReadUstarAttributes(ReadOnlySpan<byte> buffer)
         {
-            _prefix = TarHelpers.GetTrimmedUtf8String(buffer.Slice(FieldLocations.Prefix, FieldLengths.Prefix));
+            _prefix = TarHelpers.ParseUtf8String(buffer.Slice(FieldLocations.Prefix, FieldLengths.Prefix));
 
             // In ustar, Prefix is used to store the *leading* path segments of
             // Name, if the full path did not fit in the Name byte array.
@@ -631,15 +632,18 @@ void ThrowSizeFieldTooLarge() =>
         // Returns a dictionary containing the extended attributes collected from the provided byte buffer.
         private void ReadExtendedAttributesFromBuffer(ReadOnlySpan<byte> buffer, string name)
         {
-            buffer = TarHelpers.TrimEndingNullsAndSpaces(buffer);
-
             while (TryGetNextExtendedAttribute(ref buffer, out string? key, out string? value))
             {
                 if (!ExtendedAttributes.TryAdd(key, value))
                 {
                     throw new InvalidDataException(SR.Format(SR.TarDuplicateExtendedAttribute, name));
                 }
             }
+
+            if (buffer.Length > 0)
+            {
+                throw new InvalidDataException(SR.Format(SR.ExtHeaderInvalidRecords));
+            }
         }
 
         // Reads the long path found in the data section of a GNU entry of type 'K' or 'L'
@@ -691,7 +695,7 @@ private async ValueTask ReadGnuLongPathDataBlockAsync(Stream archiveStream, Canc
         // Collects the GNU long path info from the buffer and sets it in the right field depending on the type flag.
         private void ReadGnuLongPathDataFromBuffer(ReadOnlySpan<byte> buffer)
         {
-            string longPath = TarHelpers.GetTrimmedUtf8String(buffer);
+            string longPath = TarHelpers.ParseUtf8String(buffer);
 
             if (_typeFlag == TarEntryType.LongLink)
             {
@@ -725,15 +729,21 @@ private static bool TryGetNextExtendedAttribute(
             }
             ReadOnlySpan<byte> line = buffer.Slice(0, newlinePos);
 
-            // Update buffer to point to the next line for the next call
-            buffer = buffer.Slice(newlinePos + 1);
-
-            // Find the end of the length and remove everything up through it.
+            // Find the end of the length
             int spacePos = line.IndexOf((byte)' ');
             if (spacePos < 0)
             {
                 return false;
             }
+
+            // Check the length matches the line length
+            ReadOnlySpan<byte> length = buffer.Slice(0, spacePos);
+            if (!int.TryParse(length, NumberStyles.None, CultureInfo.InvariantCulture, out int lengthValue) || lengthValue != (line.Length + 1))
+            {
+                return false;
+            }
+
+            // Remove the length
             line = line.Slice(spacePos + 1);
 
             // Find the equal separator.
@@ -749,6 +759,9 @@ private static bool TryGetNextExtendedAttribute(
             // Return the parsed key and value.
             key = Encoding.UTF8.GetString(keySlice);
             value = Encoding.UTF8.GetString(valueSlice);
+
+            // Update buffer to point to the next line for the next call
+            buffer = buffer.Slice(newlinePos + 1);
             return true;
         }
     }

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

@@ -240,8 +240,10 @@ internal static T ParseNumeric<T>(ReadOnlySpan<byte> buffer) where T : struct, I
         /// <summary>Parses a byte span that represents an ASCII string containing a number in octal base.</summary>
         internal static T ParseOctal<T>(ReadOnlySpan<byte> buffer) where T : struct, INumber<T>
         {
-            buffer = TrimEndingNullsAndSpaces(buffer);
-            buffer = TrimLeadingNullsAndSpaces(buffer);
+            buffer = TrimNullTerminated(buffer);
+
+            // We ignore spaces because some archives seem to have them (even though they shouldn't).
+            buffer = buffer.Trim((byte)' ');
 
             if (buffer.Length == 0)
             {
@@ -268,44 +270,23 @@ internal static T ParseOctal<T>(ReadOnlySpan<byte> buffer) where T : struct, INu
         private static void ThrowInvalidNumber() =>
             throw new InvalidDataException(SR.Format(SR.TarInvalidNumber));
 
-        // Returns the string contained in the specified buffer of bytes,
-        // in the specified encoding, removing the trailing null or space chars.
-        private static string GetTrimmedString(ReadOnlySpan<byte> buffer, Encoding encoding)
+        // Returns the null-terminated UTF8 string contained in the specified buffer.
+        internal static string ParseUtf8String(ReadOnlySpan<byte> buffer)
         {
-            buffer = TrimEndingNullsAndSpaces(buffer);
-            return buffer.IsEmpty ? string.Empty : encoding.GetString(buffer);
-        }
-
-        internal static ReadOnlySpan<byte> TrimEndingNullsAndSpaces(ReadOnlySpan<byte> buffer)
-        {
-            int trimmedLength = buffer.Length;
-            while (trimmedLength > 0 && buffer[trimmedLength - 1] is 0 or 32)
-            {
-                trimmedLength--;
-            }
-
-            return buffer.Slice(0, trimmedLength);
+            buffer = TrimNullTerminated(buffer);
+            return Encoding.UTF8.GetString(buffer);
         }
 
-        private static ReadOnlySpan<byte> TrimLeadingNullsAndSpaces(ReadOnlySpan<byte> buffer)
+        internal static ReadOnlySpan<byte> TrimNullTerminated(ReadOnlySpan<byte> buffer)
         {
-            int newStart = 0;
-            while (newStart < buffer.Length && buffer[newStart] is 0 or 32)
+            int i = buffer.IndexOf((byte)0);
+            if (i != -1)
             {
-                newStart++;
+                buffer = buffer[0..i];
             }
-
-            return buffer.Slice(newStart);
+            return buffer;
         }
 
-        // Returns the ASCII string contained in the specified buffer of bytes,
-        // removing the trailing null or space chars.
-        internal static string GetTrimmedAsciiString(ReadOnlySpan<byte> buffer) => GetTrimmedString(buffer, Encoding.ASCII);
-
-        // Returns the UTF8 string contained in the specified buffer of bytes,
-        // removing the trailing null or space chars.
-        internal static string GetTrimmedUtf8String(ReadOnlySpan<byte> buffer) => GetTrimmedString(buffer, Encoding.UTF8);
-
         // After the file contents, there may be zero or more null characters,
         // which exist to ensure the data is aligned to the record size. Skip them and
         // set the stream position to the first byte of the next entry.

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

@@ -221,17 +221,8 @@ internal void AdvanceDataStreamIfNeeded()
                     return;
                 }
 
-                if (!dataStream.HasReachedEnd)
-                {
-                    // If the user did not advance the position, we need to make sure the position
-                    // pointer is located at the beginning of the next header.
-                    if (dataStream.Position < (_previouslyReadEntry._header._size - 1))
-                    {
-                        long bytesToSkip = _previouslyReadEntry._header._size - dataStream.Position;
-                        TarHelpers.AdvanceStream(_archiveStream, bytesToSkip);
-                        dataStream.HasReachedEnd = true; // Now the pointer is beyond the limit, so any read attempts should throw
-                    }
-                }
+                dataStream.AdvanceToEnd();
+
                 TarHelpers.SkipBlockAlignmentPadding(_archiveStream, _previouslyReadEntry._header._size);
             }
         }
@@ -263,17 +254,8 @@ internal async ValueTask AdvanceDataStreamIfNeededAsync(CancellationToken cancel
                     return;
                 }
 
-                if (!dataStream.HasReachedEnd)
-                {
-                    // If the user did not advance the position, we need to make sure the position
-                    // pointer is located at the beginning of the next header.
-                    if (dataStream.Position < (_previouslyReadEntry._header._size - 1))
-                    {
-                        long bytesToSkip = _previouslyReadEntry._header._size - dataStream.Position;
-                        await TarHelpers.AdvanceStreamAsync(_archiveStream, bytesToSkip, cancellationToken).ConfigureAwait(false);
-                        dataStream.HasReachedEnd = true; // Now the pointer is beyond the limit, so any read attempts should throw
-                    }
-                }
+                await dataStream.AdvanceToEndAsync(cancellationToken).ConfigureAwait(false);
+
                 await TarHelpers.SkipBlockAlignmentPaddingAsync(_archiveStream, _previouslyReadEntry._header._size, cancellationToken).ConfigureAwait(false);
             }
         }

src/libraries/System.Formats.Tar/tests/TarReader/TarReader.File.Async.Tests.cs

@@ -6,6 +6,7 @@
 using System.IO;
 using System.IO.Compression;
 using System.Linq;
+using System.Text;
 using System.Threading.Tasks;
 using Xunit;
 
@@ -261,7 +262,8 @@ public async Task AllowSpacesInOctalFieldsAsync(string folderName, string testCa
         [InlineData("gnu-multi-hdrs")] // Multiple consecutive GNU metadata entries
         [InlineData("neg-size")] // Garbage chars
         [InlineData("invalid-go17")] // Many octal fields are all zero chars
-        [InlineData("issue11169")] // Checksum with null in the middle
+        [InlineData("issue11169")] // Extended header uses spaces instead of newlines to separate records
+        [InlineData("pax-bad-hdr-file")] // Extended header record is not terminated by newline
         [InlineData("issue10968")] // Garbage chars
         public async Task Throw_ArchivesWithRandomCharsAsync(string testCaseName)
         {
@@ -308,6 +310,32 @@ public async Task SparseEntryNotSupportedAsync(string testFolderName, string tes
             await Assert.ThrowsAsync<NotSupportedException>(async () => await reader.GetNextEntryAsync());
         }
 
+        [Fact]
+        public async Task ReaderIgnoresFieldValueAfterTrailingNullAsync()
+        {
+            // Fields in the tar archives are terminated by a trailing null.
+            // When reading these fields the reader must ignore all bytes past that null.
+
+            // Construct an archive that has a filename with some data after the trailing null.
+            const string FileName = "  filename  ";
+            const string FileNameWithDataPastTrailingNull = $"{FileName}\0nonesense";
+            using MemoryStream ms = new();
+            using (TarWriter writer = new(ms, leaveOpen: true))
+            {
+                var entry = new UstarTarEntry(TarEntryType.RegularFile, FileNameWithDataPastTrailingNull);
+                writer.WriteEntry(entry);
+            }
+            ms.Position = 0;
+            // Check the writer serialized the complete name passed to the constructor.
+            bool archiveIsExpected = ms.ToArray().IndexOf(Encoding.UTF8.GetBytes(FileNameWithDataPastTrailingNull)) != -1;
+            Assert.True(archiveIsExpected);
+
+            // Verify the reader doesn't return the data past the trailing null.
+            using TarReader reader = new(ms);
+            TarEntry firstEntry = await reader.GetNextEntryAsync();
+            Assert.Equal(FileName, firstEntry.Name);
+        }
+
         [Fact]
         public async Task DirectoryListRegularFileAndSparseAsync()
         {