Backport #11328: Memory leak in str_buf_cat?

• Status: Closed
• Assignee: -

[skippy (adam greene)]

There may be a memory leak in str_buf_cat that showed up in 2.2 but was 'fixed' in 2.3. The main reason I'm filing this issue is it looks like the 'fix' in 2.3 is actually a byproduct of another bug. So I wanted to file a ticket to bring this up and see about a test case to prevent a regression in the future.

Details:

• the underlying issue was discovered running google/protobuf gem within ruby 2.2: https://github.com/google/protobuf/issues/474
• the issue seems to crop up between v2_2_0_preview1 and v2_2_0_preview2, but is gone in 2.3.0-dev
• this looks like the patch that resolved this behavior, but again, I think it was trying to fix a different issue: https://github.com/ruby/ruby/commit/2b0a6f47ac9ea1ac81c9d438ac9bc8d38b46cb04
• as josh noted, a pure ruby version worked just fine

all the usual caveats apply about a newbie to this issue tracker :) If you need additional information or a more formal writeup here rather than linking back to github, just ask.

thank you for your help!
Adam

[jhaberman (Josh Haberman)]

Hi, I am the maintainer of the Ruby Protobuf package.

To sum up the issue, we have a case where str_buf_cat() appears to leak memory in Ruby 2.2, but the following code (which seems like it should be equivalent) does not:

rb_str_modify_expand(rb_str, len);
char *p = RSTRING_PTR(rb_str);
memcpy(p + oldlen, str, len);
rb_str_set_len(rb_str, oldlen + len);

In other versions of Ruby, str_buf_cat() does not leak memory.

[nagachika (Tomoyuki Chikanaga)]

Hello, Adam and Josh.
First of all, thank you for your report.

• https://github.com/ruby/ruby/commit/2b0a6f47ac9ea1ac81c9d438ac9bc8d38b46cb04 (r50386) isn't related with the issue, cause it revert r50336 which committed into trunk after ruby_2_2 branch has been created. Currently, str_buf_cat() is completely same between trunk(2.3-dev) and 2.2-dev.
• Which version ruby 2.2 can you reproduce the issue? Please show us ruby -v. And try with ruby-2.2-head if you can.
• I cannot reproduce with the following gist https://gist.github.com/nagachika/396fc77f95f0f4aac707 Isn't this sufficient to reproduce? Maybe some preconditions are required.

Regards,

[jhaberman (Josh Haberman)]

Hi Tomoyuki, thank you for taking a look at this.

I was afraid this might be hard to reproduce in a reduced test case. I wasn't able to reproduce it with a pure Ruby program that just calls << over and over, even though that appears to use str_buf_cat() internally.

I was using Ruby from RVM. My ruby -v looks like:

ruby 2.2.1p85 (2015-02-26 revision 49769) [x86_64-linux]

Here is the best way to reproduce the problem.

$ git clone https://github.com/google/protobuf.git
$ cd protobuf/ruby
$ ruby ext/google/protobuf_c/extconf.rb
$ make  (this will spew a lot of warnings, sorry)
$ cp protobuf_c.so lib/google/
$ export RUBYLIB=lib
$ ruby test.rb  (warning, this will allocate gigs of RAM)

Where test.rb is:

#!/usr/bin/ruby

require 'google/protobuf'
require 'pp'           
pool = Google::Protobuf::DescriptorPool.new
pool.build do
  add_message "M" do
    optional :foo, :string, 1
  end
end
M = pool.lookup("M").msgclass

# creates a ~50KB msg
m = M.new(:foo => ("hello" * 10_000))
data = M.encode(m)           
GC.start                     
puts "------------ pre run"
pp GC.stat
5_000_000.times do
  M.decode(data)
end
GC.start
puts "------------ post run"
pp GC.stat

Now edit ext/google/protobuf_c/encode_decode.c and replace rb_str_cat() on line 167 with this code, which seems like it should be equivalent:

size_t oldlen = RSTRING_LEN(rb_str);
rb_str_modify_expand(rb_str, len);
char *p = RSTRING_PTR(rb_str);
memcpy(p + oldlen, str, len);
rb_str_set_len(rb_str, oldlen + len);

Now the test no longer leaks memory.

You can also try the test with Ruby 2.1 (I tried ruby 2.1.5p273 (2014-11-13 revision 48405) [x86_64-linux]) and the test does not leak memory.

Sorry I wasn't able to create a smaller test case.

[nagachika (Tomoyuki Chikanaga)]

• Is duplicate of Bug #10942: Suspected memory leak added

[nagachika (Tomoyuki Chikanaga)]

• Status changed from Open to Closed

Hi Josh, thank you for your information.

I can reproduce the leaks with 2.2.1(p85) and 2.2.2(p95). Thanks!
And I confirmed that this issue was already fixed on ruby_2_2 branch HEAD.
I've found that r50550 (https://github.com/ruby/ruby/commit/20ef4699e510f89ad4e9ccbeafa2d3d725ed2d3c) for #10942 fixes this issue too.
Coming 2.2.3 release include the fix. Please wait a while.

[jhaberman (Josh Haberman)]

Thanks Tomoyuki.

I will plan to modify my software to use the code above (with rb_str_modify_expand()) instead of rb_str_cat() -- does this seems like a reasonable approach?

[skippy (adam greene)]

Thank you both, and Tomoyuki, I appreciate the followup.