Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ruby: register grpc_rb_sStatus as a global variable #36125

Closed
wants to merge 1 commit into from

Conversation

casperisfine
Copy link
Contributor

Ref: https://bugs.ruby-lang.org/issues/20311

C global variable that contain references to Ruby object MUST be declared to the Ruby GC to prevent these objects from being collected or moved.

There are a few exceptions to that, such as classes defined using the C APIs such as rb_define_class etc.

Up to Ruby 3.4 however, there was a bug that caused classes created from Ruby with the Struct.new("Name") API to also be marked as immortal and immovable.

GRPC has been relying on this bug, which I fixed in Ruby 3.4, and now GRPC is crashing when Struct::Status is moved around by the GC.

-- C level backtrace information -------------------------------------------
ruby(rb_print_backtrace+0x14) [0x5577db219d41] ruby-3.4.0/vm_dump.c:820
ruby(rb_vm_bugreport) ruby-3.4.0/vm_dump.c:1151
ruby(rb_bug_for_fatal_signal+0xfc) [0x5577db3cc60c] ruby-3.4.0/error.c:1066
ruby(sigsegv+0x4d) [0x5577db16358d] ruby-3.4.0/signal.c:926
libc.so.6(0x7f5bacd32520) [0x7f5bacd32520]
ruby(rb_class_superclass+0x32) [0x5577db0a9152] ruby-3.4.0/object.c:2239
ruby(struct_ivar_get+0x2a) [0x5577db194e02] ruby-3.4.0/struct.c:49
ruby(struct_ivar_get) ruby-3.4.0/struct.c:40
ruby(num_members) ruby-3.4.0/struct.c:705
ruby(rb_struct_new+0x56) [0x5577db19a9d6] ruby-3.4.0/struct.c:848
lib/grpc/grpc_c.so(grpc_run_batch_stack_build_result+0xe6) [0x7f5b84bb6b96] /tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/ext/grpc/rb_call.c:780
lib/grpc/grpc_c.so(grpc_rb_call_run_batch_try) /tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/ext/grpc/rb_call.c:839
ruby(rb_ensure+0x10c) [0x5577db007d3c] ruby-3.4.0/eval.c:1000
/tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/lib/grpc/grpc_c.so(grpc_rb_call_run_batch+0xca) [0x7f5b84bb595a] /tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/ext/grpc/rb_call.c:893
ruby(vm_call_cfunc_with_frame_+0x117) [0x5577db1f4c77] ruby-3.4.0/vm_insnhelper.c:3524

cc @apolcyn @peterzhu2118 @k0kubun

Ref: https://bugs.ruby-lang.org/issues/20311

C global variable that contain references to Ruby object MUST
be declared to the Ruby GC to prevent these objects from being
collected or moved.

There are a few exceptions to that, such as classes defined using
the C APIs such as `rb_define_class` etc.

Up to Ruby 3.4 however, there was a bug that caused classes created
from Ruby with the `Struct.new("Name")` API to also be marked as
immortal and immovable.

GRPC has been relying on this bug, which I fixed in Ruby 3.4, and
now GRPC is crashing when `Struct::Status` is moved around by the GC.
@casperisfine
Copy link
Contributor Author

NB: I grepped to see if some others Strucs may be impacted, but didn't find any other in this case.

@apolcyn apolcyn added release notes: yes Indicates if PR needs to be in release notes kokoro:force-run labels Mar 14, 2024
Copy link
Contributor

@apolcyn apolcyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix! one question

@@ -467,6 +467,7 @@ void Init_grpc_c() {
grpc_rb_mGrpcCore = rb_define_module_under(grpc_rb_mGRPC, "Core");
grpc_rb_sNewServerRpc = rb_struct_define(
"NewServerRpc", "method", "host", "deadline", "metadata", "call", NULL);
rb_global_variable(&grpc_rb_sStatus);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does grpc_rb_sNewServerRpc also need this?

Also wondering about grpc_rb_sBatchResult in rb_call.c.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No because rb_struct_define, like rb_define_class all basically all the C API functions that end up creating a class or module, do "root" that object, which means the GC will never collect nor move it.

grpc_rb_sStatus need it only because that class is defined in Ruby, and grabbed in C via rb_const_get.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative could be to define that struct in C like the other (and Struct.new("Status") should probably be avoided altogether as it might clash with other gems), but I went for the fix the the minimal amount of changes.

@casperisfine
Copy link
Contributor Author

which I fixed in Ruby 3.4,

Another quick note. It's currently fixed in 3.4-dev, but the bug is marked as needing backport, so eventually it will make it into point release of 3.3, 3.2 and 3.1.

eugeneo pushed a commit to eugeneo/grpc that referenced this pull request Mar 18, 2024
Ref: https://bugs.ruby-lang.org/issues/20311

C global variable that contain references to Ruby object MUST be declared to the Ruby GC to prevent these objects from being collected or moved.

There are a few exceptions to that, such as classes defined using the C APIs such as `rb_define_class` etc.

Up to Ruby 3.4 however, there was a bug that caused classes created from Ruby with the `Struct.new("Name")` API to also be marked as immortal and immovable.

GRPC has been relying on this bug, which I fixed in Ruby 3.4, and now GRPC is crashing when `Struct::Status` is moved around by the GC.

```
-- C level backtrace information -------------------------------------------
ruby(rb_print_backtrace+0x14) [0x5577db219d41] ruby-3.4.0/vm_dump.c:820
ruby(rb_vm_bugreport) ruby-3.4.0/vm_dump.c:1151
ruby(rb_bug_for_fatal_signal+0xfc) [0x5577db3cc60c] ruby-3.4.0/error.c:1066
ruby(sigsegv+0x4d) [0x5577db16358d] ruby-3.4.0/signal.c:926
libc.so.6(0x7f5bacd32520) [0x7f5bacd32520]
ruby(rb_class_superclass+0x32) [0x5577db0a9152] ruby-3.4.0/object.c:2239
ruby(struct_ivar_get+0x2a) [0x5577db194e02] ruby-3.4.0/struct.c:49
ruby(struct_ivar_get) ruby-3.4.0/struct.c:40
ruby(num_members) ruby-3.4.0/struct.c:705
ruby(rb_struct_new+0x56) [0x5577db19a9d6] ruby-3.4.0/struct.c:848
lib/grpc/grpc_c.so(grpc_run_batch_stack_build_result+0xe6) [0x7f5b84bb6b96] /tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/ext/grpc/rb_call.c:780
lib/grpc/grpc_c.so(grpc_rb_call_run_batch_try) /tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/ext/grpc/rb_call.c:839
ruby(rb_ensure+0x10c) [0x5577db007d3c] ruby-3.4.0/eval.c:1000
/tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/lib/grpc/grpc_c.so(grpc_rb_call_run_batch+0xca) [0x7f5b84bb595a] /tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/ext/grpc/rb_call.c:893
ruby(vm_call_cfunc_with_frame_+0x117) [0x5577db1f4c77] ruby-3.4.0/vm_insnhelper.c:3524
```

cc @apolcyn @peterzhu2118 @k0kubun

Closes grpc#36125

COPYBARA_INTEGRATE_REVIEW=grpc#36125 from Shopify:fix-ruby-3.4-compat 7a12759
PiperOrigin-RevId: 616152904
@ngan
Copy link

ngan commented Apr 27, 2024

Just want to add that the fix was also backported to (and released in) Ruby 3.3.1.

apolcyn pushed a commit to apolcyn/grpc that referenced this pull request May 2, 2024
Ref: https://bugs.ruby-lang.org/issues/20311

C global variable that contain references to Ruby object MUST be declared to the Ruby GC to prevent these objects from being collected or moved.

There are a few exceptions to that, such as classes defined using the C APIs such as `rb_define_class` etc.

Up to Ruby 3.4 however, there was a bug that caused classes created from Ruby with the `Struct.new("Name")` API to also be marked as immortal and immovable.

GRPC has been relying on this bug, which I fixed in Ruby 3.4, and now GRPC is crashing when `Struct::Status` is moved around by the GC.

```
-- C level backtrace information -------------------------------------------
ruby(rb_print_backtrace+0x14) [0x5577db219d41] ruby-3.4.0/vm_dump.c:820
ruby(rb_vm_bugreport) ruby-3.4.0/vm_dump.c:1151
ruby(rb_bug_for_fatal_signal+0xfc) [0x5577db3cc60c] ruby-3.4.0/error.c:1066
ruby(sigsegv+0x4d) [0x5577db16358d] ruby-3.4.0/signal.c:926
libc.so.6(0x7f5bacd32520) [0x7f5bacd32520]
ruby(rb_class_superclass+0x32) [0x5577db0a9152] ruby-3.4.0/object.c:2239
ruby(struct_ivar_get+0x2a) [0x5577db194e02] ruby-3.4.0/struct.c:49
ruby(struct_ivar_get) ruby-3.4.0/struct.c:40
ruby(num_members) ruby-3.4.0/struct.c:705
ruby(rb_struct_new+0x56) [0x5577db19a9d6] ruby-3.4.0/struct.c:848
lib/grpc/grpc_c.so(grpc_run_batch_stack_build_result+0xe6) [0x7f5b84bb6b96] /tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/ext/grpc/rb_call.c:780
lib/grpc/grpc_c.so(grpc_rb_call_run_batch_try) /tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/ext/grpc/rb_call.c:839
ruby(rb_ensure+0x10c) [0x5577db007d3c] ruby-3.4.0/eval.c:1000
/tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/lib/grpc/grpc_c.so(grpc_rb_call_run_batch+0xca) [0x7f5b84bb595a] /tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/ext/grpc/rb_call.c:893
ruby(vm_call_cfunc_with_frame_+0x117) [0x5577db1f4c77] ruby-3.4.0/vm_insnhelper.c:3524
```

cc @apolcyn @peterzhu2118 @k0kubun

Closes grpc#36125

COPYBARA_INTEGRATE_REVIEW=grpc#36125 from Shopify:fix-ruby-3.4-compat 7a12759
PiperOrigin-RevId: 616152904
apolcyn pushed a commit to apolcyn/grpc that referenced this pull request May 21, 2024
Ref: https://bugs.ruby-lang.org/issues/20311

C global variable that contain references to Ruby object MUST be declared to the Ruby GC to prevent these objects from being collected or moved.

There are a few exceptions to that, such as classes defined using the C APIs such as `rb_define_class` etc.

Up to Ruby 3.4 however, there was a bug that caused classes created from Ruby with the `Struct.new("Name")` API to also be marked as immortal and immovable.

GRPC has been relying on this bug, which I fixed in Ruby 3.4, and now GRPC is crashing when `Struct::Status` is moved around by the GC.

```
-- C level backtrace information -------------------------------------------
ruby(rb_print_backtrace+0x14) [0x5577db219d41] ruby-3.4.0/vm_dump.c:820
ruby(rb_vm_bugreport) ruby-3.4.0/vm_dump.c:1151
ruby(rb_bug_for_fatal_signal+0xfc) [0x5577db3cc60c] ruby-3.4.0/error.c:1066
ruby(sigsegv+0x4d) [0x5577db16358d] ruby-3.4.0/signal.c:926
libc.so.6(0x7f5bacd32520) [0x7f5bacd32520]
ruby(rb_class_superclass+0x32) [0x5577db0a9152] ruby-3.4.0/object.c:2239
ruby(struct_ivar_get+0x2a) [0x5577db194e02] ruby-3.4.0/struct.c:49
ruby(struct_ivar_get) ruby-3.4.0/struct.c:40
ruby(num_members) ruby-3.4.0/struct.c:705
ruby(rb_struct_new+0x56) [0x5577db19a9d6] ruby-3.4.0/struct.c:848
lib/grpc/grpc_c.so(grpc_run_batch_stack_build_result+0xe6) [0x7f5b84bb6b96] /tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/ext/grpc/rb_call.c:780
lib/grpc/grpc_c.so(grpc_rb_call_run_batch_try) /tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/ext/grpc/rb_call.c:839
ruby(rb_ensure+0x10c) [0x5577db007d3c] ruby-3.4.0/eval.c:1000
/tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/lib/grpc/grpc_c.so(grpc_rb_call_run_batch+0xca) [0x7f5b84bb595a] /tmp/bundle/ruby/3.4.0+0/bundler/gems/grpc-5ed33ee673e3/src/ruby/ext/grpc/rb_call.c:893
ruby(vm_call_cfunc_with_frame_+0x117) [0x5577db1f4c77] ruby-3.4.0/vm_insnhelper.c:3524
```

cc @apolcyn @peterzhu2118

Closes grpc#36125

COPYBARA_INTEGRATE_REVIEW=grpc#36125 from Shopify:fix-ruby-3.4-compat 7a12759
PiperOrigin-RevId: 616152904
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/ruby release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants