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
@@ -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 Down Expand Up @@ -51,5 +52,68 @@ protected void VerifyPlatformSpecificMetadata(string filePath, TarEntry entry)
}
}
}

protected int CreateGroup(string groupName)
{
int exitCode = Execute("groupadd", groupName, out string standardOutput, out string standardError);
if (exitCode != 0)
{
ThrowOnError(exitCode, "groupadd", groupName, standardError);
}
return GetGroupId(groupName);
}

protected int GetGroupId(string groupName)
{
int exitCode = Execute("getent", $"group {groupName}", out string standardOutput, out string standardError);
if (exitCode != 0)
{
ThrowOnError(exitCode, "getent", "group", standardError);
}

string[] values = standardOutput.Split(':', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries);

return int.Parse(values[^1]);
}

protected void SetGroupAsOwnerOfFile(string groupName, string filePath)
{
int exitCode = Execute("chgrp", $"{groupName} {filePath}", out string standardOutput, out string standardError);
if (exitCode != 0)
{
ThrowOnError(exitCode, "chgroup", $"{groupName} {filePath}", standardError);
}
}

protected void DeleteGroup(string groupName)
{
int exitCode = Execute("groupdel", groupName, out string standardOutput, out string standardError);
if (exitCode != 0)
{
ThrowOnError(exitCode, "groupdel", groupName, standardError);
}
}

private int Execute(string command, string arguments, out string standardOutput, out string standardError)
{
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


standardOutput = p.StandardOutput.ReadToEnd();
standardError = p.StandardError.ReadToEnd();
return p.ExitCode;
}

private void ThrowOnError(int code, string command, string arguments, string message)
{
throw new IOException($"Error '{code}' when executing '{command} {arguments}'. Message: {message}");
}
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -139,5 +139,40 @@ public void Add_CharacterDevice(TarEntryFormat format)

}, format.ToString(), new RemoteInvokeOptions { RunAsSudo = true }).Dispose();
}

[ConditionalFact(nameof(IsRemoteExecutorSupportedAndPrivilegedProcess))]
public void CreateEntryFromFileOwnedByNonExistentGroup()
{
RemoteExecutor.Invoke(() =>
{
string groupName = Path.GetRandomFileName()[0..6];
int groupId = CreateGroup(groupName);

using TempDirectory root = new TempDirectory();

string fileName = "file.txt";
string filePath = Path.Join(root.Path, fileName);
File.Create(filePath).Dispose();

SetGroupAsOwnerOfFile(groupName, filePath);

DeleteGroup(groupName);
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved

using MemoryStream archive = new MemoryStream();
using (TarWriter writer = new TarWriter(archive, TarEntryFormat.Ustar, leaveOpen: true))
{
writer.WriteEntry(filePath, fileName); // Should not throw
}
archive.Seek(0, SeekOrigin.Begin);

using (TarReader reader = new TarReader(archive, leaveOpen: false))
{
UstarTarEntry entry = reader.GetNextEntry() as UstarTarEntry;
Assert.NotNull(entry);
Assert.Equal(entry.GroupName, string.Empty);
Assert.Equal(groupId, entry.Gid);
}
}, new RemoteInvokeOptions { RunAsSudo = true }).Dispose();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -149,5 +149,40 @@ public void Add_CharacterDevice_Async(TarEntryFormat format)
}
}, format.ToString(), new RemoteInvokeOptions { RunAsSudo = true }).Dispose();
}

[ConditionalFact(nameof(IsRemoteExecutorSupportedAndPrivilegedProcess))]
public void CreateEntryFromFileOwnedByNonExistentGroup_Async()
{
RemoteExecutor.Invoke(async () =>
{
string groupName = Path.GetRandomFileName()[0..6];
int groupId = CreateGroup(groupName);

using TempDirectory root = new TempDirectory();

string fileName = "file.txt";
string filePath = Path.Join(root.Path, fileName);
File.Create(filePath).Dispose();

SetGroupAsOwnerOfFile(groupName, filePath);

DeleteGroup(groupName);

await using MemoryStream archive = new MemoryStream();
await using (TarWriter writer = new TarWriter(archive, TarEntryFormat.Ustar, leaveOpen: true))
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
{
await writer.WriteEntryAsync(filePath, fileName); // Should not throw
}
archive.Seek(0, SeekOrigin.Begin);

await using (TarReader reader = new TarReader(archive, leaveOpen: false))
{
UstarTarEntry entry = await reader.GetNextEntryAsync() as UstarTarEntry;
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
}
}, new RemoteInvokeOptions { RunAsSudo = true }).Dispose();
}
}
}