CodingCanuck
commented
2 years ago @benh in case you want to poke at this: it looks like the use-after-free we were discussing might be coming from eventuals-grpc (or even eventuals).
Closed CodingCanuck closed 2 years ago
CodingCanuck
commented
2 years ago @benh in case you want to poke at this: it looks like the use-after-free we were discussing might be coming from eventuals-grpc (or even eventuals).
benh
commented
2 years ago Awesome stuff @CodingCanuck!
I have a hypothesis here that ::grpc::ClientContext can not be deleted before ::grpc::ClientAsyncReaderWriter (although I can't confirm via documentation and the API is only suggestive of this requirement but not definitive).
Unfortunately trying the temporary hack of allocating ::grpc::ClientContext on the heap and never deleting it does not fix the branch that @while-false and I were working on, so while this definitely needs to be fixed I'm not sure it's our smoking gun yet!
I'll dig deeper into @while-false's branch tomorrow as well as propose a fix for this issue.
So glad to have asan turned on!
CodingCanuck
commented
2 years ago Relevant: https://github.com/grpc/grpc/issues/16877 and https://github.com/grpc/grpc/pull/17386/files
So you're correct, clientContext cannot be deleted before ClientAsyncReaderWriter.
CodingCanuck
commented
2 years ago Given this, it sounds like the problem / fix should be in this scope: https://github.com/3rdparty/eventuals-grpc/blob/0f66b899e72ce34f6242b7b432a5ea1d9ab61b85/eventuals/grpc/client.h#L370-L383
Ben, is there any documentation on how eventuals' destruction order works? I'm guessing that for this:
template <typename Request, typename Response>
return Context()
| Then([this,
name = std::move(name),
host = std::move(host)](
::grpc::ClientContext* context) mutable {
return Call<Request, Response>(
std::move(name),
context,
std::move(host));
});The issue is that the memory in which the result of Context() is stored is destroyed first, and after that the memory in which the result of Call() is allocated is destroyed second. It seems we should invert that, in a similar fashion to how constructors are called least-to-most derived yet destructors are called in the reversed most-to-least derived order.
Edit: I think my understanding is insufficient / wrong here. operator| already seems to create a struct with two fields where the left-hand thing is stored before the right-hand thing (and thus the right-hand thing will be destructed first), so I don't understand how the Call is being destroyed before the Context is. (I'm guessing my misunderstanding in how Then() works, where maybe the Call() there has a lifetime separate to the Context() and Then() lifetimes)
EventualsGrpcTest.CancelledByClient exhibits a use-after-free on this line: https://github.com/3rdparty/eventuals-grpc/blob/b40df00734f5ecdb3f680d255b7226dcf3648701/test/cancelled-by-client.cc#L71
Which evaluates the eventual constructed here: https://github.com/3rdparty/eventuals-grpc/blob/b40df00734f5ecdb3f680d255b7226dcf3648701/test/cancelled-by-client.cc#L63-L69
This is easily reproducible with asan (which is added in https://github.com/3rdparty/eventuals-grpc/pull/70 : that's being auto-merged now so it should land soon):
bazel test --config=asan //test:grpcSample failure log showing a use-after-free: