Fix use-after-free metadata corruption in C# when receiving response headers for streaming response calls #27382
Conversation
…headers for streaming response calls
jtattermusch
added
release notes: yes
jtattermusch
changed the title
jtattermusch
changed the title
|
@donnadionne we should probably include this fix in v1.41.x release once it gets merged. |
| @@ -62,6 +62,7 @@ internal abstract class AsyncCallBase<TWrite, TRead> : IReceivedMessageCallback, | |||
|
|
|||
| protected bool initialMetadataSent; | |||
| protected long streamingWritesCounter; // Number of streaming send operations started so far. | |||
| protected bool receiveResponseHeadersPending; // True if this is a call with streaming response and the recv_initial_metadata_on_client operation hasn't finished yet. | |||
There was a problem hiding this comment.
nit: this comment should read, "True if this is a server-streaming or bidi-streaming call...."
since this isn't used for client streaming calls
There was a problem hiding this comment.
"a call with streaming response" is a bit clumsy way of saying "server-streaming or bidi-streaming call", so strictly speaking the meaning is identical, but I agree that your wording is better.
| @@ -236,6 +236,7 @@ public void StartServerStreamingCall(TRequest msg) | |||
| Initialize(details.Channel.CompletionQueue); | |||
|
|
|||
| halfcloseRequested = true; | |||
| receiveResponseHeadersPending = true; | |||
There was a problem hiding this comment.
UnsafeSerialize can throw, right? Looks like receiveResponseHeadersPending would never be unset in that case.
There was a problem hiding this comment.
that's right, but unless callStartedOk = true is set (and it's only set if all the previous steps succeed), the finally statement will run OnFailedToStartCallLocked(); , which automatically disposes the call. In that case the value of receiveResponseHeadersPending won't matter at all (since the call is already destroyed).
|
Python bazel test failure is unrelated: https://source.cloud.google.com/results/invocations/9aba656b-1de8-491b-ba10-afd520cf8026/log |
copybara-service
bot
added
the
imported
…headers for streaming response calls (grpc#27382)
Fixes #25219 and #25004 (also see internal b/158826903)
I've been trying to debug this problem for long time, and I finally found the rootcause (with help of modifications that allowed me to reproduce more consistently and with more logging, see #23564).
The problem is that for calls with streaming response (server streaming and full duplex), gRPC C# registers for "recv_response_headers_on_client" event when the call starts and when the duration of the call is very short, sometimes the "recv_response_headers_on_client" can be delivered AFTER receiving all the responses and after delivering the final status of the call. If that happens, the call handle gets disposed (which destroys the underlying grpc_call native object), which can trigger unreferencing of the metadata that hasn't yet been delivered to the C# layer.
So in the "HandleReceivedResponseHeaders", when the initial metadata is being read from the native grpc_metadata_array object, it can be already unreffed by C core and due to the "use-after-free" it's basically changing under our hands as we read it. This was the reason why malformed metadata was being encountered in the InteropClientServerTest.CustomMetadata test.
This problem only affected calls with streaming response that 1. read the response headers 2. have a very short duration (so that the response headers that happens at the very beginning of the call can be delivered after all the responses and final status have been delivered - this won't happen for long lived calls), so hopefully the bug hasn't affected many users in practice.