Missing length-check in WireFormatLite::ReadMessageNoVirtual?

[Rawler]

Hi,

Trying out ProtoBuffers for streaming. I was hoping to exploit some
internal ProtoBuf-code for a streaming-protocol. The streaming-
protocol is basically the same as the message-internal protocol used
by ProtoBuf:

[MSGTYPE(varint)] [LENGTH(varint)] [MESSAGE(blob)].

My attempt was to simply CodedInputStream::ReadTag() +
internal::WireFormatLite::ReadMessageNoVirtual() (which should AFAIU
read a length-prefixed message). However, I hit an error that I traced
down to ReadMessageNoVirtual returning true, even when the not the
entire LENGTH of the message was available on the CodedInputStream.

For my particular example, I'll probably rewrite it to read the outer-
message length externally in CodedInputStream and have the checks
there, but couldn't this be a failing validation for embedded streams
as well? I.E. one message of say, 40 bytes claiming to include a 60-
byte sub-message?

It seems to me that particular condition could be easily checked for
by extending ReadMessageNoVirtual with a check:

uint32 length;
if (!input->ReadVarint32(&length)) return false;
+ int bytesLeft = input->BytesUntilLimit();
+ if ((bytesLeft >= 0) && (length > bytesLeft)) return false;
if (!input->IncrementRecursionDepth()) return false;
io::CodedInputStream::Limit limit = input->PushLimit(length);

Just a thought.

[Eyal Farago]

your claims does not match the code of both CodedInputStream::PushLimit and WireFormatLite::ReadMessageNoVirtual.

1. PushLimit does check for a larger limit inside a smaller one (respects the closer limit). see http://code.google.com/p/protobuf/source/browse/trunk/src/google/protobuf/io/coded_stream.cc#119

2. ReadMessageNoVirtual reads the message length and passes it to PushLimit before actually parsing the message, when the masses is fully parsed, ConsumedEntireMessage is used to determine if there's anything left in the stream. see http://code.google.com/p/protobuf/source/browse/trunk/src/google/protobuf/wire_format_lite_inl.h#408.

please notice that there's no guarantee that the stream reached EOF once this ReamMessageNoVirtual returns, as this message may be a part of another one, my guess is that you didn't push a limit into the coded stream before calling this method.

please let me know if this is helpful,

Etal.

[Rawler]

Yes, you are correct. I did not push a limit before. It was non-obvious this was needed, which can of course be expected in ::internal, so no worries.

When I got the effect that the code wasn't detecting premature end of stream, I assumed it was due to the return value of input->PushLimit(length) not being checked in WireFormatLite::ReadMessageNoVirtual. The comment above the ConsumedEntireMessage-check in WireFormatLite::ReadMessageNoVirtual made me believe it was just related to the deprecated group-functionality:

// Make sure that parsing stopped when the limit was hit, not at an endgroup

// tag. Anyways since the ConsumedEntireMessage DOES in fact cover this as well, I see no problems. Sorry for the confusion.