Issues with MessagePack implementation of IHubProtocol
- Dominant language
- C#
- Stars
- 38.4k
- Forks
- 10.9k
- Avg merge
- 2d 6h
- Merged PRs (30d)
- 290
Description
### Describe the bug
In writing [a Nerdbank.MessagePack implementation of `IHubProtocol`](https://github.com/AArnott/Nerdbank.MessagePack/pull/520), I've been studying SignalR's [existing implementation](https://github.com/dotnet/aspnetcore/blob/main/src/SignalR/common/Protocols.MessagePack/src/Protocol/MessagePackHubProtocolWorker.cs) based on MessagePack-CSharp. I've noticed several potential issues that I will collect here for your consideration:
## Real bugs
1. Serialization requires knowing the declared types of arguments and return values. It is _not_ enough to simply call `object.GetType()` and serialize with that. We need to know that the parameter is typed as `BaseType` and that the argument is an instance of `DerivedType` so that `[Union]` attributes on `BaseType` can be serialized. As you have it here where the serializer relies on `GetType` on the value to be serialized, a parameter or return type declared as `BaseType` but has an instance of `DerivedType` will serialize as a 'naked' DerivedType. But upon **de**serialization where we _do_ have `BaseType` as a known type, the deserializer will look for a type disambiguating header to precede the `DerivedType` data. But it won't be there because `BaseType` wasn't a known type at serialization time. As a result, your SignalR implementation will fail when users try to send types with `[Union]` attributes on them. It also means my own implementation for Nerdbank.MessagePack will have the same bug, because your `IHubProtocol` interface simply doesn't make this information available.
2. The [spec](https://github.com/dotnet/aspnetcore/blob/main/src/SignalR/docs/specs/HubProtocol.md#messagepack-msgpack-encoding) mandates a fixed protocol version of 1. But your code currently sets it to be 2. And while your code allegedly _accepts_ a version 1 as compatible, if the version is exchanged both directions, surely your version 1 implementation on the other side will reject version 2 anyway. Am I missing something?
## Other issues for your consideration
1. The protocol version being part of the protocol really ought to be communicated with the `IHubProtocol` implementation. At the moment, all the protocol version evidently does is to serve as an accept/reject decision. But it would be much more useful if the `IHubProtocol` also knew which version the remote party was using so that any necessary adjustments could be made in the protocol.
1. The message deserializing functions do not adequately guard against reading too much. While it calls `ReadArrayHeader`, most paths disregard its value and read as many elements as they want. In fact the only paths that use the return value are those that expect an _optional_ value in the array. This issue is a lower priority than it would otherwise be because the `MessagePackReader` works off of a bounded buffer so that it cannot read more than one message. So this is _ok_ for now.
2. The message deserializing functions do not guard against reading _too little_. Typically when reading an array, you should make sure you've read or skipped every single element in the array. Again, the fact that the top-level read function has guaranteed to have consumed the entire envelope makes this less of a manifesting bug. But it does prevent you from noticing when there's garbage at the end of a message.
3. The schema is fundamentally incompatible with using standard serialize/deserialize functions from msgpack libraries because it mixes both the disambiguating 'type' integer into the same array that stores the object's fields. This makes it impossible to have each message serialize/deserialize itself, and have a higher-level method write/read the message type identifier first. The best practice here would be something like this: `[1, [message, fields, here]]` instead of `[1, message, fields, here]`. Fixing this would be a protocol breaking change.
### Expected Behavior
Ideally a messagepack protocol that follows best practices in the messagepack community.
Contributor guide
Assessment
This issue has not been assessed yet.