Don't crash when the packet length is zero

[greatroar]

A packet with an invalid length causes a panic in recvPacket. Patch includes the fuzz test that found this out in a few seconds.

[davecheney]

thank you for working on this. I don't see much value in fuzzing the private API. Can you demonstrate that this crash can be trigged via a public API?

[greatroar]

Not an API, but a client panics when it connects to a broken server like the following one:

func TestBrokenServer(t *testing.T) {
	cr, sw := io.Pipe() // client reads, server writes
	sr, cw := io.Pipe() // server reads, client writes

	// Broken server that doesn't speak proper SFTP.
	go func() {
		go io.Copy(ioutil.Discard, sr)
		sw.Write([]byte{0, 0, 0, 0})
		sw.Close()
	}()

	NewClientPipe(cr, cw)
}

[greatroar]

I can change the fuzz test to do something similar. It would be more expensive to run, obviously.

[davecheney]

Can you write that as a failing test case?

[davecheney]

Remember that new client pipe doesn't actually need an io.pipe, that should make the test simpler, and simpler to write a fuzzer

[greatroar]

New test and fuzzer now calls NewClientPipe.

[greatroar]

The test failure is the same as one master (fixed in #387).

[davecheney]

Thank you for working on this

[davecheney]

Thank you for working on this

I don't see the utility of testing recvPacket for short packets. I would like to see a test against the external API exposed by NewClientPipe. If recvPacket crashes because the input is smaller than expected then this is the fault of its caller. It should be fixed at that point.

[greatroar]

I would like to see a test against the external API exposed by NewClientPipe.

Done, and also added a test that crashes the server on current master.

[davecheney]

LGTM, thank you. I'll leave this for the other maintainers.

[puellanivis]

Probably fine from me as well, but I’d like to get #387 merged first. Then a rebase would be required here.

[greatroar]

Rebased on #387.