icerpc / icerpc-csharp

A C# RPC framework built for QUIC, with bidirectional streaming, first-class async/await, and Protobuf support.
https://docs.icerpc.dev
Apache License 2.0
107 stars 13 forks source link

Decoding of Slice1 tagged proxy that holds null broken in 0.3 #3930

Closed bernardnormier closed 8 months ago

bernardnormier commented 8 months ago

In IceRPC 0.3.0 (but no 0.2.x, 0.1.x), the decoding of a Slice1-encoded tagged proxy that holds the null value (a very uncommon situation) is broken.

The issue is that in 0.3.0, we decode Slice1-encoded tagged proxies/service addresses with DecodeServiceAddress--the non-nullable extension method--when we should be using DecodeNullableServiceAddress like in previous versions.

I propose to fix this bug as follows:

That's what we did in previous versions. It works well but is a bit inconsistent, since we're using EncodeXxx for encoding and DecodeNullableXxx for decoding.

And the reason we're not simply doing the 0.4.x fix on 0.3.x is because changing the EncodeTagged extension on 0.3.x would most likely break binary compatibility.

It's also possible we can find a single binary-compatible fix that works for both branches.

Also, to be clear:

InsertCreativityHere commented 8 months ago

Fixed by #3929!


It works well but is a bit inconsistent, since we're using EncodeXxx for encoding and DecodeNullableXxx for decoding.

This inconsistency has real consequences if there's a difference in encoding between nullable and non-nullable versions of your type (which should nearly always be true). If EncodeFoo sends a [string], but DecodeNullableFoo expects a [bool][string] the decoding will fail (the bool encodes whether it's set or not).

This bug is why I opened #3919: we really do need to be using the same function on both sides (encoding/decoding).

3919 just happened to pick the wrong function, because I forgot about this capability of Ice, and only considered the IceRPC docs.