Skip to content
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

archive/tar: Writer should not accept empty Xattrs values #20698

Closed
cyphar opened this issue Jun 16, 2017 · 3 comments
Closed

archive/tar: Writer should not accept empty Xattrs values #20698

cyphar opened this issue Jun 16, 2017 · 3 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@cyphar
Copy link

cyphar commented Jun 16, 2017

% go version
go version go1.8.3 linux/amd64
% go env
GOARCH="amd64"
GOBIN="/home/cyphar/go/bin"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/cyphar/.local"
GORACE=""
GOROOT="/usr/lib64/go/1.8"
GOTOOLDIR="/usr/lib64/go/1.8/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build495773064=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

You can verify this at https://play.golang.org/p/T9jR4DO492. Here's the contents of the paste:

package main

import (
	"archive/tar"
	"fmt"
	"io"
)

func main() {
	reader, writer := io.Pipe()

	tw := tar.NewWriter(writer)
	tr := tar.NewReader(reader)

	go func() {
		tw.WriteHeader(&tar.Header{
			Name: "somefile",
			Mode: 0777,
			Xattrs: map[string]string{
				"user.emptyvalue": "",
				"user.nonempty":   "some value",
			},
		})
		tw.Close()
	}()

	hdr, err := tr.Next()
	if err != nil {
		panic(err)
	}

	if value, ok := hdr.Xattrs["user.emptyvalue"]; !ok {
		fmt.Printf("expected user.emptyvalue to exist\n")
	} else if value != "" {
		fmt.Printf("expected user.emptyvalue to be '', got %q\n", value)
	}

	if value, ok := hdr.Xattrs["user.nonempty"]; !ok {
		fmt.Printf("expected user.nonempty to exist\n")
	} else if value != "some value" {
		fmt.Printf("expected user.nonempty to be 'some value', got %q\n", value)
	}
}

What did you expect to see?

No output.

What did you see instead?

expected user.emptyvalue to exist

This is happening on the reading, not writing side (which you can verify by dumping the generated tar archive to stdout). According this section of archive/tar it looks like this is intentional:

		default:
			// According to PAX specification, a value is stored only if it is
			// non-empty. Otherwise, the key is deleted.
			if len(value) > 0 {
				extHdrs[key] = value
			} else {
				delete(extHdrs, key)
			}
		}

Which begs the question, should we be generating xattrs that aren't valid according to PAX or should we accept such headers? I don't really mind either way, but it seems quite odd that we are liberal in what we generate but strict in what we accept.

@odeke-em
Copy link
Member

/cc @dsnet

@dsnet
Copy link
Member

dsnet commented Jun 16, 2017

The PAX "specification" says the following:

If the field is zero length, it shall delete any header
block field, previously entered extended header value, or
global extended header value of the same name.

Which means that zero-length values have a very specific meaning: deletion. So the correct solution is to prevent the tar.Writer from generating these bogus entries.

A lot of work happened on the tar.Reader in the Go1.8 cycle, and I was planning on vastly improving the writer in Go1.9, but really dropped the ball this cycle. Will attempt to get the fix in Go1.10.

@dsnet dsnet self-assigned this Jun 16, 2017
@dsnet dsnet added this to the Go1.10 milestone Jun 16, 2017
@dsnet dsnet added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 16, 2017
@dsnet dsnet changed the title archive/tar: empty-value Xattrs are dropped with Reader archive/tar: Writer should not accept empty Xattr values Jun 16, 2017
@dsnet dsnet changed the title archive/tar: Writer should not accept empty Xattr values archive/tar: Writer should not accept empty Xattrs values Jun 16, 2017
@gopherbot
Copy link

Change https://golang.org/cl/55571 mentions this issue: archive/tar: reject bad key-value pairs for PAX records

@golang golang locked and limited conversation to collaborators Aug 15, 2018
@rsc rsc unassigned dsnet Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants