Passing gzip.NewReader to s3manager.UploadInput causes SIGSEGV

[Integralist]

Please fill out the sections below to help us address your issue.

Version of AWS SDK for Go?

v1.28.10

Version of Go (go version)?

go version go1.13 darwin/amd64

What issue did you see?

Trying to upload gzip compressed http.Response#Body to S3 would cause the content to be stored compressed, but we want to store the decompressed content instead.

To work around that we attempt to wrap the io.ReadCloser of the response body in gzip.NewReader which caused the following panic...

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x49882d]

goroutine 54 [running]:
io.(*teeReader).Read(0xc00050c400, 0xc000a66000, 0x500000, 0x500000, 0x898391, 0xc00047cba8, 0x921ec0)
        /usr/local/go/src/io/io.go:535 +0x2d
github.com/aws/aws-sdk-go/service/s3/s3manager.readFillBuf(0xbad740, 0xc00050c400, 0xc000a66000, 0x500000, 0x500000, 0xc00049e400, 0x40b70a, 0x95dee0)
        /go/pkg/mod/github.com/aws/aws-sdk-go@v1.29.0/service/s3/s3manager/upload.go:494 +0x77
github.com/aws/aws-sdk-go/service/s3/s3manager.(*uploader).nextReader(0xc000250480, 0x0, 0x0, 0x10, 0x18, 0xc000022e00, 0x7fec7fd886d0)
        /go/pkg/mod/github.com/aws/aws-sdk-go@v1.29.0/service/s3/s3manager/upload.go:480 +0xb9
github.com/aws/aws-sdk-go/service/s3/s3manager.(*uploader).upload(0xc000250480, 0x0, 0x0, 0x0)
        /go/pkg/mod/github.com/aws/aws-sdk-go@v1.29.0/service/s3/s3manager/upload.go:376 +0x1fc
github.com/aws/aws-sdk-go/service/s3/s3manager.Uploader.UploadWithContext(0x500000, 0x5, 0x0, 0x2710, 0xbc4d40, 0xc0002220e0, 0x0, 0x0, 0x0, 0x0, ...)
        /go/pkg/mod/github.com/aws/aws-sdk-go@v1.29.0/service/s3/s3manager/upload.go:294 +0x17e
github.com/aws/aws-sdk-go/service/s3/s3manager.Uploader.Upload(...)
        /go/pkg/mod/github.com/aws/aws-sdk-go@v1.29.0/service/s3/s3manager/upload.go:264
github.com/buzzfeed/mono/perimeter/internal/pkg/proxy.(*responseHandler).store.func1(0xc0000d65b0, 0xc0002200f0, 0xc000491010, 0xc0002220d0)
        /app/internal/pkg/proxy/base.go:763 +0x100
created by github.com/buzzfeed/mono/perimeter/internal/pkg/proxy.(*responseHandler).store
        /app/internal/pkg/proxy/base.go:762 +0x924

Steps to reproduce

The following code demonstrates how we're reading the http response body...

func (rh *responseHandler) readBody() (io.Reader, error) {
	var reader io.Reader

	switch rh.r.Header.Get("Content-Encoding") {
	case "gzip":
		reader, err := gzip.NewReader(rh.r.Body)
		if err != nil {
			return nil, err
		}

		defer reader.Close() // doesn't close underlying io.Reader (see docs)
	default:
		reader = rh.r.Body
	}

	return reader, nil
}

We then pass the result of readBody to s3manager.UploadInput#Body field.

[diehlaws]

Hi @Integralist, thanks for reaching out to us. This looks like a duplicate of #3124, the code snippet isn't exactly the same but it still contains the variable scoping issue where reader is redeclared within the switch statement so the value assigned to it in this statement does not persist to the return value. Please do let me know if there's something I've missed here that makes this different from #3124.

I was able to successfully pass a gzip.Reader as the Body of the input for a s3manager.Upload call in a simplified reproduction of this behavior:

f, err := os.Open("zipped.txt.gz")
if err != nil {
  fmt.Printf("Error reading file: %v\n", err)
}
defer f.Close()

reader, err := gzip.NewReader(f)
if err != nil {
  fmt.Printf("Error unzipping file: %v\n", err)
}
defer reader.Close()

input := &s3manager.UploadInput{
  Bucket: aws.String("alex-sdk"),
  Key:    aws.String("go/3130/unzipped.txt"),
  Body:   reader,
}
resp, err := svc.Upload(input)
if err != nil {
  fmt.Printf("Error uploading object: %v\n", err)
} else {
  fmt.Printf("Object successfully uploaded to %v\n", resp.Location)
}

$ go run gzipReader.go 
Object successfully uploaded to https://alex-sdk.s3.us-west-2.amazonaws.com/go/3130/unzipped.txt

[Integralist]

@diehlaws oh god I hope it's not the same issue as before 🤦‍♂️ I'll check this in the morning when I'm at my laptop. Thanks for the details (and patience!)

[jasdel]

In Go 1.12 vet included a -shadow flag to help detect this issue, but in Go 1.13 this was removed from vet and moved into a separate package, golang.org/x/tools/go/analysis/passes/shadow/cmd/shadow. (golang/go#34053)

You can either run vet with shadow as a add on, or run shadow directly on your pkg. This should catch errors like this.

Note, make sure to run this outside of your Go module's directory tree because it will add addition entries to your go.mod, that aren't really needed.

go install golang.org/x/tools/go/analysis/passes/shadow/cmd/shadow
go vet -vettool=$(which shadow)

The steller` library had a neat way of installing tools like this on demand without modifing the project's go.mod.

command -v shadow >/dev/null 2>&1 || (
  dir=$(mktemp -d)
  pushd $dir
  go mod init tool
  go get golang.org/x/tools/go/analysis/passes/shadow/cmd/shadow
  popd
)

shadow ./...

[Integralist]

Going to close this issue, as the problem with gzip.NewReader did appear to be related to a variable scope bug (my apologies for not catching that, especially considering my previous issue was exactly the same cause. your patience was appreciated ❤️)

It seems there are still issues with using s3.manager.Upload with a httputil.ReverseProxy but it's a hard one to nail down because the stdlib proxy code provided by Go swallows the errors that are causing our application to panic (and as some requests work and some don't it's inconsistent behaviour which again means there's some form of race condition involved with data access or creation ...maybe 🤷‍♂ ) 😒

Any way, thanks for everyone's time, and @jasdel for the info re: the -shadow flag. I will look to configure that so I don't get this issue in future.