net/textproto: swallows errors returned by underlying reader

[kayrus]

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

$ go version
go version go1.18.3 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What did you do?

See example: https://go.dev/play/p/Bn8H6p4QUf6

What did you expect to see?

I expect that bufio package react on underlying reader errors and stop the reading.

What did you see instead?

I see that bufio skips an error multiple times, and the example code causes OOM error becuase reader is rewinded every time:

here are cases when an error is not tracked:
https://cs.opensource.google/go/go/+/refs/tags/go1.18.4:src/net/textproto/reader.go;l=148;drc=27794c4d4a18c61d8c158d253421d72b5a6a8673
https://cs.opensource.google/go/go/+/refs/tags/go1.18.4:src/net/textproto/reader.go;l=496;drc=27794c4d4a18c61d8c158d253421d72b5a6a8673
https://cs.opensource.google/go/go/+/refs/tags/go1.18.4:src/net/textproto/reader.go;l=566;drc=27794c4d4a18c61d8c158d253421d72b5a6a8673
https://cs.opensource.google/go/go/+/refs/tags/go1.18.4:src/net/textproto/reader.go;l=571;drc=27794c4d4a18c61d8c158d253421d72b5a6a8673

the error, returned by autoRewind is just ignored and set to nil:
https://cs.opensource.google/go/go/+/refs/tags/go1.18.4:src/bufio/bufio.go;l=156;drc=27794c4d4a18c61d8c158d253421d72b5a6a8673
https://cs.opensource.google/go/go/+/refs/tags/go1.18.4:src/bufio/bufio.go;l=124;drc=27794c4d4a18c61d8c158d253421d72b5a6a8673

I know that it's something that I have to expect with autoRewind, but this behavior caused an unpleasant surprise.

see also:

• net/textproto: fails to parse headers without a body #53854
• net/textproto: fails when headers contain invalid headers #53855

[seankhliao]

Your code never returns an error. There is nothing for bufio to report.
If you fix your code to actually return errors, it still violates the io.Reader contract:

a Reader returning a non-zero number of bytes at the end of the input stream may return either err == EOF or err == nil. The next Read should return 0, EOF

Closing as there is nothing to do.

For questions please refer to https://github.com/golang/go/wiki/Questions

[kayrus]

Your code never returns an error. There is nothing for bufio to report.

@seankhliao it does, see line 29

[seankhliao]

Your Read creates a fresh strings.Reader every time because its defined with a value receiver, and strings.Reader doesn't return an EOF until the second read.

[kayrus]

@seankhliao I updated the example code: https://go.dev/play/p/XD0e4msIGqt
you can see that simple io.Copy works, simple io.Copy wrapped around bufio also works, but net/textproto read fails.
Yes, I was wrong with the issue topic, so I renamed it to net/textproto.

[ianlancetaylor]

Thanks, I do think there is a bug here. net/textproto.Reader.skipSpace assumes that it doesn't have to return the error, with a comment saying "Bufio will keep err until next read.", but that's not how the bufio package works. Once the bufio package has returned an error, it won't return it again.

[cz2h]

@ianlancetaylor One quick fix I can think of is to add one more field in bufio's Reader struct to memorize if EOF is reached and let ReadLine() returns while EOF is reached before calling ReadSlice() for the following reasons.

In the case of using a non-autorewind reader, after reading the last line of message with bufio.ReadLine(), there will be one more call on bufio.ReadLine() and the EOF error pops up.

In the case of using autorewind reader, the bufio.ReaderLine() seems to always being able to read something without returning the error and the methods in textproto calling ReadLine() will always read something without receiving the EOF error, making the ReadMIMEHeader() non stop.

Please point out if there anything unclear or unthoughtful above, thanks

[ianlancetaylor]

The bufio package is pretty low level and it would be much better if we didn't have to change it.

As far as I can see the bug here is in net/textproto, and that is where the fix should occur.

[thedadams]

Could I make an attempt at fixing this issue?

[gopherbot]

Change https://go.dev/cl/418694 mentions this issue: net/textproto: add extra field to indicate whether EOF is reached

[gopherbot]

Change https://go.dev/cl/421554 mentions this issue: net/textproto: refactor return type of skipSpace function

[gopherbot]

Change https://go.dev/cl/544935 mentions this issue: net/textproto: improve error handling in skipSpace

[ianlancetaylor]

Looking again at the original example, the Reader there looks like this:

func (r *autoRewind) Read(p []byte) (int, error) {
	if r.r == nil {
		r.r = strings.NewReader(r.buf)
	}

	n, err := r.r.Read(p)
	fmt.Printf("%d: %v\n", n, err)
	if err == io.EOF {
		// rewind
		r.r = strings.NewReader(r.buf)
	}

	return n, err
}

This is going to return io.EOF and then continue to return data. That violates the io.Reader contract, which 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. An instance of this general case is that a Reader returning a non-zero number of bytes at the end of the input stream may return either err == EOF or err == nil. The next Read should return 0, EOF.

That is, after returning EOF, the Reader should then return 0, EOF. The code in this example does not do that.

So now I'm not sure whether there is a bug here at all.

[joeshin]

Thanks for looking this one more time @ianlancetaylor. Re-reading the thread again as well as your last comment, I see what @seankhliao was saying. I agree, the example implementation doesn't do that.