bufio: Writer.Flush panics if it holds a nil interface writer

[stevvooe]

Calling (*bufio.Writer).Flush panics if (*bufio.Writer).Reset is called with a nil io.Writer, then (*bufio.Writer).Write is called before the flush. This seems like an unlikely scenario but it has been seen on moby/moby#10228. Though the use case may be invalid, the call should not panic.

The following code reproduces the issue:

package main

import (
    "bufio"
    "bytes"
)

func main() {
    var buf bytes.Buffer
    wr := bufio.NewWriter(&buf)
    wr.Reset(nil)
    wr.Write([]byte("asdf"))
    wr.Flush()
}

Running the above results in the following panic:

panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x20 pc=0x3366a]

goroutine 1 [running]:
bufio.(*Writer).flush(0x2080f0000, 0x0, 0x0)
    /usr/local/go/src/bufio/bufio.go:530 +0xda
bufio.(*Writer).Flush(0x2080f0000, 0x0, 0x0)
    /usr/local/go/src/bufio/bufio.go:519 +0x3a
main.main()
    /tmp/bug.go:13 +0x10b

goroutine 2 [runnable]:
runtime.forcegchelper()
    /usr/local/go/src/runtime/proc.go:90
runtime.goexit()
    /usr/local/go/src/runtime/asm_amd64.s:2232 +0x1

goroutine 3 [runnable]:
runtime.bgsweep()
    /usr/local/go/src/runtime/mgc0.go:82
runtime.goexit()
    /usr/local/go/src/runtime/asm_amd64.s:2232 +0x1
exit status 2

This was reproduced with Go 1.4.1 on Mac OS X 10.9 but has been experienced in other versions.

[minux]

The example code is pretty much asking for panic:
it is equivalent to this:
var wr io.Writer
wr.Write([]byte("asdf"))

This is working as intended.

[stevvooe]

@minux What? Are you kidding me? wr is not nil. Calls to it should not panic. This can be fixed with a nil check in Flush.

How does a caller check for this to avoid panic?

[minux]

wr := bufio.NewWriter(&buf)
wr.Reset(nil)
wr.Write([]byte("asdf"))
wr.Flush()

The 2nd line set the underlying writer of wr to nil, and
the fourth line ask bufio.Writer to write to a nil writer,
is that any different from my example?

var wr io.Writer
wr.Write([]byte("asdf"))

[stevvooe]

@minux Please read into the history covered in moby/moby#10228 and the panic at https://gist.github.com/stevvooe/12e63e5f902b5ba7ec01. The reality is much more subtle but the behavior is broken.

I conceded above that the use case is not valid but the panic is not expected.

[minux]

I fail to see why the panic is not expected. Why Reset(nil)?
Why Write and Flush after reseting the underlying writer to
nil?

Adding the check to the Flush method is just hiding bugs in
the client code. As using an invalid io.Writer is a programming
bug, panic is the correct way.

[ztdwu]

@minux "As using an invalid io.Writer is a programming
bug, panic is the correct way"

Flushing on a closed Writer is a programming bug too, and yet Go returns an error instead if panicking in this case. Sounds inconsistent to me.

[minux]

Valid use cases of panic is an excellent question to ask on the
golang-nuts mailing list, but not here. I'm sure gophers there will
explain the idiomatic way of using panics much better than me.
Thanks.

We're not going to make every standard package function to
check for nil interfaces before calling method on them.

[griesemer]

@stevvooe bufio.Writer.Reset(w) is setting the underlying writer to w; if Reset is called with nil, that writer is nil. Arguably that is a programmer error, or shall I say "invalid use" of the library. In your case you see a panic in Flush, but any function that eventually writes to the underlying nil writer will panic (e.g.: http://play.golang.org/p/P_f8BbNvCa).

I'd agree with @minux that it's not the job of the library to check for nil each time in this case. And even if one would check, that would lead to a difficult to use library because in the case of bufio, the panics (or errors, rather, if we check) would come at somewhat "arbitrary times". For instance in the example I gave above, all is fine if the buffer is large enough and never flushed, and the programming error is thus hidden as @minux @Aluded to.

That said, I think an argument could be made that the bufio factory functions (NewReader, NewScanner, NewWriter, ... ) but also functions modifying the underlying data structure (Reset, etc.) should check their arguments for validity and report an error otherwise.

However, this is not how this library was written. In case of the factory functions, one could argue that they should return a nil Reader/Writer/what-have-you if the incoming arguments are invalid. But that again puts the burden on each client, even clients that don't do anything wrong, because they cannot know if something else might cause a nil return.

To make a long story short: this package is written with the assumption that the supplied readers and writers are valid: Calling bufio.NewWriter with a nil underlying writer simply makes no sense.

Thus I must agree with @minux that this is working as intended. If one wanted to be very explicit, the documentation could say that the supplied arguments must be non-nil. At best this is a documentation issue.

[stevvooe]

@griesemer Should the documentation be updated to indicate that once a nil argument is passed, the underlying bufio.Writer is no longer valid and could panic at anytime?

[minux]

Because in majority of cases passing nil interface is invalid, I don't
think we need
to make that explicit. The document only needs to point out when an
interface
argument can be legally nil and what will happen if it's nil.

Similarly, if a function takes a pointer, it's invalid to pass a nil
pointer unless its
document explicitly allows that and generally it will also document what
will happen
in that case.

[minux]

I'd like to add that we can't check for nil in (*bufio.Writer).Reset,
because
that will be an incompatible API change.

It's legal to use wr.Reset(nil) to mark (poison) a Writer as not be used
again so that subsequent uses will very likely generate a panic. This is
another reason that we can't make Flush check for nil writers, the panic
might be intended.