Skip to content

Commit

Permalink
[ruby] backport "Fix use-after-free for post-fork channel recreation (#…
Browse files Browse the repository at this point in the history
…35488)" (#35613)

The `grpc_channel_args` is retained on the Ruby object and used for
recreating the channel after forking in
grpc_rb_channel_maybe_recreate_channel_after_fork(). Previously, the key
for each argument was taken from a Ruby string directly, which could be
invalidated if the Ruby string is modified or moved by the GC. Duplicate
the string for the key instead, so we own it.

Reproducer in #35489

<!--

If you know who should review your pull request, please assign it to
that person, otherwise the pull request would get assigned randomly.

If your pull request is for a specific language, please add the
appropriate lang label.

-->

Closes #35488

COPYBARA_INTEGRATE_REVIEW=#35488 from
Shopify:key-uaf c1813ce
PiperOrigin-RevId: 599304551




<!--

If you know who should review your pull request, please assign it to
that
person, otherwise the pull request would get assigned randomly.

If your pull request is for a specific language, please add the
appropriate
lang label.

-->

Co-authored-by: Alan Wu <XrXr@users.noreply.github.com>
  • Loading branch information
apolcyn and XrXr committed Jan 19, 2024
1 parent 8e92b0a commit 3ce4476
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 1 deletion.
64 changes: 64 additions & 0 deletions src/ruby/end2end/fork_test_repro_35489.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
#!/usr/bin/env ruby
#
# Copyright 2016 gRPC authors.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

ENV['GRPC_ENABLE_FORK_SUPPORT'] = "1"
fail "forking only supported on linux" unless RUBY_PLATFORM =~ /linux/

this_dir = File.expand_path(File.dirname(__FILE__))
protos_lib_dir = File.join(this_dir, 'lib')
grpc_lib_dir = File.join(File.dirname(this_dir), 'lib')
$LOAD_PATH.unshift(grpc_lib_dir) unless $LOAD_PATH.include?(grpc_lib_dir)
$LOAD_PATH.unshift(protos_lib_dir) unless $LOAD_PATH.include?(protos_lib_dir)
$LOAD_PATH.unshift(this_dir) unless $LOAD_PATH.include?(this_dir)

require 'grpc'
require 'end2end_common'

def do_rpc(stub)
stub.echo(Echo::EchoRequest.new(request: 'hello'), deadline: Time.now + 300)
end

def main
this_dir = File.expand_path(File.dirname(__FILE__))
echo_server_path = File.join(this_dir, 'echo_server.rb')
to_child_r, _to_child_w = IO.pipe
to_parent_r, to_parent_w = IO.pipe
# Note gRPC has not yet been initialized, otherwise we would need to call prefork
# before spawn and postfork_parent after.
# TODO(apolcyn): consider redirecting server's stderr to a file
Process.spawn(RbConfig.ruby, echo_server_path, in: to_child_r, out: to_parent_w, err: "server_log")
to_child_r.close
to_parent_w.close
child_port = to_parent_r.gets.strip
STDERR.puts "server running on port: #{child_port}"
key = "grpc." * 100_000 # large enough to malloc backing storage
value = "testvalue" * 100_000
stub = Echo::EchoServer::Stub.new("localhost:#{child_port}", :this_channel_is_insecure, channel_args: { key => value })
with_logging("parent: GRPC.prefork") { GRPC.prefork }
pid = fork do
with_logging("child: GRPC.postfork_child") { GRPC.postfork_child }
key.clear
value.clear
GC.compact
with_logging("child: post-fork RPC") { do_rpc(stub) }
STDERR.puts "child: done"
end
with_logging("parent: GRPC.postfork_parent") { GRPC.postfork_parent }
Process.wait pid
STDERR.puts "parent: done"
end

main
4 changes: 3 additions & 1 deletion src/ruby/ext/grpc/rb_channel_args.c
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ static int grpc_rb_channel_create_in_process_add_args_hash_cb(VALUE key,
return ST_STOP;
}

args->args[args->num_args - 1].key = (char*)the_key;
args->args[args->num_args - 1].key = gpr_strdup(the_key);
switch (TYPE(val)) {
case T_SYMBOL:
args->args[args->num_args - 1].type = GRPC_ARG_STRING;
Expand Down Expand Up @@ -163,6 +163,8 @@ void grpc_rb_channel_args_destroy(grpc_channel_args* args) {
GPR_ASSERT(args != NULL);
if (args->args == NULL) return;
for (int i = 0; i < args->num_args; i++) {
// the key was created with gpr_strdup
gpr_free(args->args[i].key);
if (args->args[i].type == GRPC_ARG_STRING) {
// we own string pointers, which were created with gpr_strdup
gpr_free(args->args[i].value.string);
Expand Down
1 change: 1 addition & 0 deletions tools/run_tests/run_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -1026,6 +1026,7 @@ def test_specs(self):
"src/ruby/end2end/bad_usage_fork_test.rb",
"src/ruby/end2end/prefork_without_using_grpc_test.rb",
"src/ruby/end2end/prefork_postfork_loop_test.rb",
"src/ruby/end2end/fork_test_repro_35489.rb",
]:
# Skip fork tests in general until https://github.com/grpc/grpc/issues/34442
# is fixed. Otherwise we see too many flakes.
Expand Down

0 comments on commit 3ce4476

Please sign in to comment.