-
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
[release/7.0] Fix prefix writing on TarHeaderWrite #75373
Conversation
Tagging subscribers to this area: @dotnet/area-system-io Issue DetailsBackport of #75350 to release/7.0 /cc @jozkee Customer ImpactTestingRiskIMPORTANT: 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.
|
Is this filenames, or paths over 200? |
@stephentoub any comments? |
Have you examined other metadata that the TAR API's read/write to check for similar arbitrary limits? |
It is the |
OK, we clearly want to be able to support TAR'ing 200 chars below a root. The severity meets the bar. The change seems straightforward, but I don't know the format. I'd like @stephentoub 's take. BTW -- for the 'risk' in these templates, it's usually good to add some reasoning -- it helps think through. Eg., is change affecting other scenarios? is code already proven in another branch -- or reusing existing code tested elsewhere - or just self evidently correct? and my favorite question, reused from a past team - if I told you that I looked into the future and two weeks from now we discovered this change broke something -- what would that thing most likely have been, and why? 😺 |
no
It is self-evidently correct. Code that is manipulating a string was using incorrect
The thing that could be broken is either the consts/math is still wrong or the assumption that n
@carlossanlop is there a test for that? i.e: what if the Name contains non-UTF8 characters? |
@danmoseley it would be nice if those questions were in the template 😝. |
Haha fair.. maybe I will edit it ( in the bot) |
And no further objections from me ..@stephentoub? |
It's 7:30pm on a Friday night. I'll review but it won't be right this second.... ;-) |
Haha I'm just over communicating |
src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntry.Tests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs
Show resolved
Hide resolved
@stephentoub do the latest commits look good for merging? |
[InlineData(TarEntryFormat.Gnu)] | ||
public void WriteLongName(TarEntryFormat format) | ||
{ | ||
var r = new Random(); |
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.
Ideally this would have a seed. That way the test is reproducible.
var r = new Random(); | |
var r = new Random(42); |
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.
Glad you used 42. It's the answer to life, the universe, and everything. 😃
{ | ||
TarEntry entry; | ||
MemoryStream ms = new(); | ||
using (TarWriter writer = new(ms, true)) |
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.
using (TarWriter writer = new(ms, true)) | |
using (TarWriter writer = new(ms, leaveOpen: true)) |
The CI failure is unrelated (System.Net.Http.Functional.Tests timeout). |
Since the two remaining feedback comments are in test code, I asked @jozkee to include them in the next backport PR so we don't restart the CI on this one. @danmoseley can we get an approval? Edit: I confirmed with Dan that this comment can be interpreted as approval #75373 (comment) |
Backport of #75350 to release/7.0
/cc @jozkee
Customer Impact
Customers using the new Tar APIs to archive filenames larger than 200 characters in Pax format may find an unexpected and unclear exception:
It is an important scenario as Pax is the default format.
Testing
Tested with multiple formats, although the only one presenting the error was Pax.
Risk
Low.