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

Use write barrier when setting busy_handler #556

Merged
merged 2 commits into from
Sep 12, 2024

Conversation

jhawthorn
Copy link
Member

The Database type struct is marked as WB_PROTECTED which means that any new references added to the object need to fire the write barrier. Previously we were missing this when setting the busy_handler.

I wasn't able to make a smaller reproduction, but I started investigating this after seeing this crash in the Rails CI suite: https://buildkite.com/rails/rails/builds/111147#0191d7ec-de81-49c8-b20a-d207f446d45e

<OBJ_INFO:gc_mark_ptr@gc.c:7072> 0x00007f4fd042cb20 [0 M    ] T_NONE
test/cases/connection_pool_test.rb:1041: [BUG] try to mark T_NONE object
ruby 3.3.5 (2024-09-03 revision ef084cc8f4) [x86_64-linux]
-- Control frame information -----------------------------------------------
c:0060 p:---- s:0311 e:000310 CFUNC  :initialize
c:0059 p:---- s:0308 e:000307 CFUNC  :new
c:0058 p:0012 s:0304 e:000303 METHOD test/cases/connection_pool_test.rb:1041
-----8<-----
-- C level backtrace information -------------------------------------------
/usr/local/lib/libruby.so.3.3(rb_print_backtrace+0x14) [0x7f4fecfcfaeb] /usr/src/ruby/vm_dump.c:820
/usr/local/lib/libruby.so.3.3(rb_vm_bugreport) /usr/src/ruby/vm_dump.c:1151
/usr/local/lib/libruby.so.3.3(bug_report_end+0x0) [0x7f4fecdc8a7a] /usr/src/ruby/error.c:1042
/usr/local/lib/libruby.so.3.3(rb_bug_without_die) /usr/src/ruby/error.c:1042
/usr/local/lib/libruby.so.3.3(die+0x0) [0x7f4fecd064ab] /usr/src/ruby/error.c:1050
/usr/local/lib/libruby.so.3.3(rb_bug) /usr/src/ruby/error.c:1052
/usr/local/lib/libruby.so.3.3(gc_mark_ptr+0x1ad) [0x7f4fecdeff4d] /usr/src/ruby/gc.c:7073
/usr/local/bundle/gems/sqlite3-2.0.4-x86_64-linux-gnu/lib/sqlite3/3.3/sqlite3_native.so(0x7f4fd01d55d6) [0x7f4fd01d55d6]
/usr/local/lib/libruby.so.3.3(gc_mark_stacked_objects+0x78) [0x7f4fecdf3408] /usr/src/ruby/gc.c:7565
/usr/local/lib/libruby.so.3.3(gc_mark_stacked_objects_all) /usr/src/ruby/gc.c:7603
/usr/local/lib/libruby.so.3.3(gc_marks_rest) /usr/src/ruby/gc.c:8798
-----8<-----

Given that afaict this is the only mark function within sqlite3 I think it's pretty likely this was the cause.

The Database type struct is marked as WB_PROTECTED which means that any
new references added to the object need to fire the write barrier.
Previously we were missing this when setting the busy_handler.
We no longer use this instance variable. This is just cleanup, I dont
believe it caused any issues.
@flavorjones
Copy link
Member

@jhawthorn Thanks for this!

@flavorjones flavorjones merged commit 480f3e7 into sparklemotion:main Sep 12, 2024
130 checks passed
@flavorjones
Copy link
Member

I'll cut a release in the next day or so.

@flavorjones
Copy link
Member

Updated the changelog in 6c274b4, feel free to suggest alternate wording.

@jhawthorn
Copy link
Member Author

That wording sounds fine, although we could be less specific about how the crash will surface (I don't think it will always be in the mark phase, we could also be super unlucky and end up with a different object in place of the busy handler).

When setting a Database busy_handler, fire the write barrier to prevent potential crashes on Garbage Collection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants