From 1e476651b883b1f11373cd572ba10412df463184 Mon Sep 17 00:00:00 2001 From: Peter Zhu Date: Wed, 7 Jun 2023 16:58:35 -0400 Subject: [PATCH 1/2] [Ruby] Fix memory leak in grpc_rb_server_request_call The function grpc_rb_server_request_call has many places that could raise errors, including in child functions. Since a raised error will longjump out of the function, it will cause memory leaks since the function cannot perform any clean up. This commit fixes the issue by wrapping the whole function in an rb_ensure, which will ensure that a cleanup function is ran before the error is propagated upwards. --- src/ruby/ext/grpc/rb_server.c | 85 ++++++++++++++++++++++------------- 1 file changed, 53 insertions(+), 32 deletions(-) diff --git a/src/ruby/ext/grpc/rb_server.c b/src/ruby/ext/grpc/rb_server.c index f41a2c9e92c5b..1de08c095e4c7 100644 --- a/src/ruby/ext/grpc/rb_server.c +++ b/src/ruby/ext/grpc/rb_server.c @@ -191,61 +191,82 @@ static void grpc_request_call_stack_cleanup(request_call_stack* st) { grpc_call_details_destroy(&st->details); } -/* call-seq: - server.request_call +struct server_request_call_args { + grpc_rb_server* server; + grpc_completion_queue* call_queue; + request_call_stack st; +}; + +static VALUE grpc_rb_server_request_call_try(VALUE value_args) { + struct server_request_call_args *args = (struct server_request_call_args *)value_args; - Requests notification of a new call on a server. */ -static VALUE grpc_rb_server_request_call(VALUE self) { - grpc_rb_server* s = NULL; grpc_call* call = NULL; - grpc_event ev; - grpc_call_error err; - request_call_stack st; - VALUE result; - void* tag = (void*)&st; - grpc_completion_queue* call_queue = - grpc_completion_queue_create_for_pluck(NULL); - gpr_timespec deadline; + void* tag = (void*)&args->st; + + args->call_queue = grpc_completion_queue_create_for_pluck(NULL); + grpc_request_call_stack_init(&args->st); - TypedData_Get_Struct(self, grpc_rb_server, &grpc_rb_server_data_type, s); - if (s->wrapped == NULL) { - rb_raise(rb_eRuntimeError, "destroyed!"); - return Qnil; - } - grpc_request_call_stack_init(&st); /* call grpc_server_request_call, then wait for it to complete using * pluck_event */ - err = grpc_server_request_call(s->wrapped, &call, &st.details, &st.md_ary, - call_queue, s->queue, tag); + grpc_call_error err = grpc_server_request_call(args->server->wrapped, &call, &args->st.details, &args->st.md_ary, + args->call_queue, args->server->queue, tag); if (err != GRPC_CALL_OK) { - grpc_request_call_stack_cleanup(&st); rb_raise(grpc_rb_eCallError, "grpc_server_request_call failed: %s (code=%d)", grpc_call_error_detail_of(err), err); - return Qnil; } - ev = rb_completion_queue_pluck(s->queue, tag, + grpc_event ev = rb_completion_queue_pluck(args->server->queue, tag, gpr_inf_future(GPR_CLOCK_REALTIME), NULL); if (!ev.success) { - grpc_request_call_stack_cleanup(&st); rb_raise(grpc_rb_eCallError, "request_call completion failed"); - return Qnil; } /* build the NewServerRpc struct result */ - deadline = gpr_convert_clock_type(st.details.deadline, GPR_CLOCK_REALTIME); - result = rb_struct_new( - grpc_rb_sNewServerRpc, grpc_rb_slice_to_ruby_string(st.details.method), - grpc_rb_slice_to_ruby_string(st.details.host), + gpr_timespec deadline = gpr_convert_clock_type(args->st.details.deadline, GPR_CLOCK_REALTIME); + VALUE result = rb_struct_new( + grpc_rb_sNewServerRpc, grpc_rb_slice_to_ruby_string(args->st.details.method), + grpc_rb_slice_to_ruby_string(args->st.details.host), rb_funcall(rb_cTime, id_at, 2, INT2NUM(deadline.tv_sec), INT2NUM(deadline.tv_nsec / 1000)), - grpc_rb_md_ary_to_h(&st.md_ary), grpc_rb_wrap_call(call, call_queue), + grpc_rb_md_ary_to_h(&args->st.md_ary), grpc_rb_wrap_call(call, args->call_queue), NULL); - grpc_request_call_stack_cleanup(&st); + args->call_queue = NULL; return result; } +static VALUE grpc_rb_server_request_call_ensure(VALUE value_args) { + struct server_request_call_args *args = (struct server_request_call_args *)value_args; + + if (args->call_queue) { + grpc_rb_completion_queue_destroy(args->call_queue); + } + + grpc_request_call_stack_cleanup(&args->st); + + return Qnil; +} + +/* call-seq: + server.request_call + + Requests notification of a new call on a server. */ +static VALUE grpc_rb_server_request_call(VALUE self) { + grpc_rb_server* s; + + TypedData_Get_Struct(self, grpc_rb_server, &grpc_rb_server_data_type, s); + if (s->wrapped == NULL) { + rb_raise(rb_eRuntimeError, "destroyed!"); + } + + struct server_request_call_args args = { + .server = s, + .call_queue = NULL + }; + + return rb_ensure(grpc_rb_server_request_call_try, (VALUE)&args, grpc_rb_server_request_call_ensure, (VALUE)&args); +} + static VALUE grpc_rb_server_start(VALUE self) { grpc_rb_server* s = NULL; TypedData_Get_Struct(self, grpc_rb_server, &grpc_rb_server_data_type, s); From 534d2213fa3346703f79f066c8b1f3b739ff9514 Mon Sep 17 00:00:00 2001 From: peterzhu2118 Date: Wed, 7 Jun 2023 21:57:11 +0000 Subject: [PATCH 2/2] Automated change: Fix sanity tests --- src/ruby/ext/grpc/rb_server.c | 41 +++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/src/ruby/ext/grpc/rb_server.c b/src/ruby/ext/grpc/rb_server.c index 1de08c095e4c7..486a5f914d342 100644 --- a/src/ruby/ext/grpc/rb_server.c +++ b/src/ruby/ext/grpc/rb_server.c @@ -198,7 +198,8 @@ struct server_request_call_args { }; static VALUE grpc_rb_server_request_call_try(VALUE value_args) { - struct server_request_call_args *args = (struct server_request_call_args *)value_args; + struct server_request_call_args* args = + (struct server_request_call_args*)value_args; grpc_call* call = NULL; void* tag = (void*)&args->st; @@ -208,35 +209,39 @@ static VALUE grpc_rb_server_request_call_try(VALUE value_args) { /* call grpc_server_request_call, then wait for it to complete using * pluck_event */ - grpc_call_error err = grpc_server_request_call(args->server->wrapped, &call, &args->st.details, &args->st.md_ary, - args->call_queue, args->server->queue, tag); + grpc_call_error err = grpc_server_request_call( + args->server->wrapped, &call, &args->st.details, &args->st.md_ary, + args->call_queue, args->server->queue, tag); if (err != GRPC_CALL_OK) { rb_raise(grpc_rb_eCallError, "grpc_server_request_call failed: %s (code=%d)", grpc_call_error_detail_of(err), err); } - grpc_event ev = rb_completion_queue_pluck(args->server->queue, tag, - gpr_inf_future(GPR_CLOCK_REALTIME), NULL); + grpc_event ev = rb_completion_queue_pluck( + args->server->queue, tag, gpr_inf_future(GPR_CLOCK_REALTIME), NULL); if (!ev.success) { rb_raise(grpc_rb_eCallError, "request_call completion failed"); } /* build the NewServerRpc struct result */ - gpr_timespec deadline = gpr_convert_clock_type(args->st.details.deadline, GPR_CLOCK_REALTIME); - VALUE result = rb_struct_new( - grpc_rb_sNewServerRpc, grpc_rb_slice_to_ruby_string(args->st.details.method), - grpc_rb_slice_to_ruby_string(args->st.details.host), - rb_funcall(rb_cTime, id_at, 2, INT2NUM(deadline.tv_sec), - INT2NUM(deadline.tv_nsec / 1000)), - grpc_rb_md_ary_to_h(&args->st.md_ary), grpc_rb_wrap_call(call, args->call_queue), - NULL); + gpr_timespec deadline = + gpr_convert_clock_type(args->st.details.deadline, GPR_CLOCK_REALTIME); + VALUE result = + rb_struct_new(grpc_rb_sNewServerRpc, + grpc_rb_slice_to_ruby_string(args->st.details.method), + grpc_rb_slice_to_ruby_string(args->st.details.host), + rb_funcall(rb_cTime, id_at, 2, INT2NUM(deadline.tv_sec), + INT2NUM(deadline.tv_nsec / 1000)), + grpc_rb_md_ary_to_h(&args->st.md_ary), + grpc_rb_wrap_call(call, args->call_queue), NULL); args->call_queue = NULL; return result; } static VALUE grpc_rb_server_request_call_ensure(VALUE value_args) { - struct server_request_call_args *args = (struct server_request_call_args *)value_args; + struct server_request_call_args* args = + (struct server_request_call_args*)value_args; if (args->call_queue) { grpc_rb_completion_queue_destroy(args->call_queue); @@ -259,12 +264,10 @@ static VALUE grpc_rb_server_request_call(VALUE self) { rb_raise(rb_eRuntimeError, "destroyed!"); } - struct server_request_call_args args = { - .server = s, - .call_queue = NULL - }; + struct server_request_call_args args = {.server = s, .call_queue = NULL}; - return rb_ensure(grpc_rb_server_request_call_try, (VALUE)&args, grpc_rb_server_request_call_ensure, (VALUE)&args); + return rb_ensure(grpc_rb_server_request_call_try, (VALUE)&args, + grpc_rb_server_request_call_ensure, (VALUE)&args); } static VALUE grpc_rb_server_start(VALUE self) {