[csharp] increase default recursion limit

[EronWright]

The inability to configure the recursion limit for .NET programs is a long-standing problem (see https://github.com/grpc/grpc/issues/22682), and the latest attempt at making it configurable has seemingly stalled (see https://github.com/protocolbuffers/protobuf/issues/15773).

Maintaining a fork of Google.Protobuf is the typical workaround, but is brittle. Let's re-consider increasing the default limit from 100 to 500.

Note that this PR targets the C# language only, because the other languages are assumed to be configurable.

[jskeet]

I don't mind changing the default limit, but I wouldn't want to do so without input from the rest of the Protobuf team. @mkruskal-google do you have thoughts here?

Does the increase to 500 come from empirical testing, or is it just "something much bigger than 100"? I'd generally prefer to take a more conservative approach, e.g. just doubling to 200, unless we have a reason to believe that we're likely to see a need for more than that.

[mkruskal-google]

In C++ we default to 100 but allow users to override it. Why not take that approach here? Just bumping it arbitrarily seems like a bandaid that might not hold. It also might expose users to vulnerabilities, I'm not sure what the maximum stack depth in C# is

[jskeet]

@mkruskal-google: It's already configurable on CodedInputStream, but that's relatively fiddly to use. There was a proposal to introduce a new SerializationOptions, but that wasn't going to be backwardly compatible. I don't personally have time to create a backward-compatible "nice" way of configuring this, so I think this PR was an attempt to alleviate some of the problems without requiring the full design aspects.

[EronWright]

Thanks @jskeet for considering this, and @mkruskal-google for weighing in.

At Pulumi, we maintain a fork of Google.Protobuf with a recursion limit of 1000, which is generous but not unreasonable. I think we'd advocate for at least 300 (a tripling of the current default). I initially proposed 500 to be closer to our tested value. See also: https://rosettacode.org/wiki/Find_limit_of_recursion#C#

Our use-case involves marshaling a user-supplied Kubernetes CRD as a struct (see example). CRDs tend to be deeply nested, and we observe that each level of nesting brings 3 levels of recursion. In other words, a limit of 300 would give us a max nesting of 100, which is probably good enough.

[mkruskal-google]

It seems a little strange for C# to diverge from other languages in the default behavior. That means a proto that C# accepts could be rejected by other runtimes. This is also an irreversible change, once we make it we won't be able to undo it w/o a breaking release. But I'm not strongly opposed to this, it seems like a decent mitigation for the underlying issue

[EronWright]

Shall I change the default in the other languages too, for consistency? Was seeking to minimize scope but have no other objection.

[mkruskal-google]

No, that would probably require much more careful review. If Jon is ok with this for C# I don't mind it, but in other languages it may put things at risk

[jskeet]

I'm going to have another look at the gRPC issue, and see how gRPC performs parsing, to see if there's an alternative that doesn't take C# out of kilter with everything else. I'm on vacation for a week starting tomorrow, so unless it just "drops out" this morning, it'll be a bit longer. If I can't see a fairly straightforward, backward-compatible way of allowing gRPC to set its own recursion limit, I'm happy for us to go ahead with this change instead.

[jskeet]

Okay, I've had a look.

It's definitely feasible, but it's somewhat fiddly. Fortunately only in a relatively small number of places, but even so, we need to think carefully before merging. I've created #17912 for feedback.

[github-actions[bot]]

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active, please add a comment.

This PR is labeled inactive because the last activity was over 90 days ago. This PR will be closed and archived after 14 additional days without activity.

[github-actions[bot]]

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active or becomes active again, please reopen it.

This PR was closed and archived because there has been no new activity in the 14 days since the inactive label was added.

[ThaDaVos]

Please re-open this - it seems Pulumi is still looking for this and as I just did an update to the dotnet library I run into this specific issue