C#: getting RecursionLimitExceeded with no way to configure

[mikhailshilkov]

What version of gRPC and what language are you using?

C#, 2.24

<PackageReference Include="Google.Protobuf" Version="3.10.0" />
<PackageReference Include="Grpc" Version="2.24.0" />
<PackageReference Include="Grpc.Tools" Version="2.24.0">

What operating system (Linux, Windows,...) and version?

macOS Catalina 10.15.3

What runtime / compiler are you using (e.g. python version or version of gcc)

.NET Core 3.1

What did you do?

I'm sending a message with high level of recursion in it (a map containing a map containing a map etc.)

[map[apiVersion:apiextensions.k8s.io/v1beta1 kind:CustomResourceDefinition spec:map[validation:map[openAPIV3Schema:map[properties:map[spec:map[properties:map[solver:map[properties:map[http01:map[properties:map[ingress:map[properties:map[podTemplate:map[properties:map[spec:map[properties:map[affinity:map[properties:map[podAffinity:map[properties:map[preferredDuringSchedulingIgnoredDuringExecution:map[items:map[properties:map[podAffinityTerm:map[properties:map[labelSelector:map[properties:map[matchExpressions:map[items:map[properties:map[values:map[type:array]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]

What did you expect to see?

C# client would successfully parse the gRPC response.

What did you see instead?

I get an exception:

Grpc.Core.RpcException: Status(StatusCode=Internal, Detail="Failed to deserialize response message.")
   at Pulumi.GrpcMonitor.InvokeAsync(InvokeRequest request)

The actual exception is swallowed but I got it with a debugger:

Google.Protobuf.InvalidProtocolBufferException: Protocol message had too many levels of nesting.  May be malicious.  Use CodedInputStream.SetRecursionLimit() to increase the depth limit.
   at Google.Protobuf.CodedInputStream.ReadMessage(IMessage builder)
   at Google.Protobuf.FieldCodec.<>c__DisplayClass32_0`1.<ForMessage>b__0(CodedInputStream input)
   at Google.Protobuf.FieldCodec`1.Read(CodedInputStream input)
... (a very long call stack follows)

It happens on this line in Protobuf's CodedInputStream.

Suggested solution

Please allow setting the max recursion level in the channel options.

[jtattermusch]

The configuration doesn't really belong to the channel options, but we should think about how to make (de)serialization more configurable (we should go through this exercise when integrating the new protobuf C# parser from protocolbuffers/protobuf#7351).

@JamesNK any thoughts?

[JamesNK]

Configuration either needs to happen at runtime on channel - that would require a way to pass the configuration from the channel into the marshaller - or put it on <Proto> element and have it baked into code generation.

Channel seems like a better place, but configuration shouldn't be tied to Google.Protobuf.

It's a tricky problem.

[sfmskywalker]

I just noticed this comment from @anandolee on another issue:

Update: enable allow_oversize_protos also sets RecursionLimit max now.

@mikhailshilkov I don't know yet what is this thing called allow_oversize_protos, but might it provide a workaround for this reported issue?

[sfmskywalker]

Never mind my previous comment, I was looking at a completely different repo. 🤦

[mikhailshilkov]

@sfmskywalker I guess this depends on whether this setting is exposed in the C# SDK. I have no idea yet...

[stale]

This issue/PR has been automatically marked as stale because it has not had any update (including commits, comments, labels, milestones, etc) for 30 days. It will be closed automatically if no further update occurs in 7 day. Thank you for your contributions!

[gitfool]

Digging into the details, I see CodedInputStream.ReadMessage calls ParsingPrimitivesMessages.ReadMessage where it throws InvalidProtocolBufferException. The message says:

Use CodedInputStream.SetRecursionLimit() to increase the depth limit.

I see this is available for Java at CodedInputStream.setRecursionLimit but not for C#.

@jtattermusch @JamesNK could the equivalent be added to the C# class? 🙏

This is a showstopper as it is, so are there any workarounds in the meantime?

[gitfool]

@mikhailshilkov Pulumi should fork this repo and remove the recursion limit in the meantime, otherwise dotnet support is hosed.

[mikhailshilkov]

I see this is available for Java at CodedInputStream.setRecursionLimit but not for C#.

CodedInputStream does have an overload that accepts recursionLimit. However, Grpc uses MessageParser class which already hides that option.

I created a separate issue in the protobuf repo protocolbuffers/protobuf#7995

Pulumi should fork this repo and remove the recursion limit in the meantime

There's no point in forking this repo. It's Google.Protobuf that we'd need to fork.

[jtattermusch]

I filed grpc/grpc-dotnet#1983 to address the root cause of this problem.

[aclemmensen]

We've resorted to patching the internal state of CodedInputStream to work around the unconfigurable default. This is probably about as ugly as it gets. 🙈 This is also keeping us from using the new proto parsing code as it uses a const in a struct we can't patch up externally. We've had to use this hack in all C# code that deals with this specific message type.

// DOM aspects can be very deeply nested. Unfortunately, the recursion limit used when deserializing gRPC messages is 
// not configurable through any publicly exposed API and the default is only 100. This very ugly hack intercepts calls 
// to Dom.Document.Mergefrom(CodedInputStream) and uses reflection to alter the private member `recursionLimit` on
// the given `CodedInputStream` instance. The author feels dirty. 
//
// IMPORTANT: This hack works with Grpc.Tools 2.32.0 and Google.Protobuf 3.13.0. If you upgrade any of these please 
// make sure that this hack still works!
class UglyRecursionLimitMonkeyPatchHack
{
	public static void Apply()
	{
		var harmony = new Harmony("com.siteimprove.pidan");
		harmony.PatchAll();
	}

	[HarmonyPatch(typeof(Dom.Document), "MergeFrom", typeof(CodedInputStream))]
	class Patch
	{
		public static void Prefix(ref CodedInputStream input)
		{
			var stateField = typeof(CodedInputStream).GetField(
				"state", BindingFlags.NonPublic | BindingFlags.Instance);

			var state = stateField.GetValue(input);
			var recursionLimit = state.GetType().GetField("recursionLimit", BindingFlags.NonPublic | BindingFlags.Instance);
			recursionLimit.SetValue(state, 1500);
			stateField.SetValue(input, state);
		}
	}
}

[jtattermusch]

I'm going to close this issue since it's basically a duplicate of grpc/grpc-dotnet#1983 (once we have configurable marshallers, configuring the RecursionLimit for protobuf parsing will be easy).