google / skicka

Command-line utility for working with Google Drive. Join the mailing list at https://groups.google.com/forum/#!forum/skicka-users.
Apache License 2.0
1.3k stars 118 forks source link

fixed GetFileContents() to surely return a non-nil error on Fail. #138

Closed KojiNakamaru closed 4 years ago

KojiNakamaru commented 4 years ago

func (gd *GDrive) GetFileContents(f *File) (io.ReadCloser, error) may return nil, nil on Fail:,

https://github.com/google/skicka/blob/a5c371fe953182b69bfd44c14297c1686382cbda/gdrive/gdrive.go#L1011-L1020 https://github.com/google/skicka/blob/a5c371fe953182b69bfd44c14297c1686382cbda/gdrive/http.go#L54-L60

as gd.client.Do(request) doesn't cause an error for non-2xx status code.

cf. https://golang.org/pkg/net/http/#Client.Do

An error is returned if caused by client policy (such as CheckRedirect), or failure to speak HTTP (such as a network connectivity problem). A non-2xx status code doesn't cause an error.

func downloadDriveFile(writer io.Writer, driveFile *gdrive.File) however supposes contentReader is non-nil if err is nil

https://github.com/google/skicka/blob/a5c371fe953182b69bfd44c14297c1686382cbda/download.go#L549-L564

and may cause a panic as below:

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

goroutine 92 [running]:
main.(*byteCountingReader).Read(0xc0004dafe0, 0xc00069e000, 0x8000, 0x8000, 0x0, 0x1592000, 0xc000b0a119)
    /Users/foobar/go/1.13.12/src/github.com/google/skicka/skicka.go:168 +0x29
io.copyBuffer(0x18118e0, 0xc00023e040, 0x1811920, 0xc0004dafe0, 0xc00069e000, 0x8000, 0x8000, 0x104cf26, 0x8, 0x18)
    /Users/foobar/.goenv/versions/1.13.12/src/io/io.go:402 +0x122
io.Copy(...)
    /Users/foobar/.goenv/versions/1.13.12/src/io/io.go:364
main.downloadDriveFile(0x18118e0, 0xc00023e040, 0xc000b09400, 0x0, 0x0)
    /Users/foobar/go/1.13.12/src/github.com/google/skicka/download.go:601 +0x330
main.downloadFile(0xc000b09400, 0xc000970060, 0x29, 0xc0002f2160, 0xc00066d808, 0x0)
    /Users/foobar/go/1.13.12/src/github.com/google/skicka/download.go:384 +0x140
main.syncHierarchyDown.func1(0xc000310060, 0xc00009e420, 0xc0002f2160, 0xc0003aa22c, 0xc00095b500)
    /Users/foobar/go/1.13.12/src/github.com/google/skicka/download.go:273 +0xe0
created by main.syncHierarchyDown
    /Users/foobar/go/1.13.12/src/github.com/google/skicka/download.go:267 +0xcb1

This patch adjusts GetFileContents() to always return a non-nil error on Fail:.

agoode commented 4 years ago

Thank you!