Address some System.Formats.Tar TODOs (infra and syscalls) (#69107)

* Fix Design time build errors by removing src project as dependency of the test project.

* Add Browser to target platform identifiers. Ensure Browser can consume the Unix specific ArchivingUtils file too.

* Remove nullable enable from csproj since it's now default

* Use FileStream constructor with FileMode.CreateNew to detect and throw if destination file exists when creating archive.

* No error checking on syscalls that do not set errno.

* Add RDev field in FileStatus and retrieve it with stat/lstat so devmajor and devminor get their correct values.

* Simplify some File.Open calls with direct FileStream constructor calls. Simplify FileStreamOptions objects too.

* Implement and consume p/invokes to retrieve uname from uid and gname from gid.

* size_t variables should not be checked for negative values

* FileStream calls simplified to File.OpenRead

* Remove check for RDev > 0

* Use dictionary to preserve repeated unames and gnames mapped to uids and gids

* Missing documentation in pal_uid.h new PALEXPORT methods.

* Adjust syscalls to thread-safe ones. Start with stackalloc, then use loop.

* Make dicts readonly and non-nullable, use TryGetValue

* Reuse 'GetNameFromUid' from pal_networking.c, move to pal_uid.c, use similar logic for Gid method. Simplify Interop.Sys method.

* Remove unnecessary comment

Co-authored-by: Adam Sitnik <[email protected]>

* Put TargetFrameworks back in the first position of the PropertyGroup.

* Address eerhardt suggestions

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

* Clarify in pal_uid.h methods comments that new memory is returned

Co-authored-by: carlossanlop <[email protected]>
Co-authored-by: Adam Sitnik <[email protected]>

src/libraries/Common/src/Interop/Unix/System.Native/Interop.DeviceFiles.cs

@@ -23,6 +23,6 @@ internal static int CreateCharacterDevice(string pathName, uint mode, uint major
         private static partial int MkNod(string pathName, uint mode, uint major, uint minor);
 
         [LibraryImport(Libraries.SystemNative, EntryPoint = "SystemNative_GetDeviceIdentifiers", SetLastError = true)]
-        internal static unsafe partial int GetDeviceIdentifiers(ulong dev, uint* majorNumber, uint* minorNumber);
+        internal static unsafe partial void GetDeviceIdentifiers(ulong dev, uint* majorNumber, uint* minorNumber);
     }
 }

src/libraries/Common/src/Interop/Unix/System.Native/Interop.GroupNameUserName.cs

@@ -0,0 +1,35 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System.Runtime.InteropServices;
+using System.Buffers;
+using System.Text;
+using System;
+using System.Collections.Generic;
+using System.Reflection;
+
+internal static partial class Interop
+{
+    internal static partial class Sys
+    {
+        /// <summary>
+        /// Gets the group name associated to the specified group ID.
+        /// </summary>
+        /// <param name="gid">The group ID.</param>
+        /// <returns>On success, return a string with the group name. On failure, throws an IOException.</returns>
+        internal static string GetGroupName(uint gid) => GetGroupNameInternal(gid) ?? throw GetIOException(GetLastErrorInfo());
+
+        /// <summary>
+        /// Gets the user name associated to the specified user ID.
+        /// </summary>
+        /// <param name="uid">The user ID.</param>
+        /// <returns>On success, return a string with the user name. On failure, throws an IOException.</returns>
+        internal static string GetUserName(uint uid) => GetUserNameInternal(uid) ?? throw GetIOException(GetLastErrorInfo());
+
+        [LibraryImport(Libraries.SystemNative, EntryPoint = "SystemNative_GetGroupName", StringMarshalling = StringMarshalling.Utf8, SetLastError = true)]
+        private static unsafe partial string? GetGroupNameInternal(uint uid);
+
+        [LibraryImport(Libraries.SystemNative, EntryPoint = "SystemNative_GetUserName", StringMarshalling = StringMarshalling.Utf8, SetLastError = true)]
+        private static unsafe partial string? GetUserNameInternal(uint uid);
+    }
+}

src/libraries/Common/src/Interop/Unix/System.Native/Interop.Stat.cs

@@ -31,6 +31,7 @@ internal struct FileStatus
             internal long BirthTime;
             internal long BirthTimeNsec;
             internal long Dev;
+            internal long RDev;
             internal long Ino;
             internal uint UserFlags;
         }

src/libraries/System.Formats.Tar/src/System.Formats.Tar.csproj

@@ -1,6 +1,6 @@
 <Project Sdk="Microsoft.NET.Sdk">
   <PropertyGroup>
-    <TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)-Unix;$(NetCoreAppCurrent)</TargetFrameworks>
+    <TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)-Unix;$(NetCoreAppCurrent)-Browser;$(NetCoreAppCurrent)</TargetFrameworks>
     <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
   </PropertyGroup>
 
@@ -57,10 +57,14 @@
     <Compile Include="$(CommonPath)Interop\Unix\Interop.Errors.cs" Link="Common\Interop\Unix\System.Native\Interop.Errors.cs" />
     <Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.DeviceFiles.cs" Link="Common\Interop\Unix\System.Native\Interop.DeviceFiles.cs" />
     <Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.FChMod.cs" Link="Common\Interop\Unix\System.Native\Interop.FChMod.cs" />
+    <Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.GroupNameUserName.cs" Link="Common\Interop\Unix\System.Native\Interop.GroupNameUserName.cs" />
     <Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.Link.cs" Link="Common\Interop\Unix\System.Native\Interop.Link.cs" />
     <Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.MkFifo.cs" Link="Common\Interop\Unix\System.Native\Interop.MkFifo.cs" />
     <Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.Permissions.cs" Link="Common\Interop\Unix\Interop.Permissions.cs" />
     <Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.Stat.cs" Link="Common\Interop\Unix\Interop.Stat.cs" />
+  </ItemGroup>
+  <!-- Unix and Browser specific files -->
+  <ItemGroup Condition="'$(TargetPlatformIdentifier)' == 'Unix' or '$(TargetPlatformIdentifier)' == 'Browser'">
     <Compile Include="$(CommonPath)System\IO\Archiving.Utils.Unix.cs" Link="Common\System\IO\Archiving.Utils.Unix.cs" />
   </ItemGroup>
   <ItemGroup>

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

@@ -431,7 +431,7 @@ private void ExtractAsRegularFile(string destinationFileName)
         {
             Debug.Assert(!Path.Exists(destinationFileName));
 
-            FileStreamOptions fileStreamOptions = new FileStreamOptions()
+            FileStreamOptions fileStreamOptions = new()
             {
                 Access = FileAccess.Write,
                 Mode = FileMode.CreateNew,

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

@@ -72,12 +72,8 @@ public static void CreateFromDirectory(string sourceDirectoryName, string destin
                 throw new DirectoryNotFoundException(string.Format(SR.IO_PathNotFound_Path, sourceDirectoryName));
             }
 
-            if (Path.Exists(destinationFileName))
-            {
-                throw new IOException(string.Format(SR.IO_FileExists_Name, destinationFileName));
-            }
-
-            using FileStream fs = File.Create(destinationFileName, bufferSize: 0x1000, FileOptions.None);
+            // Throws if the destination file exists
+            using FileStream fs = new(destinationFileName, FileMode.CreateNew, FileAccess.Write);
 
             CreateFromDirectoryInternal(sourceDirectoryName, fs, includeBaseDirectory, leaveOpen: false);
         }
@@ -170,15 +166,7 @@ public static void ExtractToDirectory(string sourceFileName, string destinationD
                 throw new DirectoryNotFoundException(string.Format(SR.IO_PathNotFound_Path, destinationDirectoryName));
             }
 
-            FileStreamOptions fileStreamOptions = new()
-            {
-                Access = FileAccess.Read,
-                BufferSize = 0x1000,
-                Mode = FileMode.Open,
-                Share = FileShare.Read
-            };
-
-            using FileStream archive = File.Open(sourceFileName, fileStreamOptions);
+            using FileStream archive = File.OpenRead(sourceFileName);
 
             ExtractToDirectoryInternal(archive, destinationDirectoryName, overwriteFiles, leaveOpen: false);
         }

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

@@ -1,6 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Collections.Generic;
 using System.Diagnostics;
 using System.IO;
 
@@ -9,6 +10,9 @@ namespace System.Formats.Tar
     // Unix specific methods for the TarWriter class.
     public sealed partial class TarWriter : IDisposable
     {
+        private readonly Dictionary<uint, string> _userIdentifiers = new Dictionary<uint, string>();
+        private readonly Dictionary<uint, string> _groupIdentifiers = new Dictionary<uint, string>();
+
         // Unix specific implementation of the method that reads an entry from disk and writes it into the archive stream.
         partial void ReadFileFromDiskAndWriteToArchiveStreamAsEntry(string fullPath, string entryName)
         {
@@ -41,13 +45,13 @@ partial void ReadFileFromDiskAndWriteToArchiveStreamAsEntry(string fullPath, str
                 _ => throw new FormatException(string.Format(SR.TarInvalidFormat, Format)),
             };
 
-            if ((entryType is TarEntryType.BlockDevice or TarEntryType.CharacterDevice) && status.Dev > 0)
+            if (entryType is TarEntryType.BlockDevice or TarEntryType.CharacterDevice)
             {
                 uint major;
                 uint minor;
                 unsafe
                 {
-                    Interop.CheckIo(Interop.Sys.GetDeviceIdentifiers((ulong)status.Dev, &major, &minor));
+                    Interop.Sys.GetDeviceIdentifiers((ulong)status.RDev, &major, &minor);
                 }
 
                 entry._header._devMajor = (int)major;
@@ -60,12 +64,23 @@ partial void ReadFileFromDiskAndWriteToArchiveStreamAsEntry(string fullPath, str
 
             entry._header._mode = (status.Mode & 4095); // First 12 bits
 
-            entry.Uid = (int)status.Uid;
-            entry.Gid = (int)status.Gid;
+            // Uid and UName
+            entry._header._uid = (int)status.Uid;
+            if (!_userIdentifiers.TryGetValue(status.Uid, out string? uName))
+            {
+                uName = Interop.Sys.GetUserName(status.Uid);
+                _userIdentifiers.Add(status.Uid, uName);
+            }
+            entry._header._uName = uName;
 
-            // TODO: Add these p/invokes https://github.com/dotnet/runtime/issues/68230
-            entry._header._uName = "";// Interop.Sys.GetUName();
-            entry._header._gName = "";// Interop.Sys.GetGName();
+            // Gid and GName
+            entry._header._gid = (int)status.Gid;
+            if (!_groupIdentifiers.TryGetValue(status.Gid, out string? gName))
+            {
+                gName = Interop.Sys.GetGroupName(status.Gid);
+                _groupIdentifiers.Add(status.Gid, gName);
+            }
+            entry._header._gName = gName;
 
             if (entry.EntryType == TarEntryType.SymbolicLink)
             {
@@ -74,16 +89,8 @@ partial void ReadFileFromDiskAndWriteToArchiveStreamAsEntry(string fullPath, str
 
             if (entry.EntryType is TarEntryType.RegularFile or TarEntryType.V7RegularFile)
             {
-                FileStreamOptions options = new()
-                {
-                    Mode = FileMode.Open,
-                    Access = FileAccess.Read,
-                    Share = FileShare.Read,
-                    Options = FileOptions.None
-                };
-
                 Debug.Assert(entry._header._dataStream == null);
-                entry._header._dataStream = File.Open(fullPath, options);
+                entry._header._dataStream = File.OpenRead(fullPath);
             }
 
             WriteEntry(entry);

src/libraries/System.Formats.Tar/tests/System.Formats.Tar.Tests.csproj

@@ -56,6 +56,7 @@
     <Compile Include="$(CommonPath)Interop\Unix\Interop.IOErrors.cs" Link="Common\Interop\Unix\Interop.IOErrors.cs" />
     <Compile Include="$(CommonPath)Interop\Unix\Interop.Libraries.cs" Link="Common\Interop\Unix\Interop.Libraries.cs" />
     <Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.DeviceFiles.cs" Link="Common\Interop\Unix\System.Native\Interop.DeviceFiles.cs" />
+    <Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.GroupNameUserName.cs" Link="Common\Interop\Unix\System.Native\Interop.GroupNameUserName.cs" />
     <Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.Link.cs" Link="Common\Interop\Unix\System.Native\Interop.Link.cs" />
     <Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.MkFifo.cs" Link="Common\Interop\Unix\System.Native\Interop.MkFifo.cs" />
     <Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.Stat.cs" Link="Common\Interop\Unix\System.Native\Interop.Stat.cs" />
@@ -66,7 +67,4 @@
   <ItemGroup>
     <PackageReference Include="System.Formats.Tar.TestData" Version="$(SystemFormatsTarTestDataVersion)" />
   </ItemGroup>
-  <ItemGroup>
-    <ProjectReference Include="..\src\System.Formats.Tar.csproj" />
-  </ItemGroup>
 </Project>

src/libraries/System.Formats.Tar/tests/TarFile/TarFile.ExtractToDirectory.File.Tests.cs

@@ -100,10 +100,7 @@ public void Extract_Archive_File_OverwriteFalse()
 
             string filePath = Path.Join(destination.Path, "file.txt");
 
-            using (StreamWriter writer = File.CreateText(filePath))
-            {
-                writer.WriteLine("My existence should cause an exception");
-            }
+            File.Create(filePath).Dispose();
 
             Assert.Throws<IOException>(() => TarFile.ExtractToDirectory(sourceArchiveFileName, destination.Path, overwriteFiles: false));
         }

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

@@ -713,13 +713,8 @@ private void Verify_Archive_BlockDevice(PosixTarEntry blockDevice, IReadOnlyDict
             Assert.True(blockDevice.ModificationTime > DateTimeOffset.UnixEpoch);
             Assert.Equal(expectedFileName, blockDevice.Name);
             Assert.Equal(AssetUid, blockDevice.Uid);
-
-            // TODO: Figure out why the numbers don't match https://github.com/dotnet/runtime/issues/68230
-            // Assert.Equal(AssetBlockDeviceMajor, blockDevice.DeviceMajor);
-            // Assert.Equal(AssetBlockDeviceMinor, blockDevice.DeviceMinor);
-            // Remove these two temporary checks when the above is fixed
-            Assert.True(blockDevice.DeviceMajor > 0);
-            Assert.True(blockDevice.DeviceMinor > 0);
+            Assert.Equal(AssetBlockDeviceMajor, blockDevice.DeviceMajor);
+            Assert.Equal(AssetBlockDeviceMinor, blockDevice.DeviceMinor);
             Assert.Equal(AssetGName, blockDevice.GroupName);
             Assert.Equal(AssetUName, blockDevice.UserName);
 
@@ -749,13 +744,8 @@ private void Verify_Archive_CharacterDevice(PosixTarEntry characterDevice, IRead
             Assert.True(characterDevice.ModificationTime > DateTimeOffset.UnixEpoch);
             Assert.Equal(expectedFileName, characterDevice.Name);
             Assert.Equal(AssetUid, characterDevice.Uid);
-
-            // TODO: Figure out why the numbers don't match https://github.com/dotnet/runtime/issues/68230
-            //Assert.Equal(AssetBlockDeviceMajor, characterDevice.DeviceMajor);
-            //Assert.Equal(AssetBlockDeviceMinor, characterDevice.DeviceMinor);
-            // Remove these two temporary checks when the above is fixed
-            Assert.True(characterDevice.DeviceMajor > 0);
-            Assert.True(characterDevice.DeviceMinor > 0);
+            Assert.Equal(AssetCharacterDeviceMajor, characterDevice.DeviceMajor);
+            Assert.Equal(AssetCharacterDeviceMinor, characterDevice.DeviceMinor);
             Assert.Equal(AssetGName, characterDevice.GroupName);
             Assert.Equal(AssetUName, characterDevice.UserName);
 

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

@@ -99,15 +99,8 @@ protected static string GetTarFilePath(CompressionMethod compressionMethod, Test
         protected static MemoryStream GetTarMemoryStream(CompressionMethod compressionMethod, TestTarFormat format, string testCaseName)
         {
             string path = GetTarFilePath(compressionMethod, format, testCaseName);
-            FileStreamOptions options = new()
-            {
-                Access = FileAccess.Read,
-                Mode = FileMode.Open,
-                Share = FileShare.Read
-
-            };
             MemoryStream ms = new();
-            using (FileStream fs = new FileStream(path, options))
+            using (FileStream fs = File.OpenRead(path))
             {
                 fs.CopyTo(ms);
             }

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

@@ -91,10 +91,8 @@ public void Add_BlockDevice(TarFormat format)
 
                     VerifyPlatformSpecificMetadata(blockDevicePath, entry);
 
-                    // TODO: Fix how these values are collected, the numbers don't match even though https://github.com/dotnet/runtime/issues/68230
-                    // they come from stat's dev and from the major/minor syscalls
-                    // Assert.Equal(TestBlockDeviceMajor, entry.DeviceMajor);
-                    // Assert.Equal(TestBlockDeviceMinor, entry.DeviceMinor);
+                    Assert.Equal(TestBlockDeviceMajor, entry.DeviceMajor);
+                    Assert.Equal(TestBlockDeviceMinor, entry.DeviceMinor);
 
                     Assert.Null(reader.GetNextEntry());
                 }
@@ -138,10 +136,8 @@ public void Add_CharacterDevice(TarFormat format)
 
                     VerifyPlatformSpecificMetadata(characterDevicePath, entry);
 
-                    // TODO: Fix how these values are collected, the numbers don't match even though https://github.com/dotnet/runtime/issues/68230
-                    // they come from stat's dev and from the major/minor syscalls
-                    // Assert.Equal(TestCharacterDeviceMajor, entry.DeviceMajor);
-                    // Assert.Equal(TestCharacterDeviceMinor, entry.DeviceMinor);
+                    Assert.Equal(TestCharacterDeviceMajor, entry.DeviceMajor);
+                    Assert.Equal(TestCharacterDeviceMinor, entry.DeviceMinor);
 
                     Assert.Null(reader.GetNextEntry());
                 }
@@ -161,8 +157,11 @@ partial void VerifyPlatformSpecificMetadata(string filePath, TarEntry entry)
 
             if (entry is PosixTarEntry posix)
             {
-                Assert.Equal(DefaultGName, posix.GroupName);
-                Assert.Equal(DefaultUName, posix.UserName);
+                string gname = Interop.Sys.GetGroupName(status.Gid);
+                string uname = Interop.Sys.GetUserName(status.Uid);
+
+                Assert.Equal(gname, posix.GroupName);
+                Assert.Equal(uname, posix.UserName);
 
                 if (entry.EntryType is not TarEntryType.BlockDevice and not TarEntryType.CharacterDevice)
                 {

src/native/libs/System.Native/entrypoints.c

@@ -268,6 +268,8 @@ static const Entry s_sysNative[] =
     DllImportEntry(SystemNative_GetEnv)
     DllImportEntry(SystemNative_GetEnviron)
     DllImportEntry(SystemNative_FreeEnviron)
+    DllImportEntry(SystemNative_GetGroupName)
+    DllImportEntry(SystemNative_GetUserName)
 };
 
 EXTERN_C const void* SystemResolveDllImport(const char* name);

src/native/libs/System.Native/pal_io.c

@@ -190,6 +190,7 @@ c_static_assert(PAL_IN_ISDIR == IN_ISDIR);
 static void ConvertFileStatus(const struct stat_* src, FileStatus* dst)
 {
     dst->Dev = (int64_t)src->st_dev;
+    dst->RDev = (int64_t)src->st_rdev;
     dst->Ino = (int64_t)src->st_ino;
     dst->Flags = FILESTATUS_FLAGS_NONE;
     dst->Mode = (int32_t)src->st_mode;
@@ -770,23 +771,17 @@ int32_t SystemNative_SymLink(const char* target, const char* linkPath)
     return result;
 }
 
-int32_t SystemNative_GetDeviceIdentifiers(uint64_t dev, uint32_t* majorNumber, uint32_t* minorNumber)
+void SystemNative_GetDeviceIdentifiers(uint64_t dev, uint32_t* majorNumber, uint32_t* minorNumber)
 {
     dev_t castedDev = (dev_t)dev;
     *majorNumber = (uint32_t)major(castedDev);
     *minorNumber = (uint32_t)minor(castedDev);
-    return ConvertErrorPlatformToPal(errno);
 }
 
 int32_t SystemNative_MkNod(const char* pathName, uint32_t mode, uint32_t major, uint32_t minor)
 {
     dev_t dev = (dev_t)makedev(major, minor);
 
-    if (errno > 0)
-    {
-        return -1;
-    }
-
     int32_t result;
     while ((result = mknod(pathName, (mode_t)mode, dev)) < 0 && errno == EINTR);
     return result;

src/native/libs/System.Native/pal_io.h

@@ -31,6 +31,7 @@ typedef struct
     int64_t BirthTime; // time the file was created
     int64_t BirthTimeNsec; // nanosecond part
     int64_t Dev;       // ID of the device containing the file
+    int64_t RDev;      // ID of the device if it is a special file
     int64_t Ino;       // inode number of the file
     uint32_t UserFlags; // user defined flags
 } FileStatus;
@@ -543,9 +544,8 @@ PALEXPORT int32_t SystemNative_SymLink(const char* target, const char* linkPath)
 
 /**
  * Given a device ID, extracts the major and minor and components and returns them.
- * Return 0 on success; otherwise, returns -1 and errno is set.
  */
-PALEXPORT int32_t SystemNative_GetDeviceIdentifiers(uint64_t dev, uint32_t* majorNumber, uint32_t* minorNumber);
+PALEXPORT void SystemNative_GetDeviceIdentifiers(uint64_t dev, uint32_t* majorNumber, uint32_t* minorNumber);
 
 /**
  * Creates a special or ordinary file.