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

[release/7.0] Fix TarWriter.TarEntry exception when adding file whose Unix group owner does not exist in the system #81139

Merged
merged 8 commits into from
Feb 8, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Jan 25, 2023

Backport of #81070 to release/7.0

/cc @carlossanlop

Customer Impact

Customers are reporting the issue dotnet/sdk-container-builds#246 in which the dotnet publish command is failing on MacOS on ARM when attempting to create an image test-container.

The failure is coming from our new System.Formats.Tar APIs. The container creation code calls TarWriter.WriteEntry(string filePath, string entryName), which is an API that reads the file from disk and inserts it to the TAR archive, making sure all its filesystem metadata is also included.

In Unix platforms, we try to retrieve the name of the group that owns the file, by using the group ID that is stored in the file metadata. When the group does not exist in the current platform (which can happen if for example, the file came from another machine), then our code unexpectedly throws, stopping the archive creation process.

The correct behavior, and what this fix is introducing, is to imitate what other archiving tools like the Unix tar tool do: if the group name is not found, then leave that field blank. This allows the entry to be successfully added to the archive.

Testing

Added sync and async unit tests to verify the correct behavior in all TAR formats that contain the GName field in the entry header metadata. The tests add a file, create a temporary group, change the group owner of the file to the newly created one, then delete the group, then attempt to add the file to a new archive using the same API that the customers are seeing in their crashes. The tests pass locally (in elevation mode) and in CI.

Risk

Low. This is a bug that does not match the behavior of other tools like the Unix tar tool. So ignoring the failure to retrieve the group name and writing blank is a better behavior than crashing.

cc @baronfel @rainersigwald

IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

@jeffhandley jeffhandley added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jan 26, 2023
@jeffhandley
Copy link
Member

This was approved through tactics via email. There's additional (post-merge) PR feedback that has come in on #81070 though, which should be addressed (and ideally re-validated) in main and added into this backport PR before merging into release/7.0.

@carlossanlop
Copy link
Member

which should be addressed (and ideally re-validated) in main

This wasn't a problem in main, only in this branch. The property name change had not been backported before, hence the build failure, so I had to manually add that change here to make it match main.

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @carlossanlop !

@carlossanlop
Copy link
Member

CI failures are known, pre-existing and unrelated: #81544 #81545 #74838
Signed-off by area owner.
Approved by Tactics.
No OOB package authoring changes.
Ready to merge when the branch opens again. :shipit:

@carlossanlop carlossanlop merged commit 13a9019 into release/7.0 Feb 8, 2023
@carlossanlop carlossanlop deleted the backport/pr-81070-to-release/7.0 branch February 8, 2023 23:57
@ghost ghost locked as resolved and limited conversation to collaborators Mar 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Formats.Tar Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants