Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix TarWriter.TarEntry exception when adding file whose Unix group owner does not exist in the system #81070

Merged
merged 7 commits into from
Jan 25, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -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>
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
/// <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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
Original file line number Diff line number Diff line change
@@ -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;

Expand All @@ -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);
Expand Down Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is using the pattern that is prone to a famous deadlock
https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.process.standardoutput?view=net-7.0#remarks

it will only occur if there is a sufficiently large amount of output, so this could well be fine here, but just FYI that this is not a good pattern to use in general.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Better late than never: #83126


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;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
jozkee marked this conversation as resolved.
Show resolved Hide resolved
Assert.Equal(groupId, entry.Gid);
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
Assert.Null(await reader.GetNextEntryAsync());
}
}, f.ToString(), new RemoteInvokeOptions { RunAsSudo = true }).Dispose();
}
}
}