[THRIFT-2182] segfault in regression tests (GC bug in rb_thrift_memory_buffer_write)

• Type: Bug
• Status: Closed
• Priority: Major
• Resolution: Fixed
• Affects Version/s: 0.9.1, 0.9.2
• Fix Version/s: 0.9.2
• Component/s: Ruby - Library
• Labels: easyfix patch
• Patch Info: Patch Available
• Assignee: Roger Meier
• Reporter: Paul Brannan
• Created: 12/Sep/13 18:02
• Updated: 15/Jan/14 02:59
• Resolved: 12/Jan/14 22:30

Description

This bug causes the regression tests to segfault on my machine. As this is a GC bug, it may or may not be easily reproducible.

The rb_thrift_memory_buffer_write function looks like this:

    VALUE rb_thrift_memory_buffer_write(VALUE self, VALUE str) {
      VALUE buf = GET_BUF(self);
      str = force_binary_encoding(str);
      rb_str_buf_cat(buf, RSTRING_PTR(str), RSTRING_LEN(str));
      return Qnil;
    }

When gcc compiles this, it optimizes away the value of str (it is no longer used after RSTRING_PTR(str) and RSTRING_LEN(str) are computed). Later, rb_str_buf_cat invokes the GC, and the string referenced by str is collected, because there are no references to it on the stack.

Some possible solutions:

• Use StringValuePtr instead of RSTRING_PTR (in general RSTRING_PTR should be avoided in favor of StringValuePtr or StringValueCStr); I believe this will also fix #THRIFT-1047
• Use rb_str_cat instead of rb_str_buf_cat
• Use RB_GC_GUARD to prevent str from getting collected

It appears a similar bug may exist with buffer_value in rb_thrift_memory_buffer_read_into_buffer, and possibly in any of the other 30 places that RSTRING_PTR is used.

Issue Links

• incorporates: THRIFT-1047 rb_thrift_memory_buffer_write treats arg as string without check, segfaults if you pass non-string