-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Conversation
src/libraries/Common/src/Interop/Unix/System.Native/Interop.GetGroupName.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/Interop/Unix/System.Native/Interop.GetGroupName.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you for providing the fix @carlossanlop. If you like my suggestions please apply them before merging.
src/libraries/Common/src/Interop/Unix/System.Native/Interop.GetGroupName.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarWriter.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.File.Base.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.File.Base.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntry.File.Tests.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/Interop/Unix/System.Native/Interop.GetGroupName.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntryAsync.File.Tests.Unix.cs
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntryAsync.File.Tests.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntryAsync.File.Tests.Unix.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for fixing this, @carlossanlop!
CI failures are all unrelated:
|
/backport to release/7.0 |
Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/4001696599 |
p.StartInfo.RedirectStandardError = true; | ||
|
||
p.Start(); | ||
p.WaitForExit(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Fixes #81047
Addresses dotnet/sdk-container-builds#246
Users in MacOSX ARM are seeing an exception show up when executing
dotnet publish
. The exception shows up when the test container is created and theTarWriterWriteEntry(fileName, entryName)
is called. The method is throwing when attempting to add a file whose Unix group doesn't exist in the current machine.The root cause is that when we call the native method that retrieves a groupName using a groupId, the P/Invoke that wraps it throws an exception if the returned string is null.
WriteEntry
should be able to continue writing the rest of the entry with an empty groupName if such group is not found in the current system using the groupId. In this case, the groupName field should simply be an empty string.Added a couple of tests that create a randomly named group, create a file in disk, assigns the group as owner of the file, then deletes the group, then adds the file as an entry to the archive.
I deleted a method that was not used anywhere but in a test file. For future reference, if we need to bring it back, it should also add a new resource string with a more useful error message for when the errno is 0 and the string is null:
This affects users from .NET 7, so we will need to backport it.
cc @baronfel @rainersigwald