Don't call rb_str_set_len while released the GVL.

[ioquatix]

https://bugs.ruby-lang.org/issues/20863

[ioquatix]

@jeremyevans I'm looking at failure cases.

Nothing about:

static VALUE
rb_inflate_inflate(int argc, VALUE* argv, VALUE obj)
{
    struct zstream *z = get_zstream(obj);
    VALUE dst, src, opts, buffer = Qnil;

    if (OPTHASH_GIVEN_P(opts)) {
        VALUE buf;
        rb_get_kwargs(opts, &id_buffer, 0, 1, &buf);
        if (buf != Qundef && buf != Qnil) {
            buffer = StringValue(buf);
        }
    }
    if (buffer != Qnil) {
        if (!(ZSTREAM_REUSE_BUFFER_P(z) && z->buf == buffer)) {
            long len = RSTRING_LEN(buffer);
            if (len >= ZSTREAM_AVAIL_OUT_STEP_MAX) {
                rb_str_modify(buffer);
            }
            else {
                len = ZSTREAM_AVAIL_OUT_STEP_MAX - len;
                rb_str_modify_expand(buffer, len);
            }
            rb_str_set_len(buffer, 0);
            z->flags |= ZSTREAM_REUSE_BUFFER;
            z->buf = buffer;
        }
    } else if (ZSTREAM_REUSE_BUFFER_P(z)) {
        z->flags &= ~ZSTREAM_REUSE_BUFFER;
        z->buf = Qnil;
    }
    rb_scan_args(argc, argv, "10", &src);

    if (ZSTREAM_IS_FINISHED(z)) {
	if (NIL_P(src)) {
	    dst = zstream_detach_buffer(z);
	}
	else {
	    StringValue(src);
	    zstream_append_buffer2(z, src);
	    if (ZSTREAM_REUSE_BUFFER_P(z)) {
                dst = rb_str_resize(buffer, 0);
            } else {
                dst = rb_str_new(0, 0);
            }
	}
    }
    else {
	do_inflate(z, src);
	dst = zstream_detach_buffer(z);
	if (ZSTREAM_IS_FINISHED(z)) {
	    zstream_passthrough_input(z);
	}
    }

    return dst;
}

...looks safe to me in a multi-threaded context. For example, what if one call assigns to z->buf = buffer; but another is already running, using the buffer, and possibly expanding it? Or it was expanded, and then re-assigned? If we are going to use a mutex, it needs to be locked way earlier, at the start of that method at least? There is no way it is safe to use a single instance of this class across multiple threads due to the internal buffer assignment and reuse.

[jeremyevans]

Looks good to me. Please give some time for @nobu to review as well.

[jeremyevans]

My preference would be to only prefix with underscore if the argument is not used at all. But I'll leave that up to you.

[ioquatix]

I would prefer to leave it as I've done it. I understand your position. For me, _arguments is immediately converted to a known type. _arguments should not be used in the function except for this one conversion.

[ioquatix]

I found a (rather contrived) example that causes Ruby to segfault:

#!/usr/bin/env ruby

require 'zlib'
require 'securerandom'

class String
	def each_slice(n)
		0.step(self.size, n) do |i|
			yield self[i, n]
		end
	end
end

original_data = SecureRandom.random_bytes(10_000_000)
compressed_data = Zlib.deflate(original_data)

buffer = String.new(capacity: 10_000_000)
thread = Thread.new do
	while true
		buffer.clear
	end
end

$stderr.puts "Entering decompression loop..."

1.times do
	decompressor = Zlib::Inflate.new
	
	decompressed_data = String.new
	
	compressed_data.each_slice(1_000_000) do |chunk|
		decompressed_data += decompressor.inflate(chunk, buffer: buffer)
		# ./test.rb:32: [BUG] probable buffer overflow: 16384 for 15
	end
	
	decompressed_data += decompressor.finish
	
	if decompressed_data != original_data
		raise "Data corruption: #{decompressed_data.inspect}"
	end
end

$stderr.puts "Decompression loop completed."

thread.kill

Basically, the internal buffer should probably use rb_str_locktmp to avoid this.

-- C level backtrace information -------------------------------------------
/home/samuel/.rubies/ruby-head/bin/ruby(rb_print_backtrace+0x14) [0x5f528c500ae3] /home/samuel/Developer/ioquatix/ruby/vm_dump.c:822
/home/samuel/.rubies/ruby-head/bin/ruby(rb_vm_bugreport) /home/samuel/Developer/ioquatix/ruby/vm_dump.c:1151
/home/samuel/.rubies/ruby-head/bin/ruby(bug_report_end+0x0) [0x5f528c4b7880] /home/samuel/Developer/ioquatix/ruby/error.c:1084
/home/samuel/.rubies/ruby-head/bin/ruby(rb_bug_without_die) /home/samuel/Developer/ioquatix/ruby/error.c:1084
/home/samuel/.rubies/ruby-head/bin/ruby(die+0x0) [0x5f528c0db6bd] /home/samuel/Developer/ioquatix/ruby/error.c:1092
/home/samuel/.rubies/ruby-head/bin/ruby(rb_bug) /home/samuel/Developer/ioquatix/ruby/error.c:1094
/home/samuel/.rubies/ruby-head/bin/ruby(Check_Type+0x0) [0x5f528c0cffff] /home/samuel/Developer/ioquatix/ruby/string.c:3264
/home/samuel/.rubies/ruby-head/bin/ruby(rb_fstring) /home/samuel/Developer/ioquatix/ruby/string.c:486
/home/samuel/.rubies/ruby-head/lib/ruby/3.4.0+0/x86_64-linux/zlib.so(zstream_run_func+0x61) [0x74d83af71181] /home/samuel/Developer/ioquatix/ruby/ext/zlib/zlib.c:1040
/home/samuel/.rubies/ruby-head/bin/ruby(rb_nogvl+0x2c6) [0x5f528c260086] /home/samuel/Developer/ioquatix/ruby/thread.c:1557

[ioquatix]

After adding rb_str_locktmp, the error is no longer a segfault, but becomes:

Entering decompression loop...
#<Thread:0x00007cb16e7c7378 ./test.rb:18 run> terminated with exception (report_on_exception is true):
./test.rb:20:in 'String#clear': can't modify string; temporarily locked (RuntimeError)
	from ./test.rb:20:in 'block in <main>'
Decompression loop completed.