blob: Fix panics if reader recreation fails after Seek (#3425)

blob/blob.go

@@ -128,12 +128,11 @@ func (r *Reader) Read(p []byte) (int, error) {
 		// to (SeekEnd, 0) and use the return value to determine the size
 		// of the data, then Seek back to (SeekStart, 0).
 		saved := r.savedOffset
-		r.savedOffset = -1
 		if r.relativeOffset == saved {
 			// Nope! We're at the same place we left off.
+			r.savedOffset = -1
 		} else {
 			// Yep! We've changed the offset. Recreate the reader.
-			_ = r.r.Close()
 			length := r.baseLength
 			if length >= 0 {
 				length -= r.relativeOffset
@@ -142,11 +141,13 @@ func (r *Reader) Read(p []byte) (int, error) {
 					return 0, gcerr.Newf(gcerr.Internal, nil, "blob: invalid Seek (base length %d, relative offset %d)", r.baseLength, r.relativeOffset)
 				}
 			}
-			var err error
-			r.r, err = r.b.NewRangeReader(r.ctx, r.key, r.baseOffset+r.relativeOffset, length, r.dopts)
+			newR, 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)
 			}
+			_ = r.r.Close()
+			r.savedOffset = -1
+			r.r = newR
 		}
 	}
 	n, err := r.r.Read(p)

blob/blob_test.go

@@ -265,6 +265,74 @@ func TestDownloader(t *testing.T) {
 	}
 }
 
+func TestSeekAfterReadFailure(t *testing.T) {
+	const filename = "f.txt"
+
+	ctx := context.Background()
+
+	bucket := NewBucket(&oneTimeReadBucket{first: true})
+	defer bucket.Close()
+
+	reader, err := bucket.NewRangeReader(ctx, filename, 0, 100, nil)
+	if err != nil {
+		t.Fatalf("failed NewRangeReader: %v", err)
+	}
+	defer reader.Close()
+
+	b := make([]byte, 10)
+
+	_, err = reader.Read(b)
+	if err != nil {
+		t.Fatalf("failed Read#1: %v", err)
+	}
+
+	_, err = reader.Seek(0, io.SeekStart)
+	if err != nil {
+		t.Fatalf("failed Seek#1: %v", err)
+	}
+
+	// This Read will force a recreation of the reader via NewRangeReader,
+	// which will fail.
+	_, err = reader.Read(b)
+	if err == nil {
+		t.Fatalf("unexpectedly succeeded Read#2: %v", err)
+	}
+
+	_, err = reader.Seek(0, io.SeekStart)
+	if err != nil {
+		t.Fatalf("failed Seek#2: %v", err)
+	}
+}
+
+// oneTimeReadBucket implements driver.Bucket for TestSeekAfterReadFailure.
+// It returns a fake reader that succeeds once, then fails.
+type oneTimeReadBucket struct {
+	driver.Bucket
+	first bool
+}
+
+type workingReader struct {
+	driver.Reader
+}
+
+func (r *workingReader) Read(p []byte) (int, error) {
+	return len(p), nil
+}
+
+func (r *workingReader) Attributes() *driver.ReaderAttributes { return &driver.ReaderAttributes{} }
+func (r *workingReader) Close() error                         { return nil }
+
+func (b *oneTimeReadBucket) NewRangeReader(ctx context.Context, key string, offset, length int64, opts *driver.ReaderOptions) (driver.Reader, error) {
+	if b.first {
+		b.first = false
+		return &workingReader{}, nil
+	}
+	return nil, errFake
+}
+
+func (b *oneTimeReadBucket) ErrorCode(err error) gcerrors.ErrorCode { return gcerrors.Unknown }
+func (b *oneTimeReadBucket) Close() error                           { return nil }
+
 // erroringBucket implements driver.Bucket. All interface methods that return
 // errors are implemented, and return errFake.
 // In addition, when passed the key "work", NewRangeReader and NewTypedWriter