proposal: x/tools/txtar: add Extract function or method

[adonovan]

While developing tests, I've written this function at least four times this week:

package txtar

// Extract writes each file in the archive to the corresponding location beneath dir.
func Extract(ar *Archive, dir string) error {
	for _, file := range ar.Files {
		name := filepath.Join(dir, file.Name)
		if err := os.MkdirAll(filepath.Dir(name), 0777); err != nil {
			return err
		}
		if err := os.WriteFile(name, file.Data, 0666); err != nil {
			return err
		}
	}
	return nil
}

I propose that we add it to the txtar package, either as a function or a method of Archive.

[mvdan]

Written a while back, but it might be useful: https://pkg.go.dev/github.com/rogpeppe/go-internal/txtar#Write cc @rogpeppe @myitcv

Also, shouldn't we use #56219?

[adonovan]

Personally I only use txtar for tests, but rejecting up-level references seems prudent. I'm less convinced about O_EXCL: I expect an extract operation to write files, even over existing ones.

[apparentlymart]

Safely extracting an arbitrary tar archive is quite tricky to get right, but indeed if this is intended only for well-known input in test cases that's less of a concern. My only reservation would be how to make it clear to potential users that it isn't safe for arbitrary archives that might potentially be malicious.

Having a robust tar extraction function available, which can guarantee that no files get written outside of the given prefix even in the presence of oddities such as symlinks and then directory entries that traverse through those symlinks, then I have a codebase that would shrink a bunch 😀 but I wouldn't personally have thought it was a common enough situation to have proposed it.

[ianlancetaylor]

@apparentlymart Just to make sure that we are all on the same page, this proposal is for the txtar format (https://pkg.go.dev/golang.org/x/tools/txtar) not the tar archive format. You said tar, not txtar; my apologies if you did indeed mean txtar.

[seankhliao]

alternatively, txtar could implement fs.FS for #44158
and, os can get a WriteFS(dir string, fsys fs.FS) error that would be more flexible

[apparentlymart]

Ah, indeed I did misunderstand. Thanks for the correction and sorry for the noise.

[rsc]

I'm fine with this as txtar.Extract, but the implementation should use filepath.ToSlash and filepath.IsLocal, as @mvdan noted.

I would rather not introduce a method since right now the Archive type is trivial, pure data, and I'd like to keep it that way.

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[rsc]

If we do #44158 and then add os.CopyFS from #45757 (comment), then like @seankhliao said above, that seems like a nicer outcome.

We would have to figure out what to do about txtar files that say ../ at the start, but archive/zip has to do that too and we can do whatever it does (drop them?).