Skip to content

Synchronize access to zstream to prevent segfault in multithreaded use #27

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jeremyevans merged 1 commit into from
Jul 16, 2021
Merged

Synchronize access to zstream to prevent segfault in multithreaded use #27

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 32 additions & 1 deletion ext/zlib/zlib.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,7 @@ struct zstream {
unsigned long flags;
VALUE buf;
VALUE input;
VALUE mutex;
z_stream stream;
const struct zstream_funcs {
int (*reset)(z_streamp);
Expand Down Expand Up @@ -621,6 +622,7 @@ zstream_init(struct zstream *z, const struct zstream_funcs *func)
z->flags = 0;
z->buf = Qnil;
z->input = Qnil;
z->mutex = rb_mutex_new();
z->stream.zalloc = zlib_mem_alloc;
z->stream.zfree = zlib_mem_free;
z->stream.opaque = Z_NULL;
Expand Down Expand Up @@ -652,7 +654,9 @@ zstream_expand_buffer(struct zstream *z)
rb_obj_reveal(z->buf, rb_cString);
}

rb_mutex_unlock(z->mutex);
rb_protect(rb_yield, z->buf, &state);
rb_mutex_lock(z->mutex);

if (ZSTREAM_REUSE_BUFFER_P(z)) {
rb_str_modify(z->buf);
Expand Down Expand Up @@ -1054,7 +1058,7 @@ zstream_unblock_func(void *ptr)
}

static void
zstream_run(struct zstream *z, Bytef *src, long len, int flush)
zstream_run0(struct zstream *z, Bytef *src, long len, int flush)
{
struct zstream_run_args args;
int err;
Expand Down Expand Up @@ -1138,6 +1142,32 @@ zstream_run(struct zstream *z, Bytef *src, long len, int flush)
rb_jump_tag(args.jump_state);
}

struct zstream_run_synchronized_args {
struct zstream *z;
Bytef *src;
long len;
int flush;
};

static VALUE
zstream_run_synchronized(VALUE value_arg)
{
struct zstream_run_synchronized_args *run_args = (struct zstream_run_synchronized_args *)value_arg;
zstream_run0(run_args->z, run_args->src, run_args->len, run_args->flush);
return Qnil;
}

static void
zstream_run(struct zstream *z, Bytef *src, long len, int flush)
{
struct zstream_run_synchronized_args run_args;
run_args.z = z;
run_args.src = src;
run_args.len = len;
run_args.flush = flush;
rb_mutex_synchronize(z->mutex, zstream_run_synchronized, (VALUE)&run_args);
}

static VALUE
zstream_sync(struct zstream *z, Bytef *src, long len)
{
Expand Down Expand Up @@ -1183,6 +1213,7 @@ zstream_mark(void *p)
struct zstream *z = p;
rb_gc_mark(z->buf);
rb_gc_mark(z->input);
rb_gc_mark(z->mutex);
}

static void
Expand Down
61 changes: 61 additions & 0 deletions test/zlib/test_zlib.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
require 'stringio'
require 'tempfile'
require 'tmpdir'
require 'securerandom'

begin
require 'zlib'
Expand Down Expand Up @@ -503,6 +504,66 @@ def test_set_dictionary
assert_raise(Zlib::StreamError) { z.set_dictionary("foo") }
z.close
end

def test_multithread_deflate
zd = Zlib::Deflate.new

s = "x" * 10000
(0...10).map do |x|
Thread.new do
1000.times { zd.deflate(s) }
end
end.each do |th|
th.join
end
ensure
zd&.finish
zd&.close
end

def test_multithread_inflate
zi = Zlib::Inflate.new

s = Zlib.deflate("x" * 10000)
(0...10).map do |x|
Thread.new do
1000.times { zi.inflate(s) }
Copy link
Member

@ioquatix ioquatix Jul 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is invalid usage and we should not be encouraging it.

Copy link
Member

@ioquatix ioquatix Jul 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://zlib.net/zlib_faq.html#faq21

Of course, you should only operate on any given zlib or gzip stream from a single thread at a time.

Copy link
Contributor Author

@jeremyevans jeremyevans Jul 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't encouraging it, merely testing it doesn't crash. I agree the spec isn't good, as you don't get the expected result when doing it. One possible way to avoid that would be to wrap the entire call to inflate and deflate in the mutex (and possibly other methods). I'll experiment with that.

end
end.each do |th|
th.join
end
ensure
zi&.finish
zi&.close
end

def test_recursive_deflate
zd = Zlib::Deflate.new

s = SecureRandom.random_bytes(1024**2)
assert_raise(Zlib::BufError) do
zd.deflate(s) do
zd.deflate(s)
end
end
ensure
zd&.finish
zd&.close
end

def test_recursive_inflate
zi = Zlib::Inflate.new

s = Zlib.deflate(SecureRandom.random_bytes(1024**2))

assert_raise(Zlib::DataError) do
zi.inflate(s) do
zi.inflate(s)
end
end
ensure
zi&.close
end
end

class TestZlibGzipFile < Test::Unit::TestCase
Expand Down