Sitelet https://github.com/sparklemotion/sqlite3-ruby/pull/556
Skip to content

Use write barrier when setting busy_handler - #556

Merged
flavorjones merged 2 commits into
sparklemotion:mainfrom
jhawthorn:fix_write_barrier
Sep 12, 2024
Merged

Use write barrier when setting busy_handler#556
flavorjones merged 2 commits into
sparklemotion:mainfrom
jhawthorn:fix_write_barrier

Conversation

@jhawthorn

@jhawthorn jhawthorn commented Sep 9, 2024

Copy link
Copy Markdown
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.

@jhawthorn
Use write barrier when setting busy_handler
d9d5b32 
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.
@jhawthorn
Remove @busy_handler = nil
f759e82 
We no longer use this instance variable. This is just cleanup, I dont
believe it caused any issues.
@flavorjones

flavorjones commented Sep 12, 2024

Copy link
Copy Markdown
Member

@jhawthorn Thanks for this!

@flavorjones
flavorjones merged commit 480f3e7 into sparklemotion:main Sep 12, 2024
@flavorjones

flavorjones commented Sep 13, 2024

Copy link
Copy Markdown
Member

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

@flavorjones flavorjones mentioned this pull request Sep 15, 2024
Closed
4 tasks done
@flavorjones

flavorjones commented Sep 15, 2024

Copy link
Copy Markdown
Member

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

@jhawthorn

jhawthorn commented Sep 16, 2024

Copy link
Copy Markdown
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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jhawthorn @flavorjones