From 998e8e021677cf25ff70f880598da44fa00e338c Mon Sep 17 00:00:00 2001 From: Kevin Sweeney Date: Thu, 14 Feb 2019 17:23:59 +0000 Subject: [PATCH 1/3] Improve memory management in Channel * 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. --- Sources/CgRPC/shim/channel.c | 14 +++++--------- Sources/SwiftGRPC/Core/Channel.swift | 26 ++++++++++++++++---------- 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/Sources/CgRPC/shim/channel.c b/Sources/CgRPC/shim/channel.c index 2f2afb0a1..451c97d5c 100644 --- a/Sources/CgRPC/shim/channel.c +++ b/Sources/CgRPC/shim/channel.c @@ -18,15 +18,12 @@ #include #include -#include -#include #include -#include cgrpc_channel *cgrpc_channel_create(const char *address, grpc_arg *args, int num_args) { - cgrpc_channel *c = (cgrpc_channel *) malloc(sizeof (cgrpc_channel)); + cgrpc_channel *c = (cgrpc_channel *) gpr_zalloc(sizeof (cgrpc_channel)); grpc_channel_args channel_args; channel_args.args = args; @@ -43,7 +40,7 @@ cgrpc_channel *cgrpc_channel_create_secure(const char *address, const char *client_private_key, grpc_arg *args, int num_args) { - cgrpc_channel *c = (cgrpc_channel *) malloc(sizeof (cgrpc_channel)); + cgrpc_channel *c = (cgrpc_channel *) gpr_zalloc(sizeof (cgrpc_channel)); grpc_channel_args channel_args; channel_args.args = args; @@ -66,7 +63,7 @@ cgrpc_channel *cgrpc_channel_create_secure(const char *address, cgrpc_channel *cgrpc_channel_create_google(const char *address, grpc_arg *args, int num_args) { - cgrpc_channel *c = (cgrpc_channel *) malloc(sizeof (cgrpc_channel)); + cgrpc_channel *c = (cgrpc_channel *) gpr_zalloc(sizeof (cgrpc_channel)); grpc_channel_args channel_args; channel_args.args = args; @@ -83,7 +80,7 @@ cgrpc_channel *cgrpc_channel_create_google(const char *address, void cgrpc_channel_destroy(cgrpc_channel *c) { grpc_channel_destroy(c->channel); c->channel = NULL; - free(c); + gpr_free(c); } cgrpc_call *cgrpc_channel_create_call(cgrpc_channel *channel, @@ -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)); call->call = channel_call; return call; } diff --git a/Sources/SwiftGRPC/Core/Channel.swift b/Sources/SwiftGRPC/Core/Channel.swift index 58a845086..e9e11a980 100644 --- a/Sources/SwiftGRPC/Core/Channel.swift +++ b/Sources/SwiftGRPC/Core/Channel.swift @@ -45,12 +45,14 @@ public class Channel { gRPC.initialize() host = address let argumentWrappers = arguments.map { $0.toCArg() } - var argumentValues = argumentWrappers.map { $0.wrapped } - if secure { - underlyingChannel = cgrpc_channel_create_secure(address, roots_pem(), nil, nil, &argumentValues, Int32(arguments.count)) - } else { - underlyingChannel = cgrpc_channel_create(address, &argumentValues, Int32(arguments.count)) + underlyingChannel = withExtendedLifetime(argumentWrappers) { + var argumentValues = argumentWrappers.map { $0.wrapped } + if secure { + return cgrpc_channel_create_secure(address, roots_pem(), nil, nil, &argumentValues, Int32(arguments.count)) + } else { + return cgrpc_channel_create(address, &argumentValues, Int32(arguments.count)) + } } completionQueue = CompletionQueue(underlyingCompletionQueue: cgrpc_channel_completion_queue(underlyingChannel), name: "Client") completionQueue.run() // start a loop that watches the channel's completion queue @@ -64,9 +66,11 @@ public class Channel { gRPC.initialize() host = googleAddress let argumentWrappers = arguments.map { $0.toCArg() } - var argumentValues = argumentWrappers.map { $0.wrapped } - - underlyingChannel = cgrpc_channel_create_google(googleAddress, &argumentValues, Int32(arguments.count)) + + underlyingChannel = withExtendedLifetime(argumentWrappers) { + var argumentValues = argumentWrappers.map { $0.wrapped } + return cgrpc_channel_create_google(googleAddress, &argumentValues, Int32(arguments.count)) + } completionQueue = CompletionQueue(underlyingCompletionQueue: cgrpc_channel_completion_queue(underlyingChannel), name: "Client") completionQueue.run() // start a loop that watches the channel's completion queue @@ -83,9 +87,11 @@ public class Channel { gRPC.initialize() host = address let argumentWrappers = arguments.map { $0.toCArg() } - var argumentValues = argumentWrappers.map { $0.wrapped } - underlyingChannel = cgrpc_channel_create_secure(address, certificates, clientCertificates, clientKey, &argumentValues, Int32(arguments.count)) + underlyingChannel = withExtendedLifetime(argumentWrappers) { + var argumentValues = argumentWrappers.map { $0.wrapped } + return cgrpc_channel_create_secure(address, certificates, clientCertificates, clientKey, &argumentValues, Int32(arguments.count)) + } completionQueue = CompletionQueue(underlyingCompletionQueue: cgrpc_channel_completion_queue(underlyingChannel), name: "Client") completionQueue.run() // start a loop that watches the channel's completion queue } From 6565abea43abc33973c52516e9a453c2a9ed1008 Mon Sep 17 00:00:00 2001 From: Kevin Sweeney Date: Thu, 14 Feb 2019 17:42:32 +0000 Subject: [PATCH 2/3] Also use gpr_free to free cgrpc_calls --- Sources/CgRPC/shim/call.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/CgRPC/shim/call.c b/Sources/CgRPC/shim/call.c index 5c88c6af0..d76ec6156 100644 --- a/Sources/CgRPC/shim/call.c +++ b/Sources/CgRPC/shim/call.c @@ -24,7 +24,7 @@ void cgrpc_call_destroy(cgrpc_call *call) { if (call->call) { grpc_call_unref(call->call); } - free(call); + gpr_free(call); } grpc_call_error cgrpc_call_perform(cgrpc_call *call, cgrpc_operations *operations, void *tag) { From b4fcf4c19e19e817eb61c3ca3c334433b7975ead Mon Sep 17 00:00:00 2001 From: Kevin Sweeney Date: Thu, 14 Feb 2019 17:44:14 +0000 Subject: [PATCH 3/3] Fix imports in call.c --- Sources/CgRPC/shim/call.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Sources/CgRPC/shim/call.c b/Sources/CgRPC/shim/call.c index d76ec6156..0436ca069 100644 --- a/Sources/CgRPC/shim/call.c +++ b/Sources/CgRPC/shim/call.c @@ -16,9 +16,9 @@ #include "internal.h" #include "cgrpc.h" +#include + #include -#include -#include void cgrpc_call_destroy(cgrpc_call *call) { if (call->call) {