NewDelimitedReader does not differentiate between EOF and 0-length message about protobuf HOT 6 CLOSED

From [email protected] on November 14, 2014 16:47:41

Actually it looks like it mishandles short messages because of confusion wrt lenBuf, or something. Still debugging.

from protobuf.

From [email protected] on November 14, 2014 17:06:03

So my theory is that messages shorter than len(lenBuf) are mishandled, in two ways:

1. EOF is triggered one message too early
2. it looks like the unconditional read of len(lenBuf)=10 bytes causes bytes to be lost

Consider two messages, 5 bytes in length, so taking each 1+5 bytes on the wire. The first read will slurp 10 bytes into lenBuf, and while it shuffles the extra from lenBuf to buf, nothing preserves those bytes between ReadMsg calls. Hence, the second message starts decoding a uvarint at byte #11, which is in the middle of the message.

Here's a version using bufio that Works For Me(tm):

func NewDelimitedReader(r io.Reader, maxSize int) ReadCloser {
vr := &varintReader{
r: bufio.NewReader(r),
maxSize: maxSize,
}
if closer, ok := r.(io.Closer); ok {
vr.closer = closer
}
return vr
}

type varintReader struct {
r *bufio.Reader
buf []byte
maxSize int
closer io.Closer
}

func (this *varintReader) ReadMsg(msg proto.Message) error {
length64, err := binary.ReadUvarint(this.r)
if err != nil {
return err
}
msgLen := int(length64)
if len(this.buf) < msgLen {
this.buf = make([]byte, msgLen)
}
if _, err := io.ReadFull(this.r, this.buf[:msgLen]); err != nil {
return err
}
return proto.Unmarshal(this.buf[:msgLen], msg)
}

func (this *varintReader) Close() error {
if this.closer != nil {
return this.closer.Close()
}
return nil
}

from protobuf.

From awalterschulze on November 20, 2014 23:13:07

Yes seems like I really overcomplicated this reader.
Good catch.

Status: Accepted
Labels: -Priority-Medium Priority-Critical

from protobuf.

From awalterschulze on November 20, 2014 23:49:26

Do you want any copyright for this code?

from protobuf.

From [email protected] on November 21, 2014 09:20:27

It's trivial enough that it doesn't matter. Thanks for your work on gogoprotobuf!

from protobuf.

From awalterschulze on November 25, 2014 07:57:38

Thank you for reporting the issue

Status: Fixed

from protobuf.