Improve memory management in Channel #368
Conversation
* Use the gpr family of allocation functions instead of malloc/free (as these cannot return `NULL`). * Explicitly extend lifetime of `argumentWrappers` - Swift doesn't guarantee that objects will live to the end of their scope, so the optimizer is free to call Channel.Argument.Wrapper.deinit before the call to cgrpc_channel_create_secure, which would result in a use-after-free.
| @@ -105,8 +102,7 @@ cgrpc_call *cgrpc_channel_create_call(cgrpc_channel *channel, | |||
| NULL); | |||
| grpc_slice_unref(host_slice); | |||
| grpc_slice_unref(method_slice); | |||
| cgrpc_call *call = (cgrpc_call *) malloc(sizeof(cgrpc_call)); | |||
| memset(call, 0, sizeof(cgrpc_call)); | |||
| cgrpc_call *call = (cgrpc_call *) gpr_zalloc(sizeof(cgrpc_call)); | |||
There was a problem hiding this comment.
I assume gpr_zalloc is guaranteed to return zeroed memory?
There was a problem hiding this comment.
Yep, here are the docs: https://grpc.io/grpc/cpp/alloc_8h.html#ad9a7a782f4f00cad27d64d27ebbd1a72
It's not clear to me from the docs that grpc_secure_channel_create copies the creds object, but tracing through the code on master it does appear to. Since a memory leak is much better than a use-after-free I'll post that change as a follow-up so that someone more knowledgable can chime in on the review. |
Good point! It does feel like ownership semantics like this are poorly documented in the gRPC-Core API. |
these cannot return
NULL).argumentWrappers- Swift doesn'tguarantee that objects will live to the end of their scope, so
the optimizer is free to call
Channel.Argument.Wrapper.deinitbeforethe call to
cgrpc_channel_create_secure, which would result in ause-after-free.