Could you build a minimal reproducer with the function in question? I would agree it shoudn't crash looking at https://pkg.go.dev/io#ReadFull

@andig Still trying, I will update here ASAP.

In case it helps, in similar issues in the past it seems like the panic might have been due to concurrent Read calls to the bufio.Reader.

@nishanths Thanks, that's the first thing came into my mind, I will double-check that.

Finally, I figured it out, it was caused by an invalid implementation of io.Read:

https://github.com/aler9/gomavlib/blob/bb2bb0de607f0b2d782de88087cf82045a4ef1fc/pkg/udplistener/udplistener.go#L97

// Read implements the net.Conn interface.
func (c *conn) Read(byt []byte) (int, error) {
	var buf []byte
	var ok bool

	if !c.readDeadline.IsZero() {
		readTimer := time.NewTimer(time.Until(c.readDeadline))
		defer readTimer.Stop()

		select {
		case <-readTimer.C:
			return 0, errTimeout
		case buf, ok = <-c.read:
		}
	} else {
		buf, ok = <-c.read
	}

	if !ok {
		return 0, errTerminated
	}

	copy(byt, buf)
	c.listener.readDone <- struct{}{}
	return len(buf), nil
}

This function returns the length of buf instead of how many bytes has been copied into byt.

On the other hand, bufio use what have returned without any check to update the write position, thus causing an invalid w that out of range of inside buf, which in my case is something more than 512.

func (b *Reader) Read(p []byte) (n int, err error) {
	n = len(p)
	if n == 0 {
		if b.Buffered() > 0 {
			return 0, nil
		}
		return 0, b.readErr()
	}
	if b.r == b.w {
		if b.err != nil {
			return 0, b.readErr()
		}
		if len(p) >= len(b.buf) {
			// Large read, empty buffer.
			// Read directly into p to avoid copy.
			n, b.err = b.rd.Read(p)
			if n < 0 {
				panic(errNegativeRead)
			}
			if n > 0 {
				b.lastByte = int(p[n-1])
				b.lastRuneSize = -1
			}
			return n, b.readErr()
		}
		// One read.
		// Do not use b.fill, which will loop.
		b.r = 0
		b.w = 0
		n, b.err = b.rd.Read(b.buf)
		if n < 0 {
			panic(errNegativeRead)
		}
		if n == 0 {
			return 0, b.readErr()
		}
		b.w += n
	}

	// copy as much as we can
	n = copy(p, b.buf[b.r:b.w])
	b.r += n
	b.lastByte = int(b.buf[b.r-1])
	b.lastRuneSize = -1
	return n, nil
}

For my case, I will fix the invalid implementation of io.Read, but I think it will be helpful if we add some extra sanity check here, maybe a panic with an error, because your problem will panic anyway later on and it's much harder to trace.

We don't generally try to defend against misbehaving io.Readers. There are lots of places readers and writers are used, and do we really want to defend against bad behavior everywhere?
In addition, it's only a partial defense. It would only catch the error if the return value was wrong enough to overflow the buffer. A wrong answer that was still within the bounds of the buffer would generate bad behavior (junk data?) without triggering the check.
The error here isn't really all that much "later". It's immediately after the call to the underlying badly-behaved reader. It's not an unbounded time later.

All that said, I think we could do something here. Maybe just a comment on the line with [b.r:b.w] saying that if it out-of-bounds panics, it must be because of a misbehaving reader. That should be enough to get a hint about the cause from the line number of the backtrace.

Change https://golang.org/cl/367214 mentions this issue: bufio: mention that panic at slicing means underlying reader is broken