tafia / quick-protobuf

A rust implementation of protobuf parser
MIT License
452 stars 87 forks source link

Why do the generated bindings read until EOF? #196

Open therealprof opened 3 years ago

therealprof commented 3 years ago

I just noticed that the generated code does something like:

while !r.is_eof() {
    match r.next_tag(bytes) {

which means that if it is fed a buffer with multiple protobuf messages included it will parse each message, fill in the structures and then overwrite it with the next until eof is reached (hopefully exactly at a message boundary without excess bytes) and then return the last of the messages.

Wouldn't it make more sense to only check for EOF once at the beginning of each (sub-)message to return an error and then only parse the first message, allowing the BytesReader state to be reused by the application to parse more messages or gracefully ignore excess data?

sollyucko commented 2 years ago

Or look for a 0 tag?

https://github.com/protocolbuffers/protobuf/blob/520c601c99012101c816b6ccc89e8d6fc28fdbb8/java/core/src/main/java/com/google/protobuf/AbstractMessage.java#L421-L446

    @Override
    public BuilderType mergeFrom(
        final CodedInputStream input, final ExtensionRegistryLite extensionRegistry)
        throws IOException {
      boolean discardUnknown = input.shouldDiscardUnknownFields();
      final UnknownFieldSet.Builder unknownFields =
          discardUnknown ? null : UnknownFieldSet.newBuilder(getUnknownFields());
      while (true) {
        final int tag = input.readTag();
        if (tag == 0) {
          break;
        }

        MessageReflection.BuilderAdapter builderAdapter =
            new MessageReflection.BuilderAdapter(this);
        if (!MessageReflection.mergeFieldFrom(
            input, unknownFields, extensionRegistry, getDescriptorForType(), builderAdapter, tag)) {
          // end group tag
          break;
        }
      }
      if (unknownFields != null) {
        setUnknownFields(unknownFields.build());
      }
      return (BuilderType) this;
    }