-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Tar: Fix buffer is too small error when writing Prefix on large filenames #75023
Conversation
Tagging subscribers to this area: @dotnet/area-system-io Issue DetailsFound while addressing #74316 (comment).
TODO: Since this may be backport-worthy, consider merging without the aforementioned.
|
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/tests/TarFile/TarFile.CreateFromDirectory.File.Tests.cs
Outdated
Show resolved
Hide resolved
|
||
[ConditionalTheory] | ||
[PlatformSpecific(TestPlatforms.AnyUnix)] | ||
[MemberData(nameof(GetPaxAndGnuTestCaseNames))] |
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.
We need another condition here confirming that the /usr/bin/tar
binary exists. We don't know if some Unix flavors put the binary somewhere else.
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.
We assume /bin/ln always exists, I thought we could do the same here.
symLinkProcess.StartInfo.FileName = "/bin/ln"; |
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.
I guess we will know when the CI is done.
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.
Another option is to assume /usr/bin
is on the $PATH in every distro we test on. Just as you're assuming here that C:\Windows\System32
is on the Windows PATH
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.
Hmm, on Alpine, it's implemented by Busybox, with a link. So /usr/bin/tar
does not exist, but tar
should work:
danmoseL:~# ls -alF /bin/tar
lrwxrwxrwx 1 root root 12 Mar 31 2021 /bin/tar -> /bin/busybox*
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.
And incidentally, the busybox tar is probably significantly different to the GNU and BSD tar's so it would be good to try it. (You can also do it locally in WSL2 of course if you want)
src/libraries/System.Formats.Tar/tests/TarFile/TarFile.CreateFromDirectory.File.Tests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/tests/TarFile/TarFile.CreateFromDirectory.File.Tests.cs
Show resolved
Hide resolved
|
||
using Process tarToolProcess = new Process(); | ||
tarToolProcess.StartInfo.FileName = "/usr/bin/tar"; | ||
tarToolProcess.StartInfo.Arguments = $"-xf {source} -C {destination}"; |
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.
We could add an additional boolean parameter isCompressed
to this ExtractWithTarTool
method. If true
, then the arguments could include the z
letter, which is used to extract *.tar.gz
files.
We would just have to adjust line 271 of the ResultingArchive_CanBeExtractedByOtherTools
test method, so that it takes a variable CompressionMethod
.
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.
Yes, that should be part of the TODOs about adding more variants of this test.
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 so far. We can add the rest of the tests in a separate PR. Let's wait for the CI to finish.
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
src/libraries/System.Formats.Tar/tests/TarFile/TarFile.CreateFromDirectory.File.Tests.cs
Outdated
Show resolved
Hide resolved
One CI issue is just that some unix images doesn't have /usr/bin/tar. Second CI issue seems to be an encoding error on macOS, see below diff comparing what filenames bsd tar produces vs what we produce: - path: /földër bytes: 2F66C3B66C64C3AB72
+ path: /földër bytes: 2F666FCC886C6465CC8872
- path: /földër/áöñ.txt bytes: 2F66C3B66C64C3AB722FC3A1C3B6C3B12E747874
+ path: /földër/áöñ.txt bytes: 2F666FCC886C6465CC88722F61CC816FCC886ECC832E747874 |
@jozkee you mention this is addressing the question "do we write archives other tools/libraries can successfully open" .. can you say what testing you're doing / what your strategy is? eg.., how are you constructing the range of archives, and what tools are you testing accept them. How did you find this bug? |
@danmoseley I just wrote a test (in this PR, named
We could do the same for other tools, such a 7zip, but I thought that testing against the |
That seems like a pretty good strategy. |
We didn't have tests that tried to write files like the ones in https://github.com/dotnet/runtime-assets/tree/main/src/System.Formats.Tar.TestData/tar/gnu. So, some write edge-cases went untested, in the case of the bug I found, it was attempting to write a file with a large name (~256 characters). |
BTW, you might try both GNU tar (typical in Linux) and BSD tar (the one in Windows) if it's easy to do both. The latter is based on libarchive, and I think (?) GNU tar is not, so there's different coverage. |
tarToolProcess.StartInfo.Arguments = "--help"; | ||
|
||
tarToolProcess.StartInfo.RedirectStandardOutput = true; | ||
tarToolProcess.Start(); |
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 will fail if the test is run on a version of Windows prior to when they added 'tar'. do we have a "find on path" method or existing code we can use? or, we could catch whatever the exception is when the file does not exist.
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.
According to this: https://docs.microsoft.com/en-us/virtualization/community/team-blog/2017/20171219-tar-and-curl-come-to-windows
Beginning in Insider Build 17063, we’re introducing two command-line tools to the Windows toolchain: curl and bsdtar
I assume they're talking about Windows 11. Interestingly, my Windows 10 machine also has tar available, so I think the document is incomplete.
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.
On Windows 10, executing tar -h
on PowerShell gets me this:
bsdtar 3.5.2 - libarchive 3.5.2 zlib/1.2.5.f-ipp
And on Windows 11:
bsdtar 3.5.2 - libarchive 3.5.2 zlib/1.2.5.f-ipp bz2lib/1.0.6
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.
Decoder .. this is Windows 10 RS4, in 2018.
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.
Ah good to know. The article's date is 04/25/2022
, that's why I was assuming Win11.
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.
Also note we support Windows 10 from RS1 (1607)
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.
@danmoseley if it fails, it will just skip the test, something like this:
Discovering: System.Formats.Tar.Tests (method display = ClassAndMethod, method display options = None)
Discovered: System.Formats.Tar.Tests (found 1 of 578 test case)
Starting: System.Formats.Tar.Tests (parallel test collections = on, max threads = 12)
System.Formats.Tar.Tests.TarFile_CreateFromDirectory_File_Tests.ResultingArchive_CanBeExtractedByOtherTools [SKIP]
Condition(s) not met: "CanExtractWithTarTool (TargetInvocationException)"
Finished: System.Formats.Tar.Tests
=== TEST EXECUTION SUMMARY ===
System.Formats.Tar.Tests Total: 1, Errors: 0, Failed: 0, Skipped: 1, Time: 0.172s
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.
I will wrap the code in a try-catch, just in case.
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.
Now I wonder, if that's how xUnit handles exceptions on expressions used for ConditionalFact/TheoryAttribute
s, does it mean that we can end up with tests no longer running and we will never notice?
cc @dotnet/area-infrastructure-libraries
src/libraries/System.Formats.Tar/tests/TarFile/TarFile.CreateFromDirectory.File.Tests.cs
Outdated
Show resolved
Hide resolved
@@ -253,5 +254,50 @@ public void SkipRecursionIntoBaseDirectorySymlink() | |||
|
|||
Assert.Null(reader.GetNextEntry()); | |||
} | |||
|
|||
[ConditionalTheory(nameof(CanExtractWithTarTool))] | |||
[MemberData(nameof(GetPaxAndGnuTestCaseNames))] |
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.
As discussed via chat - Some link tests are failing, possibly because the OS does not support the creation of symlinks, so they should get separated into their own tests and decorated with the CanCreateSymbolicLinks
condition.
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.
Addressed slightly different on latest commit:
runtime/src/libraries/System.Formats.Tar/tests/TarFile/TarFile.CreateFromDirectory.File.Tests.cs
Lines 267 to 271 in 1b4bf33
string[] symlinkTestCases = { "file_symlink", "foldersymlink_folder_subfolder_file", "file_longsymlink" }; | |
if (symlinkTestCases.Contains(testCaseName) && !MountHelper.CanCreateSymbolicLinks) | |
{ | |
throw new SkipTestException("Symlinks are not supported on this platform."); | |
} |
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.
Nvm, this wasn't the reason of the failure, my guess is that Windows Nano Server has a bsdtar version that does not support extraction of symlinks.
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.
Maybe, but won't this fail on any Windows version if MountHelper.CanCreateSymbolicLinks
is false? I don't see it being checked in the current commit.
As you know symlinks on Windows require elevation unless you have developer mode. You almost certainly have developer mode enabled on your own machine, but it may not be enabled on others' machines. This won't show up in Helix because I believe they run all tests elevated, but any test we have that creates symlinks mst test MountHelper.CanCreateSymbolicLinks
(even testing you're elevated may not be enough, eg ?Nano)
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.
I don't see it being checked in the current commit.
Yeah, I removed it because nano was still failing (even with the CanCreateSymbolicLinks
check).
You can see the run for that commit (1b4bf33):
https://github.com/dotnet/runtime/pull/75023/checks?check_run_id=8242739135
Got this error log:
https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-75023-merge-fbe5d4bb79764adabe/System.Formats.Tar.Tests/1/console.da17f2b2.log?helixlogtype=result
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.
I believe you, just saying that if you fix whatever that problem is, you'd still need that check
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.
How are we using CanCreateSymbolicLinks in our System.IO symlink tests?
src/libraries/System.Formats.Tar/tests/TarFile/TarFile.CreateFromDirectory.File.Tests.cs
Outdated
Show resolved
Hide resolved
Make CanExtractWithTarTool return false for old Windows versions.
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
Just curious @jozkee what configuration are you needing to rerun here? Extra platforms uses 100's of machine hours each time so we've talked about making it more fine grained so it's less often used. |
@danmoseley right now I need android and windows nano server, the last one is part of the normal CI. |
The new test is now failing on Linux_bionic
How is this platform not considered android i.e: Not caught by |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
And again, now is failing on Win10 Arm64 and I don't know how to pull that image.
|
I think this fix is valuable but the test is clearly unreliable for merging (even undesirable for porting to 7.0). I think the best course of action right now is to submit the fix with a minimal test and defer tar tool validation for another PR for 8.0. |
But the ARM64 failure is that links cannot be created in the filesystem:
We could just skip the test in that platform. |
@carlossanlop I feel there will be another config that will then start failing either in the future of main branch or in release/7.0. |
Found while addressing #74316 (comment).
TODO:
Add similar test with TarWriter, async, and Stream.
Since this may be backport-worthy, consider merging without the aforementioned.