Support configuring a deeper recursion limit for protobuf deserialization

[timarmstrong]

Is your feature request related to a problem?

We have a system that uses a recursive protobuf structure to represent some internal data structures.

We want to use GRPC-Java in some more places, but have run into cases where we hit the current recursion limit:

com.google.protobuf.InvalidProtocolBufferException: Protocol message had too many levels of nesting.  May be malicious.  Use CodedInputStream.setRecursionLimit() to increase the depth limit.
        at com.google.protobuf.InvalidProtocolBufferException.recursionLimitExceeded(InvalidProtocolBufferException.java:104)
        at com.google.protobuf.CodedInputStream.readMessage(CodedInputStream.java:491)

In the current version of the system, which directly deserializes the protobufs instead of going via GRPC, we set a different recursion limit (say 256) based on stress testing. We know other parts of the system can handle more than 100-deep recursion.

Describe the solution you'd like

We'd like to be able to set CodedInputStream.setRecursionLimit() for a GRPC service. It probably works fine for us if it's at the scope of a server too.

Describe alternatives you've considered

• Using a different RPC stack
• Patching GRPC for our own use
• Refactoring protobufs to avoid nesting (this is quite painful and doesn't really avoid recursion depth issues because other parts of our system will hit limitations on stack size before the protobuf deserialisation).

Additional context

I saw grpc/grpc#8256 and this is basically a duplicate of that, I hope additional context on the use case is helpful.

I'm also open to trying to implement this if there is a chance that the project will accept it.

[temawi]

This is discussed in grpc/grpc-dotnet#1983 and you can see some options there.

[ejona86]

The Marshaller change would be fairly small, with some API to configure the max recursion limit and set it at the same time we're setting the message limit:

grpc-java/protobuf-lite/src/main/java/io/grpc/protobuf/lite/ProtoLiteUtils.java

Lines 210 to 212 in 38311e8

	// Pre-create the CodedInputStream so that we can remove the size limit restriction
	// when parsing.
	cis.setSizeLimit(Integer.MAX_VALUE);

You'd use methodDescriptor.toBuilder() to swap out the MethodDescriptor. The existing marshaller implements PrototypeMarshaller which would allow you to recreate the Marshaller without knowing what the existing type is (just assuming it is protobuf). That is all pretty easy within a ClientInterceptor.

Server-side is more annoying, and requires repackaging ServerServiceDefinition. ServerInterceptors.useMarshalledMessages() may be the best example. We have that method because it was annoying. We could maybe introduce a new helper method that you provide a Function<MethodDescriptor,MethodDescriptor> to make the rewriting easier.

[timarmstrong]

Yeah anything that is feasible to do without changes to grpc-java would be helpful!

[timarmstrong]

@ejona86 is there anything I could do to help here? I'm happy to have a try at implementing something as well, if it's accessible enough to someone new to the codebase.

[ejona86]

@timarmstrong, yeah, you could contribute the needed changes. You'd need a new API like ProtoLiteUtils.marshallerWithRecursionLimit(T, int)[1] (and add a forwarding API in ProtoUtils). You'd add a parameter to ProtoLiteUtils.MessageMarshaller's constructor, maybe passing -1 from the existing marshaller() method where we don't want to change the default.

It is a small amount of plumbing.

1. We could have a builder to configure it, but there's expected to be few settings like this. The other API I could envision is ProtoLiteUtils.marshallerWithRecursionLimit(Marshaller<T>, int) where you pass and existing marshaller and it creates a new one with the changed configuration.

[kloyan]

Hi @ejona86. I took a shoot at resolving this issue by following your suggestion above. Would appreciate your feedback.

[sanjaypujare]

Hi @ejona86. I took a shoot at resolving this issue by following your suggestion above. Would appreciate your feedback.

I think you are talking about PR #10094 ? Assigned the PR to @ejona86 .

[timarmstrong]

Thank you!

…

On Tue, 2 May 2023 at 14:52, Eric Anderson ***@***.***> wrote: Closed #9643 <#9643> as completed via #10094 <#10094>. — Reply to this email directly, view it on GitHub <#9643 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAI2HKYZMNH3R7SXWRPRAWDXEF62HANCNFSM6AAAAAARKU44IY> . You are receiving this because you were mentioned.Message ID: ***@***.***>