Resumbmit tar feedback changes, include Android build fix (#69469)

* Address some System.Formats.Tar TODOs (infra and syscalls) (#69107) Re-submitting the changes approved in PR: #69107 Addresses: #68230 Includes an extra change to prevent the android build failure with the addition of the new native call to getgrgid_r. Co-authored-by: carlossanlop <[email protected]> Co-authored-by: Adam Sitnik <[email protected]>
dotnet · Jun 2, 2022 · 82a2562 · 82a2562
1 parent 0bdf195
commit 82a2562
Show file tree

Hide file tree

Showing 21 changed files with 235 additions and 149 deletions.
diff --git a/src/libraries/Common/src/Interop/Unix/System.Native/Interop.DeviceFiles.cs b/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);
     }
 }
diff --git a/src/libraries/Common/src/Interop/Unix/System.Native/Interop.GetGroupName.cs b/src/libraries/Common/src/Interop/Unix/System.Native/Interop.GetGroupName.cs
@@ -0,0 +1,25 @@
+// 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());
+
+        [LibraryImport(Libraries.SystemNative, EntryPoint = "SystemNative_GetGroupName", StringMarshalling = StringMarshalling.Utf8, SetLastError = true)]
+        private static unsafe partial string? GetGroupNameInternal(uint uid);
+    }
+}
diff --git a/src/libraries/Common/src/Interop/Unix/System.Native/Interop.GetPwUid.cs b/src/libraries/Common/src/Interop/Unix/System.Native/Interop.GetPwUid.cs
@@ -1,6 +1,10 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System;
+using System.Diagnostics;
+using System.IO;
+using System.Runtime.CompilerServices;
 using System.Runtime.InteropServices;
 
 internal static partial class Interop
@@ -20,6 +24,75 @@ internal unsafe struct Passwd
             internal byte* Shell;
         }
 
+        /// <summary>
+        /// Gets the user name associated to the specified UID.
+        /// </summary>
+        /// <param name="uid">The user ID.</param>
+        /// <returns>On success, return a string with the user name associated to the specified UID. On failure, returns an empty string.</returns>
+        internal static unsafe string GetUserNameFromPasswd(uint uid)
+        {
+            // First try with a buffer that should suffice for 99% of cases.
+            string? username;
+            const int BufLen = Interop.Sys.Passwd.InitialBufferSize;
+            byte* stackBuf = stackalloc byte[BufLen];
+            if (TryGetUserNameFromPasswd(uid, stackBuf, BufLen, out username))
+            {
+                return username ?? string.Empty;
+            }
+
+            // Fallback to heap allocations if necessary, growing the buffer until
+            // we succeed.  TryGetUserNameFromPasswd will throw if there's an unexpected error.
+            int lastBufLen = BufLen;
+            while (true)
+            {
+                lastBufLen *= 2;
+                byte[] heapBuf = new byte[lastBufLen];
+                fixed (byte* buf = &heapBuf[0])
+                {
+                    if (TryGetUserNameFromPasswd(uid, buf, heapBuf.Length, out username))
+                    {
+                        return username ?? string.Empty;
+                    }
+                }
+            }
+        }
+
+        private static unsafe bool TryGetUserNameFromPasswd(uint uid, byte* buf, int bufLen, out string? username)
+        {
+            // Call getpwuid_r to get the passwd struct
+            Interop.Sys.Passwd passwd;
+            int error = Interop.Sys.GetPwUidR(uid, out passwd, buf, bufLen);
+
+            // If the call succeeds, give back the user name retrieved
+            if (error == 0)
+            {
+                Debug.Assert(passwd.Name != null);
+                username = Marshal.PtrToStringAnsi((IntPtr)passwd.Name);
+                return true;
+            }
+
+            // If the current user's entry could not be found, give back null,
+            // but still return true (false indicates the buffer was too small).
+            if (error == -1)
+            {
+                username = null;
+                return true;
+            }
+
+            var errorInfo = new Interop.ErrorInfo(error);
+
+            // If the call failed because the buffer was too small, return false to
+            // indicate the caller should try again with a larger buffer.
+            if (errorInfo.Error == Interop.Error.ERANGE)
+            {
+                username = null;
+                return false;
+            }
+
+            // Otherwise, fail.
+            throw new IOException(errorInfo.GetErrorMessage(), errorInfo.RawErrno);
+        }
+
         [LibraryImport(Libraries.SystemNative, EntryPoint = "SystemNative_GetPwUidR", SetLastError = false)]
         internal static unsafe partial int GetPwUidR(uint uid, out Passwd pwd, byte* buf, int bufLen);
 

diff --git a/src/libraries/Common/src/Interop/Unix/System.Native/Interop.Stat.cs b/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;
         }

diff --git a/src/libraries/System.Formats.Tar/src/System.Formats.Tar.csproj b/src/libraries/System.Formats.Tar/src/System.Formats.Tar.csproj
@@ -57,6 +57,9 @@
     <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.GetEUid.cs" Link="Common\Interop\Unix\System.Native\Interop.GetEUid.cs" />
+    <Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.GetPwUid.cs" Link="Common\Interop\Unix\System.Native\Interop.GetPwUid.cs" />
+    <Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.GetGroupName.cs" Link="Common\Interop\Unix\System.Native\Interop.GetGroupName.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" />

diff --git a/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs b/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,

diff --git a/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarFile.cs b/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);
         }

diff --git a/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarWriter.Unix.cs b/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.GetUserNameFromPasswd(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);

diff --git a/src/libraries/System.Formats.Tar/tests/System.Formats.Tar.Tests.csproj b/src/libraries/System.Formats.Tar/tests/System.Formats.Tar.Tests.csproj
@@ -56,6 +56,9 @@
     <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.GetEUid.cs" Link="Common\Interop\Unix\System.Native\Interop.GetEUid.cs" />
+    <Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.GetPwUid.cs" Link="Common\Interop\Unix\System.Native\Interop.GetPwUid.cs" />
+    <Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.GetGroupName.cs" Link="Common\Interop\Unix\System.Native\Interop.GetGroupName.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 +69,4 @@
   <ItemGroup>
     <PackageReference Include="System.Formats.Tar.TestData" Version="$(SystemFormatsTarTestDataVersion)" />
   </ItemGroup>
-  <ItemGroup>
-    <ProjectReference Include="..\src\System.Formats.Tar.csproj" />
-  </ItemGroup>
 </Project>
diff --git a/src/libraries/System.Formats.Tar/tests/TarFile/TarFile.ExtractToDirectory.File.Tests.cs b/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));
         }

diff --git a/src/libraries/System.Formats.Tar/tests/TarReader/TarReader.File.Tests.cs b/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);
 

diff --git a/src/libraries/System.Formats.Tar/tests/TarTestsBase.cs b/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);
             }

diff --git a/src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntry.File.Tests.Unix.cs b/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.GetUserNameFromPasswd(status.Uid);
+
+                Assert.Equal(gname, posix.GroupName);
+                Assert.Equal(uname, posix.UserName);
 
                 if (entry.EntryType is not TarEntryType.BlockDevice and not TarEntryType.CharacterDevice)
                 {