blob/s3blob: nil pointer dereference for range reader error path

[omriarad]

Describe the bug

Panic caused by nil pointer dereference in blob Reader after error in Read. Debugging this it seems this is caused by Reader entering an invalid state:

1. Read is called and we reach line 147 (in version 0.37.0) and NewRangeReader returns an error:

           r.r, err = r.b.NewRangeReader(r.ctx, r.key, r.baseOffset+r.relativeOffset, length, r.dopts)
           if err != nil {
               return 0, wrapError(r.b, err, r.key)
           }

2. r.r is now nil.
3. Seek (which doesn't validate the object) is later called for the reader, we reach Size and r.r.Attributes().Size panics.

To Reproduce

Managed to reproduce this 100% of the time in a nonsensical way with context canceling:

package main

import (
    "context"
    "io"

    awsv2cfg "github.com/aws/aws-sdk-go-v2/config"
    "github.com/aws/aws-sdk-go-v2/credentials"
    s3v2 "github.com/aws/aws-sdk-go-v2/service/s3"
    "gocloud.dev/blob"
    "gocloud.dev/blob/s3blob"
)

const (
    AWS_ACCESS_KEY_ID     = "XXXXX"
    AWS_SECRET_ACCESS_KEY = "XXXXX"
    AWS_SESSION_TOKEN     = "XXXXX"
)

func main() {
    ctx, cancelFunc := context.WithCancel(context.Background())

    client := openClient(ctx)
    bucket, err := s3blob.OpenBucketV2(ctx, client, "test", nil)
    if err != nil {
        panic(err)
    }

    reader, err := bucket.NewRangeReader(ctx, "test.yaml", 0, 1024, &blob.ReaderOptions{})
    if err != nil {
        panic(err)
    }

    b := make([]byte, 512)
    _, err = reader.Read(b)
    if err != nil {
        panic(err)
    }

    _, err = reader.Seek(0, io.SeekStart)
    if err != nil {
        panic(err)
    }

    cancelFunc()

    _, _ = reader.Read(b)

    _, err = reader.Seek(0, io.SeekEnd)
    if err != nil {
        panic(err)
    }
}

func openClient(ctx context.Context) *s3v2.Client {
    funcs := setAWSLoadOptions()
    cfg, err := awsv2cfg.LoadDefaultConfig(ctx, funcs...)
    if err != nil {
        panic(err)
    }

    return s3v2.NewFromConfig(cfg)
}

func setAWSLoadOptions() []func(*awsv2cfg.LoadOptions) error {
    funcs := []func(*awsv2cfg.LoadOptions) error{awsv2cfg.WithRegion("us-east-1")}
    funcs = append(funcs, awsv2cfg.WithCredentialsProvider(credentials.NewStaticCredentialsProvider(
        AWS_ACCESS_KEY_ID,
        AWS_SECRET_ACCESS_KEY,
        AWS_SESSION_TOKEN)))

    return funcs
}

Traceback:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x20 pc=0x100e5f840]

goroutine 1 [running]:
gocloud.dev/blob.(*Reader).Size(0x140004c0000)
        /Users/user/go/pkg/mod/gocloud.dev@v0.37.0/blob/blob.go:222 +0x140
gocloud.dev/blob.(*Reader).Seek(0x140004c0000, 0x0, 0x2)
        /Users/user/go/pkg/mod/gocloud.dev@v0.37.0/blob/blob.go:169 +0x40
main.main()
        /Users/user/dev/example.go:49 +0x128

Process finished with the exit code 2

Expected behavior

Expected an error and not to panic.

Version

gocloud 0.37.0

Additional context

The original program consists of a lot of goroutines working on the reader with locks, the use itself is not ideal but at the very least it probably shouldn't crash. I would expect that if one gorountines errors (say from timeout getting context deadline exceeded) another goroutine using it would also get an error and not panic.

[vangent]

Thanks for the clear bug report! I've made it so that if the NewRangeReader inside Read (sometimes called after a Seek) fails, it leaves the Reader in the same state it was before the error is returned.

[omriarad]

Great! Thanks for the quick response, much appreciated!