[release/7.0] Fix prefix writing on TarHeaderWrite

[github-actions]

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:

System.ArgumentException : The output byte buffer is too small to contain the encoded data, encoding codepage '20127' and fallback 'System.Text.EncoderReplacementFallback'. (Parameter 'bytes')

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.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Backport of #75350 to release/7.0

/cc @jozkee

Customer Impact

Testing

Risk

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.

Author:	github-actions[bot]
Assignees:	-
Labels:	area-System.IO
Milestone:	-

[danmoseley]

Is this filenames, or paths over 200?
If filenames, how common do we believe this will be for customers?

[danmoseley]

@stephentoub any comments?

[danmoseley]

Have you examined other metadata that the TAR API's read/write to check for similar arbitrary limits?

[jozkee]

Is this filenames, or paths over 200?
If filenames, how common do we believe this will be for customers?

It is the TarEntry.Name property which holds the path, so, not just filename components, but nested files or entries could step into this issue.

[danmoseley]

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? 😺

[jozkee]

is change affecting other scenarios?

no

is code already proven in another branch -- or reusing existing code tested elsewhere - or just self evidently correct?

It is self-evidently correct. Code that is manipulating a string was using incorrect consts to get a buffer size for the remaining of 255 (name(100) + prefix(155)). It is proven in main branch.

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? 😺

The thing that could be broken is either the consts/math is still wrong or the assumption that n chars can fit in n bytes using Encoding.ASCII.GetBytes is wrong, the former also applies for the code without the fix, so I guess it can be discarded.

n chars can fit in n bytes using Encoding.ASCII.GetBytes

@carlossanlop is there a test for that? i.e: what if the Name contains non-UTF8 characters?

[jozkee]

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? 😺

@danmoseley it would be nice if those questions were in the template 😝.

[danmoseley]

@danmoseley it would be nice if those questions were in the template 😝.

Haha fair.. maybe I will edit it ( in the bot)

[danmoseley]

And no further objections from me ..@stephentoub?

[stephentoub]

@stephentoub any comments?

I'd like @stephentoub 's take.

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.... ;-)

[danmoseley]

Haha I'm just over communicating

[carlossanlop]

@stephentoub do the latest commits look good for merging?

[stephentoub]

Ideally this would have a seed. That way the test is reproducible.

Suggested change

	var r = new Random();
	var r = new Random(42);

[carlossanlop]

Glad you used 42. It's the answer to life, the universe, and everything. 😃

[stephentoub]

Suggested change

	using (TarWriter writer = new(ms, true))
	using (TarWriter writer = new(ms, leaveOpen: true))

[carlossanlop]

The CI failure is unrelated (System.Net.Http.Functional.Tests timeout).

[carlossanlop]

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)