[release/7.0] Fix TarWriter.TarEntry exception when adding file whose…

… Unix group owner does not exist in the system (#81139) * Do not throw if a unix group is non-existent when creating a TarEntry from a file using TarWriter. * Adjust test that was consuming removed interop method. * Add unit tests to add file entry whose group owner does not exist. * Address src feedback. * Address test feedback * Delay creation/deletion of group in test * Test all PosixTarEntry formats * Include not-yet-backported property name change to fix build failure. --------- Co-authored-by: carlossanlop <[email protected]> Co-authored-by: carlossanlop <[email protected]>
dotnet · Feb 8, 2023 · 13a9019 · 13a9019
1 parent c67e577
commit 13a9019
Show file tree

Hide file tree

Showing 7 changed files with 160 additions and 17 deletions.
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
@@ -7,19 +7,26 @@
 using System;
 using System.Collections.Generic;
 using System.Reflection;
+using System.IO;
+using System.Diagnostics.CodeAnalysis;
 
 internal static partial class Interop
 {
     internal static partial class Sys
     {
         /// <summary>
-        /// Gets the group name associated to the specified group ID.
+        /// Tries to get 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());
+        /// <param name="groupName">When this method returns true, gets the value of the group name associated with the specified id. On failure, it is null.</param>
+        /// <returns>On success, returns true. On failure, returns false.</returns>
+        internal static bool TryGetGroupName(uint gid, [NotNullWhen(returnValue: true)] out string? groupName)
+        {
+            groupName = GetGroupName(gid);
+            return groupName != null;
+        }
 
         [LibraryImport(Libraries.SystemNative, EntryPoint = "SystemNative_GetGroupName", StringMarshalling = StringMarshalling.Utf8, SetLastError = true)]
-        private static unsafe partial string? GetGroupNameInternal(uint uid);
+        private static unsafe partial string? GetGroupName(uint uid);
     }
 }
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
@@ -82,8 +82,10 @@ private TarEntry ConstructEntryForWriting(string fullPath, string entryName, Fil
             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);
+                if (Interop.Sys.TryGetGroupName(status.Gid, out gName))
+                {
+                    _groupIdentifiers.Add(status.Gid, gName);
+                }
             }
             entry._header._gName = gName;
 

diff --git a/src/libraries/System.Formats.Tar/tests/TarEntry/TarEntry.ExtractToFile.Tests.Unix.cs b/src/libraries/System.Formats.Tar/tests/TarEntry/TarEntry.ExtractToFile.Tests.Unix.cs
@@ -23,7 +23,7 @@ public static IEnumerable<object[]> GetFormatsAndSpecialFiles()
             }
         }
 
-        [ConditionalTheory(nameof(IsRemoteExecutorSupportedAndOnUnixAndSuperUser))]
+        [ConditionalTheory(nameof(IsRemoteExecutorSupportedAndPrivilegedProcess))]
         [MemberData(nameof(GetFormatsAndSpecialFiles))]
         public void Extract_SpecialFiles(TarEntryFormat format, TarEntryType entryType)
         {
@@ -36,7 +36,7 @@ public void Extract_SpecialFiles(TarEntryFormat format, TarEntryType entryType)
             Verify_Extract_SpecialFiles(destination, entry, entryType);
         }
 
-        [ConditionalTheory(nameof(IsRemoteExecutorSupportedAndOnUnixAndSuperUser))]
+        [ConditionalTheory(nameof(IsRemoteExecutorSupportedAndPrivilegedProcess))]
         [MemberData(nameof(GetFormatsAndSpecialFiles))]
         public async Task Extract_SpecialFiles_Async(TarEntryFormat format, TarEntryType entryType)
         {

diff --git a/src/libraries/System.Formats.Tar/tests/TarTestsBase.cs b/src/libraries/System.Formats.Tar/tests/TarTestsBase.cs
@@ -12,6 +12,8 @@ namespace System.Formats.Tar.Tests
 {
     public abstract partial class TarTestsBase : FileCleanupTestBase
     {
+        protected static bool IsRemoteExecutorSupportedAndPrivilegedProcess => RemoteExecutor.IsSupported && PlatformDetection.IsUnixAndSuperUser;
+
         protected const string InitialEntryName = "InitialEntryName.ext";
         protected readonly string ModifiedEntryName = "ModifiedEntryName.ext";
 
@@ -208,7 +210,6 @@ public enum TestTarFormat
             // GNU formatted files. Format used by GNU tar versions up to 1.13.25.
             gnu
         }
-        protected static bool IsRemoteExecutorSupportedAndOnUnixAndSuperUser => RemoteExecutor.IsSupported && PlatformDetection.IsUnixAndSuperUser;
 
         protected static bool IsUnixButNotSuperUser => !PlatformDetection.IsWindows && !PlatformDetection.IsSuperUser;
 
@@ -707,7 +708,7 @@ internal static IEnumerable<string> GetNamesNonAsciiTestData(NameCapabilities ma
             // this is 256 but is supported because prefix is not required to end in separator.
             yield return Repeat(OneByteCharacter, 155) + Separator + Repeat(OneByteCharacter, 100);
 
-            // non-ascii prefix + name 
+            // non-ascii prefix + name
             yield return Repeat(TwoBytesCharacter, 155 / 2) + Separator + Repeat(OneByteCharacter, 100);
             yield return Repeat(FourBytesCharacter, 155 / 4) + Separator + Repeat(OneByteCharacter, 100);
 

diff --git a/src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.File.Base.Unix.cs b/src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.File.Base.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.Diagnostics;
 using System.IO;
 using Xunit;
 
@@ -20,7 +21,7 @@ protected void VerifyPlatformSpecificMetadata(string filePath, TarEntry entry)
 
             if (entry is PosixTarEntry posix)
             {
-                string gname = Interop.Sys.GetGroupName(status.Gid);
+                Assert.True(Interop.Sys.TryGetGroupName(status.Gid, out string gname));
                 string uname = Interop.Sys.GetUserNameFromPasswd(status.Uid);
 
                 Assert.Equal(gname, posix.GroupName);
@@ -51,5 +52,49 @@ protected void VerifyPlatformSpecificMetadata(string filePath, TarEntry entry)
                 }
             }
         }
+
+        protected int CreateGroup(string groupName)
+        {
+            Execute("groupadd", groupName);
+            return GetGroupId(groupName);
+        }
+
+        protected int GetGroupId(string groupName)
+        {
+            string standardOutput = Execute("getent", $"group {groupName}");
+            string[] values = standardOutput.Split(':', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries);
+            return int.Parse(values[^1]);
+        }
+
+        protected void SetGroupAsOwnerOfFile(string groupName, string filePath) =>
+            Execute("chgrp", $"{groupName} {filePath}");
+
+
+        protected void DeleteGroup(string groupName) =>
+            Execute("groupdel", groupName);
+
+        private string Execute(string command, string arguments)
+        {
+            using Process p = new Process();
+
+            p.StartInfo.UseShellExecute = false;
+            p.StartInfo.FileName = command;
+            p.StartInfo.Arguments = arguments;
+            p.StartInfo.RedirectStandardOutput = true;
+            p.StartInfo.RedirectStandardError = true;
+
+            p.Start();
+            p.WaitForExit();
+
+            string standardOutput = p.StandardOutput.ReadToEnd();
+            string standardError = p.StandardError.ReadToEnd();
+
+            if (p.ExitCode != 0)
+            {
+                throw new IOException($"Error '{p.ExitCode}' when executing '{command} {arguments}'. Message: {standardError}");
+            }
+
+            return standardOutput;
+        }
     }
 }
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
@@ -9,7 +9,7 @@ namespace System.Formats.Tar.Tests
 {
     public partial class TarWriter_WriteEntry_File_Tests : TarWriter_File_Base
     {
-        [ConditionalTheory(nameof(IsRemoteExecutorSupportedAndOnUnixAndSuperUser))]
+        [ConditionalTheory(nameof(IsRemoteExecutorSupportedAndPrivilegedProcess))]
         [InlineData(TarEntryFormat.Ustar)]
         [InlineData(TarEntryFormat.Pax)]
         [InlineData(TarEntryFormat.Gnu)]
@@ -51,7 +51,7 @@ public void Add_Fifo(TarEntryFormat format)
             }, format.ToString(), new RemoteInvokeOptions { RunAsSudo = true }).Dispose();
         }
 
-        [ConditionalTheory(nameof(IsRemoteExecutorSupportedAndOnUnixAndSuperUser))]
+        [ConditionalTheory(nameof(IsRemoteExecutorSupportedAndPrivilegedProcess))]
         [InlineData(TarEntryFormat.Ustar)]
         [InlineData(TarEntryFormat.Pax)]
         [InlineData(TarEntryFormat.Gnu)]
@@ -96,7 +96,7 @@ public void Add_BlockDevice(TarEntryFormat format)
             }, format.ToString(), new RemoteInvokeOptions { RunAsSudo = true }).Dispose();
         }
 
-        [ConditionalTheory(nameof(IsRemoteExecutorSupportedAndOnUnixAndSuperUser))]
+        [ConditionalTheory(nameof(IsRemoteExecutorSupportedAndPrivilegedProcess))]
         [InlineData(TarEntryFormat.Ustar)]
         [InlineData(TarEntryFormat.Pax)]
         [InlineData(TarEntryFormat.Gnu)]
@@ -139,5 +139,49 @@ public void Add_CharacterDevice(TarEntryFormat format)
 
             }, format.ToString(), new RemoteInvokeOptions { RunAsSudo = true }).Dispose();
         }
+
+        [ConditionalTheory(nameof(IsRemoteExecutorSupportedAndPrivilegedProcess))]
+        [InlineData(TarEntryFormat.Ustar)]
+        [InlineData(TarEntryFormat.Pax)]
+        [InlineData(TarEntryFormat.Gnu)]
+        public void CreateEntryFromFileOwnedByNonExistentGroup(TarEntryFormat f)
+        {
+            RemoteExecutor.Invoke((string strFormat) =>
+            {
+                using TempDirectory root = new TempDirectory();
+
+                string fileName = "file.txt";
+                string filePath = Path.Join(root.Path, fileName);
+                File.Create(filePath).Dispose();
+
+                string groupName = Path.GetRandomFileName()[0..6];
+                int groupId = CreateGroup(groupName);
+
+                try
+                {
+                    SetGroupAsOwnerOfFile(groupName, filePath);
+                }
+                finally
+                {
+                    DeleteGroup(groupName);
+                }
+
+                using MemoryStream archive = new MemoryStream();
+                using (TarWriter writer = new TarWriter(archive, Enum.Parse<TarEntryFormat>(strFormat), leaveOpen: true))
+                {
+                    writer.WriteEntry(filePath, fileName); // Should not throw
+                }
+                archive.Seek(0, SeekOrigin.Begin);
+
+                using (TarReader reader = new TarReader(archive, leaveOpen: false))
+                {
+                    PosixTarEntry entry = reader.GetNextEntry() as PosixTarEntry;
+                    Assert.NotNull(entry);
+                    Assert.Equal(entry.GroupName, string.Empty);
+                    Assert.Equal(groupId, entry.Gid);
+                    Assert.Null(reader.GetNextEntry());
+                }
+            }, f.ToString(), new RemoteInvokeOptions { RunAsSudo = true }).Dispose();
+        }
     }
 }
diff --git a/...libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntryAsync.File.Tests.Unix.cs b/...libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntryAsync.File.Tests.Unix.cs
@@ -10,7 +10,7 @@ namespace System.Formats.Tar.Tests
 {
     public partial class TarWriter_WriteEntryAsync_File_Tests : TarWriter_File_Base
     {
-        [ConditionalTheory(nameof(IsRemoteExecutorSupportedAndOnUnixAndSuperUser))]
+        [ConditionalTheory(nameof(IsRemoteExecutorSupportedAndPrivilegedProcess))]
         [InlineData(TarEntryFormat.Ustar)]
         [InlineData(TarEntryFormat.Pax)]
         [InlineData(TarEntryFormat.Gnu)]
@@ -55,7 +55,7 @@ public void Add_Fifo_Async(TarEntryFormat format)
             }, format.ToString(), new RemoteInvokeOptions { RunAsSudo = true }).Dispose();
         }
 
-        [ConditionalTheory(nameof(IsRemoteExecutorSupportedAndOnUnixAndSuperUser))]
+        [ConditionalTheory(nameof(IsRemoteExecutorSupportedAndPrivilegedProcess))]
         [InlineData(TarEntryFormat.Ustar)]
         [InlineData(TarEntryFormat.Pax)]
         [InlineData(TarEntryFormat.Gnu)]
@@ -103,7 +103,7 @@ public void Add_BlockDevice_Async(TarEntryFormat format)
             }, format.ToString(), new RemoteInvokeOptions { RunAsSudo = true }).Dispose();
         }
 
-        [ConditionalTheory(nameof(IsRemoteExecutorSupportedAndOnUnixAndSuperUser))]
+        [ConditionalTheory(nameof(IsRemoteExecutorSupportedAndPrivilegedProcess))]
         [InlineData(TarEntryFormat.Ustar)]
         [InlineData(TarEntryFormat.Pax)]
         [InlineData(TarEntryFormat.Gnu)]
@@ -149,5 +149,49 @@ public void Add_CharacterDevice_Async(TarEntryFormat format)
                 }
             }, format.ToString(), new RemoteInvokeOptions { RunAsSudo = true }).Dispose();
         }
+
+        [ConditionalTheory(nameof(IsRemoteExecutorSupportedAndPrivilegedProcess))]
+        [InlineData(TarEntryFormat.Ustar)]
+        [InlineData(TarEntryFormat.Pax)]
+        [InlineData(TarEntryFormat.Gnu)]
+        public void CreateEntryFromFileOwnedByNonExistentGroup_Async(TarEntryFormat f)
+        {
+            RemoteExecutor.Invoke(async (string strFormat) =>
+            {
+                using TempDirectory root = new TempDirectory();
+
+                string fileName = "file.txt";
+                string filePath = Path.Join(root.Path, fileName);
+                File.Create(filePath).Dispose();
+
+                string groupName = Path.GetRandomFileName()[0..6];
+                int groupId = CreateGroup(groupName);
+
+                try
+                {
+                    SetGroupAsOwnerOfFile(groupName, filePath);
+                }
+                finally
+                {
+                    DeleteGroup(groupName);
+                }
+
+                await using MemoryStream archive = new MemoryStream();
+                await using (TarWriter writer = new TarWriter(archive, Enum.Parse<TarEntryFormat>(strFormat), leaveOpen: true))
+                {
+                    await writer.WriteEntryAsync(filePath, fileName); // Should not throw
+                }
+                archive.Seek(0, SeekOrigin.Begin);
+
+                await using (TarReader reader = new TarReader(archive, leaveOpen: false))
+                {
+                    PosixTarEntry entry = await reader.GetNextEntryAsync() as PosixTarEntry;
+                    Assert.NotNull(entry);
+                    Assert.Equal(entry.GroupName, string.Empty);
+                    Assert.Equal(groupId, entry.Gid);
+                    Assert.Null(await reader.GetNextEntryAsync());
+                }
+            }, f.ToString(), new RemoteInvokeOptions { RunAsSudo = true }).Dispose();
+        }
     }
 }