klauspost / pgzip

Go parallel gzip (de)compression
MIT License
1.12k stars 77 forks source link

pgzip.Writer causes panics in bufio.Write #32

Closed ajm188 closed 4 years ago

ajm188 commented 4 years ago

When pgzip's Writer is used as a bufio.Writer, calls to Write can result in panics like:

bufio: writer returned negative count from Write

which come from this bit of code in bufio:

var errNegativeWrite = errors.New("bufio: writer returned negative count from Write")

// writeBuf writes the Reader's buffer to the writer.
func (b *Reader) writeBuf(w io.Writer) (int64, error) {
    n, err := w.Write(b.buf[b.r:b.w])
    if n < 0 {
        panic(errNegativeWrite)
    }
    b.r += n
    return int64(n), err
}

This seems to be due to the section of Write that actually writes the compressed data to the underlying buffer, definitely at least during the first iteration, and possibly others. The issue is with this return:

if err := z.checkError(); err != nil {
    return len(p) - len(q) - length, err

}

On the first iteration of this loop q := p, and length is a positive integer, so this will always return a negative number, causing bufio to panic rather than to propagate the error back to the caller.

I think a simple fix here is to return the max of 0 and that value.

klauspost commented 4 years ago

Thanks for the detailed analysis.

AFAICT returning len(p) - len(q) would also be safe. It doesn't matter much what exactly it is, since it probably hasn't been compressed anyway, but of course it shouldn't be negative.

ajm188 commented 4 years ago

That sounds good to me as well.