-
Notifications
You must be signed in to change notification settings - Fork 759
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Provide a way to configure message marshallers #1983
Comments
The static generated code for instantiating marshallers is e.g. here: For clients, the Marshaller is being passed to the call invocation logic here: And for server the handlers are created like this: |
Thank you @jtattermusch for filing this! Can anyone give me a rough idea of when this could be implemented so that I could plan my downstream mitigation accordingly? |
@mikhailshilkov it looks like this would take some careful design work first (I don't currently have a straightforward way to implement this and it's probably going to be a bit tricky), so it might take a while. Ideas for how this could be implemented are welcome. |
What does Java/Go do here? |
@ejona86 I took a look and it seemed that Java also doesn't have a good way of configuring (e.g. setting recursionLimit etc.) the CodedInputStream that is used for (un)marshalling protobuf messages in the generated grpc stubs. Can you confirm that? If so, is there any plans for providing such feature? |
Yes, gRPC Java does not support setting recursion limit. We have a global for setting extension registry, although I can't say I'm particularly fond of that solution. There's only two settings on CodedInputStream: setRecursionLimit and setSizeLimit. We unconditionally setSizeLimit to MAX_INT, as gRPC manages that limit. So that means the only configuration missing is setRecursionLimit, and nobody's complained about that. If Java needed to provide a way to configure that, I think I'd add a new ProtoUtil API to create a Marshaller with a specified limit and then have users manually create the Marshaller and replace the one that's in the generated MethodDescriptor. That's annoying (mainly on server-side), but feasible for users. Since this is clearly rarely needed, that seems fine. |
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! |
Shall I come here and leave a "we still need this" comment every 30 days? 🤔 |
Since Grpc.Core.Api now lives in this repository, I transferred the issue from grpc/grpc and marked it with a new "api" label. |
Would be nice to have a way to configure these limits when using other overloads for decoding data, as opposed to only offering this functionally with the CodedInputStream API. |
In C#, the raw message payload are being deserialized/serialized using contextual marshallers (which are instantiated directly from the generated code of protobuf services).
Currently there is no way to provide any additional configuration to these marshallers, which causes problems such as
grpc/grpc#22682.
Since being able to configure marshallers (e.g. providing recursionLimit for protobuf deserialization) is a very legitimate request, we should looks into ways how we could make marshallers configurable.
The text was updated successfully, but these errors were encountered: