Per #321 use RB_GC_GUARD around rb_hash_dup #346

Merged
merged 1 commit into from Jul 10, 2013

Conversation

Projects
None yet
4 participants
Collaborator

sodabrew commented Dec 30, 2012

Waiting to hear back from @justinweiss and @jeremy to see if this resolves #321.

Collaborator

sodabrew commented Dec 31, 2012

Verified by inspection on my x86_64 macbook:

$ file lib/mysql2/mysql2.bundle 
lib/mysql2/mysql2.bundle: Mach-O 64-bit bundle x86_64
$ otool -tvVX ./lib/mysql2/mysql2.bundle
...

Before:

  callq 0x000052ce  ; symbol stub for: _rb_mysql_result_to_obj
  movq  %rax,%r15
  leaq  0x0000332d(%rip),%rsi
  movq  %rbx,%rdi
  callq 0x00005226  ; symbol stub for: _rb_iv_get
  movq  %rax,%rdi
  callq 0x00005208  ; symbol stub for: _rb_hash_dup
  leaq  0x0000332d(%rip),%rsi
  movq  %r15,%rdi
  movq  %rax,%rdx
  callq 0x0000522c  ; symbol stub for: _rb_iv_set

After:

  callq 0x000052ce  ; symbol stub for: _rb_mysql_result_to_obj
  movq  %rax,%r15
+ movq  0xe0(%rbp),%rax
  leaq  0x00003335(%rip),%rsi
  movq  %rbx,%rdi
  callq 0x00005226  ; symbol stub for: _rb_iv_get
  movq  %rax,%rdi
  callq 0x00005208  ; symbol stub for: _rb_hash_dup
+ movq  %rax,0xe0(%rbp)
  leaq  0x00003331(%rip),%rsi
  movq  %r15,%rdi
  movq  %rax,%rdx
  callq 0x0000522c  ; symbol stub for: _rb_iv_set

scottjg commented Jan 11, 2013

I'm not sure this fix is correct. Even if GC did run at the invocation of rb_iv_set, GC should consider that you are holding the hash in rax and not collect it. Even if rax was clobbered in rb_iv_set, the value would have to be saved somewhere (either on the stack or in a register) inside rb_iv_set long enough to actually save the hash into the ivar, and gc_mark it. So I'm not sure I really see a window where it could be GC'ed.

You should be able to just put an rb_gc(); in between the calls (without marking them with RB_GC_GUARD). If it was related to GC, you would see the bug reproducing 100%.

I would think it's more likely that something is just setting @query_options to nil inside rubyland before calling fields, though i haven't spotted the bug yet...

Contributor

jeremy commented Jan 11, 2013

Yeah, assuming it's a plain old bug. The segfaults are only "fixed" insofar
as that they're normal TypeError exceptions now.

The other naive GC guarding in the commit I linked are attempts to trace
down a separate issue. Results are garbage collected somewhere between the
querying and iterating the result. Since object ids and heap slots are
reused, this manifests in very strange ways: objects that work with the
results mysteriously have some other object instead! Or we see
NotImplementedError on T_NONE or terminated object (heap slot not
reused yet):

  method `method_missing' called on terminated object (0x00000006113e78
flags=0x0 klass=0x0)
  /u/apps/bcx/shared/bundle/ruby/1.9.1/bundler/gems/rails-
48810a52dfba/activerecord/lib/active_record/relation/finder_methods.rb:271:in
`find_by_attributes'

On Fri, Jan 11, 2013 at 3:00 AM, Scott J. Goldman
notifications@github.comwrote:

I'm not sure this fix is correct. Even if GC did run at the invocation of
rb_iv_set, GC should consider that you are holding the hash in rax and not
collect it. Even if rax was clobbered in rb_iv_set, the value would have
to be saved somewhere (either on the stack or in a register) inside
rb_iv_set long enough to actually save the hash into the ivar, and
gc_mark it. So I'm not sure I really see a window where it could be GC'ed.

You should be able to just put an rb_gc(); in between the calls (without
marking them with RB_GC_GUARD). If it was related to GC, you would see
the bug reproducing 100%.

I would think it's more likely that something is just setting
@query_options to nil inside rubyland before calling fields, though i
haven't spotted the bug yet...


Reply to this email directly or view it on GitHubhttps://github.com/brianmario/mysql2/pull/346#issuecomment-12138032.

Collaborator

sodabrew commented May 7, 2013

@brianmario I started running tests under Valgrind, and we are for sure accessing previously-freed memory in a few places. Two suspect commits that I tried reverting were your recent 14c6bed and my 3aa46fc. Are you around at GH offices this Friday? I could come by for a mini-hackathon to try and get to the bottom of this.

This branch holds up to tests better https://github.com/sodabrew/mysql2/tree/revert_14c6bed but not perfectly.

Owner

brianmario commented May 7, 2013

I'm actually in Florence about to board a night train to Paris for LaConf. Won't be back in the states until the 25th but I'd be happy to try and Skype sometime between now and then

On May 7, 2013, at 19:24, Aaron Stone notifications@github.com wrote:

@brianmario I started running tests under Valgrind, and we are for sure accessing previously-freed memory in a few places. Two suspect commits that I tried reverting were your recent 14c6bed and my 3aa46fc. Are you around at GH offices this Friday? I could come by for a mini-hackathon to try and get to the bottom of this.

This branch holds up to tests better https://github.com/sodabrew/mysql2/tree/revert_14c6bed but not perfectly.


Reply to this email directly or view it on GitHub.

Collaborator

sodabrew commented May 8, 2013

Nice! Have fun! I'll ping you directly to follow up.

Collaborator

sodabrew commented Jul 10, 2013

Rebased and refactored a few times. I think this is the final edition I'd like to merge. Please review!

Owner

brianmario commented Jul 10, 2013

Sweet, this should at the very least help protect against a segfault by passing the potentially nil value through Check_Type. Let's run with this.

sodabrew added a commit that referenced this pull request Jul 10, 2013

Merge pull request #346 from sodabrew/gc_guard_hash_dup
Per #321 use RB_GC_GUARD around rb_hash_dup

@sodabrew sodabrew merged commit a2fc987 into brianmario:master Jul 10, 2013

1 check passed

default The Travis CI build passed
Details

@sodabrew sodabrew deleted the sodabrew:gc_guard_hash_dup branch Jul 10, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment