Decoder throws exception when decoding an empty map

[kika]

I'm sending a message from the Rust backend which has almost all fields set to default (either 0 or empty string or empty repeated). I understand that in such case protobuf just "optimizes away" such values. Upon receiving this message the Dart code throws a nullcheck exception from runtime.

Here's the .proto code:

syntax = "proto3";
message ProtoPara {
    string id = 1;
}
message ProtoPage {
    uint32  page_id = 1;
    repeated ProtoPara paras = 2;
}
message ProtoBoard {
    string id       = 1;
    string team_id  = 2;
    map<uint32, ProtoPage> pages = 3;
}
message ProtoMessage {
    oneof msg {
        ProtoBoard board = 1;
    }
    optional string request_id = 100;
}

The message only has ProtoBoard.id set to something and also the ProtoMessage.request_id. The pages map contains a default a single value of ProtoPage (page_id is 0 and paras array is empty). A binary dump of the message which is read by protoc --decode without a problem: ChAKDDNIVkJHVGpJODBLZhoAogYRb3BlbmJvYXJkTmpkOTc3ZjA=

$ echo "ChAKDDNIVkJHVGpJODBLZhoAogYRb3BlbmJvYXJkTmpkOTc3ZjA=" | base64 -d |protoc -I . --decode ProtoMessage proto.proto
board {
  id: "3HVBGTjI80Kf"
  pages {
    key: 0
    value {
    }
  }
}

$ dart pub global list
protoc_plugin 20.0.1

protobuf package is at 2.1.0

Stack trace:

packages/protobuf/src/protobuf/pb_map.dart 110:73                                 [_mergeEntry]
packages/protobuf/src/protobuf/coded_buffer.dart 190:48                           _mergeFromCodedBufferReader
packages/protobuf/src/protobuf/generated_message.dart 170:5                       mergeFromCodedBufferReader
packages/protobuf/src/protobuf/coded_buffer_reader.dart 103:12                    readMessage
packages/protobuf/src/protobuf/coded_buffer.dart 122:14                           _mergeFromCodedBufferReader
packages/protobuf/src/protobuf/generated_message.dart 185:5                       mergeFromBuffer
packages/myprotocol/generated/myprotocol.pb.dart 1195:130                  <fn>
packages/myprotocol/generated/myprotocol.pb.dart 1195:150                  fromBuffer
packages/myapp/backend/websocket.dart 129:42                                 receive
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 325:40               runBody
dart-sdk/lib/async/zone.dart 1685:54                                              runUnary

(skipped all below zone)

Changelog refers to some similar bug fixed in 2.0.1 but I can' seem to be able to find it.

[kika]

FWIW: This happens on JS runtime

When the map contains a Page that is not empty, everything works as expected.

[kika]

No difference on master:

packages/protobuf/src/protobuf/pb_map.dart 108:73                                 [_mergeEntry]

[osa1]

Thanks for reporting this. I simplified the repro a little bit:

syntax = "proto3";

message ProtoPara {
    string id = 1;
}

message ProtoPage {
    uint32  page_id = 1;
    repeated ProtoPara paras = 2;
}

message ProtoBoard {
    string id       = 1;
    string team_id  = 2;
    map<uint32, ProtoPage> pages = 3;
}

void main() {
  List<int> protoBoardBytes = [
    10, // tag = 1, wire type = 2 (length delimited), type = string
    12, // length = 12
    51, 72, 86, 66, 71, 84, 106, 73, 56, 48, 75, 102, // <-- the string for "team_id" field
    26, // tag = 3, wire type = 2 (length delimited), type = map<uint32, ProtoPage>
    0 // length = 0
  ];
  print(ProtoBoard.fromBuffer(protoBoardBytes));
}

I annotated the wire message parts.

There may be multiple issues involved here, but one problem seems to be that we don't handle 0 as length prefix in PbMap._mergeEntry. That method has no early return when the length prefix is 0, and tries to add an entry to the map regardless (using defaults for the key and value types when they are not parsed). In the repro we create an empty string for the key (as that's the default string value), but for the value we can't create the default value for ProtoPage for some reason. I think we should avoid adding an entry when the length is 0.

I don't understand why the code fails to create a default value for the map value type here (ProtoPage). I suspect this may be another bug.

It's also a bit strange that the protobuf implementation that generates this encoding generates an empty field for the pages field in ProtoBoard. It could just omit the field entirely. I think that may be a bug in the protobuf implementation that generates this encoding.

[kika]

@osa1 speaking of your comments in the code: team_id is empty, it's the id that has string value.

I'll look into the Rust implementation as to why they emit the empty field for the pages. But maybe it's because the field is not technically empty, but contains one empty (default) element?

[kika]

@osa1 Ugh, the dump looks different for me:

$ echo "ChAKDDNIVkJHVGpJODBLZhoAogYRb3BlbmJvYXJkTmpkOTc3ZjA=" | base64 -d | od -t u1
0000000    10  16  10  12  51  72  86  66  71  84 106  73  56  48  75 102
0000020    26   0 162   6  17 111 112 101 110  98 111  97 114 100  78 106
0000040   100  57  55  55 102  48

It doesn't end with zero.

[osa1]

@kika the part I show in my repro is not your original message, it's the part of it that triggers the bug.

I'm using this Python 3 expression to generate Dart int lists: list(base64.b64decode(<b64 encoding>). Your original repro:

>>> list(base64.b64decode('ChAKDDNIVkJHVGpJODBLZhoAogYRb3BlbmJvYXJkTmpkOTc3ZjA='))
[10, 16, 10, 12, 51, 72, 86, 66, 71, 84, 106, 73, 56, 48, 75, 102, 26, 0, 162, 6, 17, 111, 112, 101, 110, 98, 111, 97, 114, 100, 78, 106, 100, 57, 55, 55, 102, 48]

The part starting with 12, 51, ... and ending with 26, 0 is a ProtoBoard encoding and it's the part that triggers the bug.

[osa1]

it's because the field is not technically empty, but contains one empty (default) element?

In proto3, without optional, there is no difference between non-presence of a field and having the default value for the field.

Unfortunately protobuf spec is not super clear about this stuff and you need to read design documents from the protobuf GitHub repo to figure it out. We have some relevant discussions in #691.

Since you don't have optional in your proto definition, the program that encodes your proto values could skip fields with default values. It should make no difference on the receiving end.

That said, we also don't follow this rule in the Dart implementation. proto3 spec says default values should be omitted by default in JSON encoding (#585, #592). There should be some mention of the same thing in binary format as well, but I can't find it now, so maybe I'm misremembering?

[kika]

So it's a bug to produce this empty map and a bug to explode when seeing this empty map on the wire, right?

[osa1]

So it's a bug to produce this empty map and a bug to explode when seeing this empty map on the wire, right?

I can't find where in the spec it says default values should be omitted (with no presence checking, i.e. no optional), but I think there's some mention of that. If that's right then the empty map should be omitted, yes.

However on the receiving end we should still be able to handle it, because it could be an optional field where non-presence (i.e. the empty part is missing) would be different than the default value (i.e. with the empty part).

So in short:

• This is a bug in protobuf.dart
• I'm not sure if there's a bug in the protobuf implementation that generates this encoding

[osa1]

I can't find where in the spec it says default values should be omitted (with no presence checking, i.e. no optional), but I think there's some mention of that. If that's right then the empty map should be omitted, yes.

I found where this is mentioned: https://github.com/protocolbuffers/protobuf/blob/main/docs/field_presence.md#presence-in-tag-value-stream-wire-format-serialization

When serializing, fields with no presence are not serialized if they contain their default value.

So without optional an empty map should not be serialized.

As mentioned in my previous comment, we also don't follow this in protobuf.dart.