archive/zip: File's reader returns io.EOF prematurely

[dgrr]

What version of Go are you using (go version)?

$ go version
go version go1.12.5 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

$ go env
GOARCH="amd64"
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"

What did you do?

The code: https://play.golang.org/p/JvKpGINHHBY

What did you expect to see?

Not a panic.
The panic doesn't happen if you read from an *os.File, *bufio.Reader or *bytes.Reader, it only happens with the Zip files because it prematurely returns the io.EOF error.

What did you see instead?

A panic.

Edit

https://go-review.googlesource.com/c/go/+/184122

[av86743]

Usually Gerrit changesets also contain test(s) for the fix.

[dgrr]

@av86743 ok. I will add it tomorrow morning. Thanks

[odeke-em]

Thank you for filing this bug @dgrr, and @av86743 for encouraging the test in the patchset!

@dgrr could you please perhaps explain the underlying problem and perhaps describe why the current behavior is wrong?

[av86743]

@odeke-em

archive/zip: zip.OpenReader panics when opened .zip file does not exist

as shown by provided example https://play.golang.org/p/JvKpGINHHBY

Panics similarly on /dev/null and /dev/zero as well.

[dgrr]

Hello @odeke-em

The underlying problem is that the Read function in a zip.File returns the io.EOF error in the last read call, when readed bytes > 0.

I think that the current behavior is wrong based on how other Readers works. Also, the io.Reader doc says:

When Read encounters an error or end-of-file condition after successfully reading n > 0 bytes, it returns the number of bytes read. It may return the (non-nil) error from the same call or return the error (and n == 0) from a subsequent call.

[av86743]

Perhaps @odeke-em was going to suggest to add verbal description of a problem and/or solution to Gerrit issue, to facilitate the process of review?

[ianlancetaylor]

@dgrr I'm sorry, I don't understand. The docs for io.Reader that you quote permit a Reader to return a non-zero number of bytes with io.EOF. Code that calls Read must always handle the bytes first, then consider the error.

The docs for io.Reader say

Callers should always process the n > 0 bytes returned before considering the error err. Doing so correctly handles I/O errors that happen after reading some bytes and also both of the allowed EOF behaviors.

It's not clear to me that there is any bug here.

[dgrr]

Yes, you are right. But it's kind of weird to see how this Reader works as other readers works. From my point of view, I see that maybe the best option should be to return the io.EOF error in the subsequent call. But it's just a suggestion here.