Fix Tar timestamp conversion from/to string and DateTimeOffset (#71038)

* Change double to decimal in timestamp conversions to preserve precision

* Re-enable disabled test

* Adjust assert message

* Reuse FileSystemInfo Last*TimeUtc fields, use implicit cast operator to store as DateTimeOffset.

* Fix using wrong fieldname for ctime.

* Using 'G' for decimal to string conversion. Adjust test to not require a dot. The DateTimeOffset comparison done afterwards should suffice.

* Ensure timestamps are converted to long, not int.

* Add Epochalypse and Past-octal-limit timestamp tests

* TarHelpers methods can be private

* Remove unreachable code for adding ctime and atime before writing Pax entry. Add tests to ensure we always add those entries to the dictionary on construction.

* Fix typo in test when retrieving ctime.

* Make sure CTime adds nanoseconds on Unix when retrieving info from disk.

Co-authored-by: carlossanlop <carlossanlop@users.noreply.github.com>

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

@@ -273,7 +273,7 @@ private bool TryReadCommonAttributes(Span<byte> buffer)
             _mode = TarHelpers.GetTenBaseNumberFromOctalAsciiChars(buffer.Slice(FieldLocations.Mode, FieldLengths.Mode));
             _uid = TarHelpers.GetTenBaseNumberFromOctalAsciiChars(buffer.Slice(FieldLocations.Uid, FieldLengths.Uid));
             _gid = TarHelpers.GetTenBaseNumberFromOctalAsciiChars(buffer.Slice(FieldLocations.Gid, FieldLengths.Gid));
-            int mTime = TarHelpers.GetTenBaseNumberFromOctalAsciiChars(buffer.Slice(FieldLocations.MTime, FieldLengths.MTime));
+            long mTime = TarHelpers.GetTenBaseLongFromOctalAsciiChars(buffer.Slice(FieldLocations.MTime, FieldLengths.MTime));
             _mTime = TarHelpers.GetDateTimeOffsetFromSecondsSinceEpoch(mTime);
             _typeFlag = (TarEntryType)buffer[FieldLocations.TypeFlag];
             _linkName = TarHelpers.GetTrimmedUtf8String(buffer.Slice(FieldLocations.LinkName, FieldLengths.LinkName));
@@ -383,10 +383,10 @@ private void ReadPosixAndGnuSharedAttributes(Span<byte> buffer)
         private void ReadGnuAttributes(Span<byte> buffer)
         {
             // Convert byte arrays
-            int aTime = TarHelpers.GetTenBaseNumberFromOctalAsciiChars(buffer.Slice(FieldLocations.ATime, FieldLengths.ATime));
+            long aTime = TarHelpers.GetTenBaseLongFromOctalAsciiChars(buffer.Slice(FieldLocations.ATime, FieldLengths.ATime));
             _aTime = TarHelpers.GetDateTimeOffsetFromSecondsSinceEpoch(aTime);
 
-            int cTime = TarHelpers.GetTenBaseNumberFromOctalAsciiChars(buffer.Slice(FieldLocations.CTime, FieldLengths.CTime));
+            long cTime = TarHelpers.GetTenBaseLongFromOctalAsciiChars(buffer.Slice(FieldLocations.CTime, FieldLengths.CTime));
             _cTime = TarHelpers.GetDateTimeOffsetFromSecondsSinceEpoch(cTime);
 
             // TODO: Read the bytes of the currently unsupported GNU fields, in case user wants to write this entry into another GNU archive, they need to be preserved. https://github.com/dotnet/runtime/issues/68230

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

@@ -3,7 +3,6 @@
 
 using System.Collections.Generic;
 using System.Diagnostics;
-using System.Globalization;
 using System.IO;
 using System.Text;
 
@@ -390,20 +389,6 @@ private void CollectExtendedAttributesFromStandardFieldsIfNeeded()
             _extendedAttributes ??= new Dictionary<string, string>();
             _extendedAttributes.Add(PaxEaName, _name);
 
-            bool containsATime = _extendedAttributes.ContainsKey(PaxEaATime);
-            bool containsCTime = _extendedAttributes.ContainsKey(PaxEaATime);
-            if (!containsATime || !containsCTime)
-            {
-                DateTimeOffset now = DateTimeOffset.UtcNow;
-                if (!containsATime)
-                {
-                    AddTimestampAsUnixSeconds(_extendedAttributes, PaxEaATime, now);
-                }
-                if (!containsCTime)
-                {
-                    AddTimestampAsUnixSeconds(_extendedAttributes, PaxEaCTime, now);
-                }
-            }
             if (!_extendedAttributes.ContainsKey(PaxEaMTime))
             {
                 AddTimestampAsUnixSeconds(_extendedAttributes, PaxEaMTime, _mTime);

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

@@ -119,20 +119,20 @@ internal static DateTimeOffset GetDateTimeOffsetFromSecondsSinceEpoch(long secon
             new DateTimeOffset((secondsSinceUnixEpoch * TimeSpan.TicksPerSecond) + DateTime.UnixEpoch.Ticks, TimeSpan.Zero);
 
         // Converts the specified number of seconds that have passed since the Unix Epoch to a DateTimeOffset.
-        internal static DateTimeOffset GetDateTimeOffsetFromSecondsSinceEpoch(double secondsSinceUnixEpoch) =>
+        private static DateTimeOffset GetDateTimeOffsetFromSecondsSinceEpoch(decimal secondsSinceUnixEpoch) =>
             new DateTimeOffset((long)(secondsSinceUnixEpoch * TimeSpan.TicksPerSecond) + DateTime.UnixEpoch.Ticks, TimeSpan.Zero);
 
         // Converts the specified DateTimeOffset to the number of seconds that have passed since the Unix Epoch.
-        internal static double GetSecondsSinceEpochFromDateTimeOffset(DateTimeOffset dateTimeOffset) =>
-            ((double)(dateTimeOffset.UtcDateTime - DateTime.UnixEpoch).Ticks) / TimeSpan.TicksPerSecond;
+        private static decimal GetSecondsSinceEpochFromDateTimeOffset(DateTimeOffset dateTimeOffset) =>
+            ((decimal)(dateTimeOffset.UtcDateTime - DateTime.UnixEpoch).Ticks) / TimeSpan.TicksPerSecond;
 
-        // If the specified fieldName is found in the provided dictionary and it is a valid double number, returns true and sets the value in 'dateTimeOffset'.
+        // If the specified fieldName is found in the provided dictionary and it is a valid decimal number, returns true and sets the value in 'dateTimeOffset'.
         internal static bool TryGetDateTimeOffsetFromTimestampString(Dictionary<string, string>? dict, string fieldName, out DateTimeOffset dateTimeOffset)
         {
             dateTimeOffset = default;
             if (dict != null &&
                 dict.TryGetValue(fieldName, out string? value) &&
-                double.TryParse(value, NumberStyles.Any, CultureInfo.InvariantCulture, out double secondsSinceEpoch))
+                decimal.TryParse(value, NumberStyles.Any, CultureInfo.InvariantCulture, out decimal secondsSinceEpoch))
             {
                 dateTimeOffset = GetDateTimeOffsetFromSecondsSinceEpoch(secondsSinceEpoch);
                 return true;
@@ -143,8 +143,10 @@ internal static bool TryGetDateTimeOffsetFromTimestampString(Dictionary<string,
         // Converts the specified DateTimeOffset to the string representation of seconds since the Unix Epoch.
         internal static string GetTimestampStringFromDateTimeOffset(DateTimeOffset timestamp)
         {
-            double secondsSinceEpoch = GetSecondsSinceEpochFromDateTimeOffset(timestamp);
-            return secondsSinceEpoch.ToString("F9", CultureInfo.InvariantCulture); // 6 decimals, no commas
+            decimal secondsSinceEpoch = GetSecondsSinceEpochFromDateTimeOffset(timestamp);
+
+            // Use 'G' to ensure the decimals get preserved (avoid losing precision).
+            return secondsSinceEpoch.ToString("G", CultureInfo.InvariantCulture);
         }
 
         // If the specified fieldName is found in the provided dictionary and is a valid string representation of a number, returns true and sets the value in 'baseTenInteger'.
@@ -179,6 +181,14 @@ internal static int GetTenBaseNumberFromOctalAsciiChars(Span<byte> buffer)
             return string.IsNullOrEmpty(str) ? 0 : Convert.ToInt32(str, fromBase: 8);
         }
 
+        // Receives a byte array that represents an ASCII string containing a number in octal base.
+        // Converts the array to an octal base number, then transforms it to ten base and returns it.
+        internal static long GetTenBaseLongFromOctalAsciiChars(Span<byte> buffer)
+        {
+            string str = GetTrimmedAsciiString(buffer);
+            return string.IsNullOrEmpty(str) ? 0 : Convert.ToInt64(str, fromBase: 8);
+        }
+
         // 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)

src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarWriter.Unix.cs

@@ -58,9 +58,10 @@ partial void ReadFileFromDiskAndWriteToArchiveStreamAsEntry(string fullPath, str
                 entry._header._devMinor = (int)minor;
             }
 
-            entry._header._mTime = TarHelpers.GetDateTimeOffsetFromSecondsSinceEpoch(status.MTime);
-            entry._header._aTime = TarHelpers.GetDateTimeOffsetFromSecondsSinceEpoch(status.ATime);
-            entry._header._cTime = TarHelpers.GetDateTimeOffsetFromSecondsSinceEpoch(status.CTime);
+            entry._header._mTime = info.LastWriteTimeUtc;
+            entry._header._aTime = info.LastAccessTimeUtc;
+            // FileSystemInfo does not have ChangeTime, but LastWriteTime and LastAccessTime make sure to add nanoseconds, so we should do the same here
+            entry._header._cTime = DateTimeOffset.FromUnixTimeSeconds(status.CTime).AddTicks(status.CTimeNsec / 100 /* nanoseconds per tick */);
 
             entry._header._mode = (status.Mode & 4095); // First 12 bits
 

src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarWriter.Windows.cs

@@ -46,9 +46,9 @@ partial void ReadFileFromDiskAndWriteToArchiveStreamAsEntry(string fullPath, str
 
             FileSystemInfo info = attributes.HasFlag(FileAttributes.Directory) ? new DirectoryInfo(fullPath) : new FileInfo(fullPath);
 
-            entry._header._mTime = new DateTimeOffset(info.LastWriteTimeUtc);
-            entry._header._aTime = new DateTimeOffset(info.LastAccessTimeUtc);
-            entry._header._cTime = new DateTimeOffset(info.LastWriteTimeUtc); // There is no "change time" property
+            entry._header._mTime = info.LastWriteTimeUtc;
+            entry._header._aTime = info.LastAccessTimeUtc;
+            entry._header._cTime = info.LastWriteTimeUtc; // There is no "change time" property
 
             entry.Mode = DefaultWindowsMode;
 

src/libraries/System.Formats.Tar/tests/TarEntry/PaxTarEntry.Conversion.Tests.cs

@@ -61,6 +61,89 @@ public class PaxTarEntry_Conversion_Tests : TarTestsConversionBase
         [Fact]
         public void Constructor_ConversionFromGnu_CharacterDevice() => TestConstructionConversion(TarEntryType.CharacterDevice, TarEntryFormat.Gnu, TarEntryFormat.Pax);
 
+        [Theory]
+        [InlineData(TarEntryFormat.V7)]
+        [InlineData(TarEntryFormat.Ustar)]
+        [InlineData(TarEntryFormat.Pax)]
+        [InlineData(TarEntryFormat.Gnu)]
+        public void Constructor_ConversionFromV7_Write(TarEntryFormat originalEntryFormat)
+        {
+            string name = "file.txt";
+            string contents = "Hello world";
+
+            TarEntry originalEntry = InvokeTarEntryCreationConstructor(originalEntryFormat, GetTarEntryTypeForTarEntryFormat(TarEntryType.RegularFile, originalEntryFormat), name);
+
+            using MemoryStream dataStream = new MemoryStream();
+            using (StreamWriter streamWriter = new StreamWriter(dataStream, leaveOpen: true))
+            {
+                streamWriter.WriteLine(contents);
+            }
+            dataStream.Position = 0;
+            originalEntry.DataStream = dataStream;
+
+            DateTimeOffset expectedATime;
+            DateTimeOffset expectedCTime;
+
+            if (originalEntryFormat is TarEntryFormat.Pax or TarEntryFormat.Gnu)
+            {
+                // The constructor should've set the atime and ctime automatically to the same value of mtime
+                expectedATime = originalEntry.ModificationTime;
+                expectedCTime = originalEntry.ModificationTime;
+            }
+            else
+            {
+                // ustar and v7 do not have atime and ctime, so the expected values of atime and ctime should be
+                // larger than mtime, because the conversion constructor sets those values automatically
+                DateTimeOffset now = DateTimeOffset.UtcNow;
+                expectedATime = now;
+                expectedCTime = now;
+            }
+
+            TarEntry convertedEntry = InvokeTarEntryConversionConstructor(TarEntryFormat.Pax, originalEntry);
+
+            using MemoryStream archiveStream = new MemoryStream();
+            using (TarWriter writer = new TarWriter(archiveStream, leaveOpen: true))
+            {
+                writer.WriteEntry(convertedEntry);
+            }
+
+            archiveStream.Position = 0;
+            using (TarReader reader = new TarReader(archiveStream))
+            {
+                PaxTarEntry paxEntry = reader.GetNextEntry() as PaxTarEntry;
+                Assert.NotNull(paxEntry);
+                Assert.Equal(TarEntryFormat.Pax, paxEntry.Format);
+                Assert.Equal(TarEntryType.RegularFile, paxEntry.EntryType);
+                Assert.Equal(name, paxEntry.Name);
+
+                Assert.NotNull(paxEntry.DataStream);
+
+                using (StreamReader streamReader = new StreamReader(paxEntry.DataStream, leaveOpen: true))
+                {
+                    Assert.Equal(contents, streamReader.ReadLine());
+                }
+
+                // atime and ctime should've been added automatically in the conversion constructor
+                // and should not be equal to the value of mtime, which was set on the original entry constructor
+
+                Assert.Contains(PaxEaATime, paxEntry.ExtendedAttributes);
+                Assert.Contains(PaxEaCTime, paxEntry.ExtendedAttributes);
+                DateTimeOffset atime = GetDateTimeOffsetFromTimestampString(paxEntry.ExtendedAttributes[PaxEaATime]);
+                DateTimeOffset ctime = GetDateTimeOffsetFromTimestampString(paxEntry.ExtendedAttributes[PaxEaCTime]);
+
+                if (originalEntryFormat is TarEntryFormat.Pax or TarEntryFormat.Gnu)
+                {
+                    Assert.Equal(expectedATime, atime);
+                    Assert.Equal(expectedCTime, ctime);
+                }
+                else
+                {
+                    AssertExtensions.GreaterThanOrEqualTo(atime, expectedATime);
+                    AssertExtensions.GreaterThanOrEqualTo(ctime, expectedCTime);
+                }
+            }
+        }
+
         [Theory]
         [InlineData(TarEntryFormat.V7)]
         [InlineData(TarEntryFormat.Ustar)]
@@ -72,7 +155,7 @@ public void Constructor_ConversionFromV7_From_UnseekableTarReader(TarEntryFormat
             using WrappedStream wrappedSource = new WrappedStream(source, canRead: true, canWrite: false, canSeek: false);
 
             using TarReader sourceReader = new TarReader(wrappedSource, leaveOpen: true);
-            V7TarEntry v7Entry = sourceReader.GetNextEntry(copyData: false) as V7TarEntry;
+            V7TarEntry v7Entry = sourceReader.GetNextEntry(copyData: false) as V7TarEntry; // Preserve the connection to the unseekable stream
             PaxTarEntry paxEntry = new PaxTarEntry(other: v7Entry); // Convert, and avoid advancing wrappedSource position
 
             using MemoryStream destination = new MemoryStream();

src/libraries/System.Formats.Tar/tests/TarEntry/TarEntry.Conversion.Tests.Base.cs

@@ -76,14 +76,14 @@ private TarEntry GetFirstEntry(MemoryStream dataStream, TarEntryType entryType,
                 PaxTarEntry paxEntry = firstEntry as PaxTarEntry;
                 Assert.Contains("atime", paxEntry.ExtendedAttributes);
                 Assert.Contains("ctime", paxEntry.ExtendedAttributes);
-                CompareDateTimeOffsets(firstEntry.ModificationTime, GetDateTimeOffsetFromTimestampString(paxEntry.ExtendedAttributes, "atime"));
-                CompareDateTimeOffsets(firstEntry.ModificationTime, GetDateTimeOffsetFromTimestampString(paxEntry.ExtendedAttributes, "ctime"));
+                Assert.Equal(firstEntry.ModificationTime, GetDateTimeOffsetFromTimestampString(paxEntry.ExtendedAttributes, "atime"));
+                Assert.Equal(firstEntry.ModificationTime, GetDateTimeOffsetFromTimestampString(paxEntry.ExtendedAttributes, "ctime"));
             }
             else if (format is TarEntryFormat.Gnu)
             {
                 GnuTarEntry gnuEntry = firstEntry as GnuTarEntry;
-                CompareDateTimeOffsets(firstEntry.ModificationTime, gnuEntry.AccessTime);
-                CompareDateTimeOffsets(firstEntry.ModificationTime, gnuEntry.ChangeTime);
+                Assert.Equal(firstEntry.ModificationTime, gnuEntry.AccessTime);
+                Assert.Equal(firstEntry.ModificationTime, gnuEntry.ChangeTime);
             }
 
             return firstEntry;
@@ -123,18 +123,18 @@ private TarEntry ConvertAndVerifyEntry(TarEntry originalEntry, TarEntryType entr
             if (formatToConvert is TarEntryFormat.Pax)
             {
                 PaxTarEntry paxEntry = convertedEntry as PaxTarEntry;
-                DateTimeOffset actualAccessTime = GetDateTimeOffsetFromTimestampString(paxEntry.ExtendedAttributes, "atime");
-                DateTimeOffset actualChangeTime = GetDateTimeOffsetFromTimestampString(paxEntry.ExtendedAttributes, "atime");
+                DateTimeOffset actualAccessTime = GetDateTimeOffsetFromTimestampString(paxEntry.ExtendedAttributes, PaxEaATime);
+                DateTimeOffset actualChangeTime = GetDateTimeOffsetFromTimestampString(paxEntry.ExtendedAttributes, PaxEaCTime);
                 if (originalEntry.Format is TarEntryFormat.Pax or TarEntryFormat.Gnu)
                 {
                     GetExpectedTimestampsFromOriginalPaxOrGnu(originalEntry, out DateTimeOffset expectedATime, out DateTimeOffset expectedCTime);
-                    CompareDateTimeOffsets(expectedATime, actualAccessTime);
-                    CompareDateTimeOffsets(expectedCTime, actualChangeTime);
+                    Assert.Equal(expectedATime, actualAccessTime);
+                    Assert.Equal(expectedCTime, actualChangeTime);
                 }
                 else if (originalEntry.Format is TarEntryFormat.Ustar or TarEntryFormat.V7)
                 {
-                    CompareDateTimeOffsets(initialNow, actualAccessTime);
-                    CompareDateTimeOffsets(initialNow, actualChangeTime);
+                    AssertExtensions.GreaterThanOrEqualTo(actualAccessTime, initialNow);
+                    AssertExtensions.GreaterThanOrEqualTo(actualChangeTime, initialNow);
                 }
             }
 
@@ -144,13 +144,13 @@ private TarEntry ConvertAndVerifyEntry(TarEntry originalEntry, TarEntryType entr
                 if (originalEntry.Format is TarEntryFormat.Pax or TarEntryFormat.Gnu)
                 {
                     GetExpectedTimestampsFromOriginalPaxOrGnu(originalEntry, out DateTimeOffset expectedATime, out DateTimeOffset expectedCTime);
-                    CompareDateTimeOffsets(expectedATime, gnuEntry.AccessTime);
-                    CompareDateTimeOffsets(expectedCTime, gnuEntry.ChangeTime);
+                    AssertExtensions.GreaterThanOrEqualTo(gnuEntry.AccessTime, expectedATime);
+                    AssertExtensions.GreaterThanOrEqualTo(gnuEntry.ChangeTime, expectedCTime);
                 }
                 else if (originalEntry.Format is TarEntryFormat.Ustar or TarEntryFormat.V7)
                 {
-                    CompareDateTimeOffsets(initialNow, gnuEntry.AccessTime);
-                    CompareDateTimeOffsets(initialNow, gnuEntry.ChangeTime);
+                    AssertExtensions.GreaterThanOrEqualTo(gnuEntry.AccessTime, initialNow);
+                    AssertExtensions.GreaterThanOrEqualTo(gnuEntry.ChangeTime, initialNow);
                 }
             }