protobuf-net does not conform protobuf spec

[Radrik5]

It seems like ImplTryReadUInt32VarintWithoutMoving should not throw OverflowException when it encounters int64:

protobuf-net/src/protobuf-net.Core/ProtoReader.Stream.cs

Line 228 in 618ee18

state.ThrowOverflow();

According to protobuf docs:

int32, uint32, int64, uint64, and bool are all compatible – this means you can change a field from one of these types to another without breaking forwards- or backwards-compatibility. If a number is parsed from the wire which doesn't fit in the corresponding type, you will get the same effect as if you had cast the number to that type in C++ (e.g. if a 64-bit number is read as an int32, it will be truncated to 32 bits).
sint32 and sint64 are compatible with each other but are not compatible with the other integer types.

https://developers.google.com/protocol-buffers/docs/proto#updating

[mgravell]

Interesting. I can't argue with your citation, but a part of me feels that it is better to not silently change data, but to make this loud. Needs consideration, IMO.

[Radrik5]

I think it's up to protocol designer of a specific .proto file to decide if stripping most significant bytes for a particular field is acceptable or not. Protobuf permits it as backwards compatible change.

protobuf-net could log warning or do something else loudly but the message must be parsed successfully.

[mgravell]

Well, protobuf-net doesn't always (or perhaps even "doesn't usually") start from .proto; since this is the first time it has come up in over a decade, I'm fairly comfortable that it isn't causing huge problems, and in many ways I'm more comfortable with a loud overflow than silently trimming data even if that is what the Google implementation does.

What I would be comfortable with, however, is something akin to an "allow overflow" flag that could be enabled on such fields, and a tweak to theOverflowException message advising people on how to enable that flag if they really intended that.

Thoughts?

[Radrik5]

I think it's not just "what the Google implementation does", it's what any implementation should do if it claims to conform protobuf spec. It's a part of specification, not implementation detail.

Here's our example. We have service with public protobuf API. The service is evolving, API is changing but we keep backwards compatibility. Clients compiled with old protocol version can connect to servers compiled with new protocol version. Some clients use protocol versions that are several years old. We don't know what programming languages or protobuf implementations they use but we assume they conform protobuf spec. This is the first case when backwards compatible change broke client application. It used protobuf-net v2.

The "allow overflow" flag wouldn't have helped in this case. You never know which field will eventually change its type to int64.

[mgravell]

Let's take that scenario to the logical conclusion. You have a new app that uses a 64 bit field, and you send data to an old app that uses a 32 bit field. If the 64 bit value happens to be valid in the 32-bit range, then: no harm, no foul - protobuf-net already works. The scenario you're talking about is what happens when the 64-bit app uses a value *higher* than 32 bits, and sends that value to the 32-bit app. At this point, one of two things can happen: 1. your app can silently screw up the data and do undefined things because it is working on garbage 2. it can throw an exception The one thing that is *not* an option is that it carries on working correctly and happily. That simply not an option available to us. You seem to be arguing for option 1. Frankly, that is a bad option. Being in favour of undefined behaviour is not something I'm keen on, especially as the default. Allow me to say bluntly: the standard behaviour is wrong, and I make no apology for not following blindly in that wrongness. I'm more than happy to consider 3. make it throw by default, but allow the user to opt into undefined bad data if that's what they really want I can even try adding custom metadata to .proto so that you can define that from .proto, as well as C# Given that (to repeat) it has come up one in over a decade, I think that's more than fair, to be honest.

…

On Thu, 17 Dec 2020, 19:50 Dmitry Oksenchuk, ***@***.***> wrote: I think it's not just "what the Google implementation does", it's what any implementation should do if it claims to conform protobuf spec. It's a part of specification, not implementation detail. Here's our example. We have service with public protobuf API. The service is evolving, API is changing but we keep backwards compatibility. Clients compiled with old protocol version can connect to servers compiled with new protocol version. Some clients use protocol versions that are several years old. We don't know what programming languages or protobuf implementations they use but we assume they conform protobuf spec. This is the first case when backwards compatible change broke client application. It used protobuf-net v2. The "allow overflow" flag wouldn't have helped in this case. You never know which field will eventually change its type to int64. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#746 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAEHMHHCQ5OVCECTMTW7WDSVJOGRANCNFSM4U5WEXIA> .

[Radrik5]

1. your app can silently screw up the data and do undefined things because
   it is working on garbage

Wrong. Old app silently modifies data in a way defined in protobuf spec. You can predict what will happen in the old app and decide if it's acceptable or not. In many cases it's acceptable. For example, bit mask that used to be int32 changed to uint64. Old apps can drop new bits, they don't know them anyway.

I agree that "fail early and fail loudly" is a good design principle and I appreciate that protobuf-net follows it. But Google developers designed Protobuf with a bit different principle in mind: "Be conservative in what you send, be liberal in what you accept." Change of int32 to int64 is not the only example when old app should silently drop data:

For string, bytes, and message fields, optional is compatible with repeated. Given serialized data of a repeated field as input, clients that expect this field to be optional will take the last input value if it's a primitive type field or merge all input elements if it's a message type field.

In many cases this is also acceptable.

In my humble opinion current behavior in protobuf-net is a bug and there is no sense to implement protobuf library that doesn't conform protobuf spec. It will have problems with communication with other implementations as it will parse messages differently. But I've never used protobuf-net and don't see the full picture.

If you still think that current implementation is better, I suggest documenting it explicitly. For example, to write on the main page: We think that some rarely used parts of protobuf spec are wrong and we didn't implement them. Protobuf-net can fail to parse messages that other implementations parse successfully but modify data. See (link to Compatibility Notes) for details.

In addition to that you could add global 'full compatibility mode' flag for users that need it. It has to be global, not per-field, because you don't know which filed will change in the future.

[mgravell]

Allowing such a flag to apply at multiple levels is trivial - see for example: CompatibilityLevel (although I'm not proposing to use the same approach - my point is more about the multiple levels).