-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Tar: Fix buffer is too small error when writing Prefix on large filenames #75023
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
Changes from all commits
e32acb8
7df093d
f9666b5
99a7381
7366e0b
1b4bf33
f666873
aab4ead
3b02c6e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |||||||||||||
| using System.Collections.Generic; | ||||||||||||||
| using System.IO; | ||||||||||||||
| using System.Linq; | ||||||||||||||
| using Microsoft.DotNet.XUnitExtensions; | ||||||||||||||
| using Xunit; | ||||||||||||||
|
|
||||||||||||||
| namespace System.Formats.Tar.Tests | ||||||||||||||
|
|
@@ -253,5 +254,55 @@ public void SkipRecursionIntoBaseDirectorySymlink() | |||||||||||||
|
|
||||||||||||||
| Assert.Null(reader.GetNextEntry()); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| [ConditionalTheory(nameof(CanExtractWithTarTool))] | ||||||||||||||
| [MemberData(nameof(GetPaxAndGnuTestCaseNames))] | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need another condition here confirming that the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess we will know when the CI is done.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another option is to assume
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, on Alpine, it's implemented by Busybox, with a link. So
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe, but won't this fail on any Windows version if 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, I removed it because nano was still failing (even with the Got this error log:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How are we using CanCreateSymbolicLinks in our System.IO symlink tests? |
||||||||||||||
| public void ResultingArchive_CanBeExtractedByOtherTools(string testCaseName) | ||||||||||||||
| { | ||||||||||||||
| if (testCaseName == "specialfiles" && !PlatformDetection.IsSuperUser) | ||||||||||||||
| { | ||||||||||||||
| throw new SkipTestException("specialfiles is only supported on Unix and it needs sudo permissions for extraction."); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| string[] symlinkTestCases = { "file_symlink", "foldersymlink_folder_subfolder_file", "file_longsymlink" }; | ||||||||||||||
| if (symlinkTestCases.Contains(testCaseName) && PlatformDetection.IsWindowsNanoServer) // Tar version of Windows Nano Server does not support extracting symlinks. | ||||||||||||||
| { | ||||||||||||||
| throw new SkipTestException("Symlinks are not supported on this platform."); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| using TempDirectory root = new TempDirectory(); | ||||||||||||||
|
|
||||||||||||||
| string archivePath = GetTarFilePath(CompressionMethod.Uncompressed, TestTarFormat.pax.ToString(), testCaseName); | ||||||||||||||
| string tarExtractedPath = Path.Join(root.Path, "tarExtracted"); | ||||||||||||||
| Directory.CreateDirectory(tarExtractedPath); | ||||||||||||||
|
|
||||||||||||||
| // extract with /usr/bin/tar | ||||||||||||||
| ExtractWithTarTool(archivePath, tarExtractedPath); | ||||||||||||||
|
|
||||||||||||||
| // create archive with TarFile | ||||||||||||||
| string dotnetTarArchive = Path.Join(root.Path, "dotnetTarArchive.tar"); | ||||||||||||||
| TarFile.CreateFromDirectory(tarExtractedPath, dotnetTarArchive, includeBaseDirectory: false); | ||||||||||||||
|
|
||||||||||||||
| // extract TarFile's archive with /usr/bin/tar | ||||||||||||||
| string dotnetTarExtractedPath = Path.Join(root.Path, "dotnetTarExtracted"); | ||||||||||||||
| Directory.CreateDirectory(dotnetTarExtractedPath); | ||||||||||||||
| ExtractWithTarTool(dotnetTarArchive, dotnetTarExtractedPath); | ||||||||||||||
|
|
||||||||||||||
| // compare results | ||||||||||||||
| Dictionary<string, FileSystemInfo> expectedEntries = GetFileSystemInfosRecursive(tarExtractedPath).ToDictionary(fsi => fsi.FullName.Substring(tarExtractedPath.Length)); | ||||||||||||||
|
|
||||||||||||||
| foreach (FileSystemInfo actualEntry in GetFileSystemInfosRecursive(dotnetTarExtractedPath)) | ||||||||||||||
| { | ||||||||||||||
| string keyPath = actualEntry.FullName.Substring(dotnetTarExtractedPath.Length); | ||||||||||||||
| Assert.True(expectedEntries.TryGetValue(keyPath, out FileSystemInfo expectedEntry), $"Entry '{keyPath}' not expected."); | ||||||||||||||
| Assert.Equal(expectedEntry.Attributes, actualEntry.Attributes); | ||||||||||||||
| Assert.Equal(expectedEntry.LinkTarget, actualEntry.LinkTarget); | ||||||||||||||
jozkee marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
|
|
||||||||||||||
| expectedEntries.Remove(keyPath); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // All expected entries should've been found and removed. | ||||||||||||||
| Assert.Equal(0, expectedEntries.Count); | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,9 @@ | |
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System.Collections.Generic; | ||
| using System.Diagnostics; | ||
| using System.IO; | ||
| using System.IO.Enumeration; | ||
| using System.Linq; | ||
| using System.Runtime.CompilerServices; | ||
| using Microsoft.DotNet.RemoteExecutor; | ||
|
|
@@ -611,5 +613,58 @@ public static IEnumerable<object[]> GetPaxAndGnuTestCaseNames() | |
| yield return new object[] { name }; | ||
| } | ||
| } | ||
|
|
||
| protected static FileSystemEnumerable<FileSystemInfo> GetFileSystemInfosRecursive(string path) | ||
| { | ||
| var enumerationOptions = new EnumerationOptions { RecurseSubdirectories = true }; | ||
| return new FileSystemEnumerable<FileSystemInfo>(path, (ref FileSystemEntry e) => e.ToFileSystemInfo(), enumerationOptions); | ||
| } | ||
|
|
||
| public static bool CanExtractWithTarTool => s_canExtractWithTarTool.Value; | ||
|
|
||
| private static readonly Lazy<bool> s_canExtractWithTarTool = new Lazy<bool>(() => | ||
| { | ||
| if (PlatformDetection.IsAndroid || PlatformDetection.IsLinuxBionic) // Android reports tar: chown 7913:3579 'file.txt': Operation not permitted. | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| if (PlatformDetection.IsWindows) | ||
| { | ||
| Version osVersion = Environment.OSVersion.Version; | ||
| bool isAtLeastWin10Build17063 = osVersion.Major >= 11 || osVersion.Major == 10 && osVersion.Build >= 17063; | ||
| if (!isAtLeastWin10Build17063) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| using Process tarToolProcess = new Process(); | ||
| tarToolProcess.StartInfo.FileName = "tar"; // location varies: find it on the PATH. | ||
| tarToolProcess.StartInfo.Arguments = "--help"; | ||
|
|
||
| tarToolProcess.StartInfo.RedirectStandardOutput = true; | ||
| tarToolProcess.Start(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
I assume they're talking about Windows 11. Interestingly, my Windows 10 machine also has tar available, so I think the document is incomplete.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On Windows 10, executing And on Windows 11:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Decoder .. this is Windows 10 RS4, in 2018.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah good to know. The article's date is
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also note we support Windows 10 from RS1 (1607)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will wrap the code in a try-catch, just in case.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
|
||
| string output = tarToolProcess.StandardOutput.ReadToEnd(); | ||
| tarToolProcess.WaitForExit(); | ||
|
|
||
| return tarToolProcess.ExitCode == 0; | ||
| }); | ||
|
|
||
| public static void ExtractWithTarTool(string source, string destination) | ||
| { | ||
| using Process tarToolProcess = new Process(); | ||
| tarToolProcess.StartInfo.FileName = "tar"; // location varies: find it on the PATH. | ||
| tarToolProcess.StartInfo.Arguments = $"-xf {source} -C {destination}"; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could add an additional boolean parameter We would just have to adjust line 271 of the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
|
||
| tarToolProcess.StartInfo.RedirectStandardOutput = true; | ||
jozkee marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| tarToolProcess.Start(); | ||
|
|
||
| string output = tarToolProcess.StandardOutput.ReadToEnd(); | ||
| tarToolProcess.WaitForExit(); | ||
|
|
||
| Assert.True(tarToolProcess.ExitCode == 0, "Tar process output: " + output); | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.