Merge pull request #318 from Shopify/refactor-buffer-view

Refactor buffer views

Author: byroot

ChangeLog

@@ -1,3 +1,5 @@
+* Undefine `#clone` and `#dup` on `MessagePack::Buffer`, `MessagePack::Packer` and `MessagePack::Unpacker`.
+  These methods were never intended, and using them could cause leaks or crashes or worse.
 * Fix a possible GC crash when GC trigger inside `MessagePack::Buffer.new` (#314).
 
 2022-09-30 1.6.0:

ext/msgpack/buffer_class.c

@@ -69,7 +69,7 @@ static const rb_data_type_t buffer_data_type = {
 static const rb_data_type_t buffer_view_data_type = {
     .wrap_struct_name = "msgpack:buffer_view",
     .function = {
-        .dmark = msgpack_buffer_mark,
+        .dmark = NULL,
         .dfree = NULL,
         .dsize = NULL,
     },

ext/msgpack/packer_class.c

@@ -47,6 +47,7 @@ static void Packer_free(void *ptr)
 static void Packer_mark(void *ptr)
 {
     msgpack_packer_t* pk = ptr;
+    msgpack_buffer_mark(pk);
     msgpack_packer_mark(pk);
     msgpack_packer_ext_registry_mark(&pk->ext_registry);
 }
@@ -106,7 +107,7 @@ VALUE MessagePack_Packer_initialize(int argc, VALUE* argv, VALUE self)
     msgpack_packer_t *pk = MessagePack_Packer_get(self);
 
     msgpack_packer_ext_registry_init(&pk->ext_registry);
-    pk->buffer_ref = MessagePack_Buffer_wrap(PACKER_BUFFER_(pk), self);
+    pk->buffer_ref = Qnil;
 
     MessagePack_Buffer_set_options(PACKER_BUFFER_(pk), io, options);
 
@@ -129,6 +130,9 @@ static VALUE Packer_compatibility_mode_p(VALUE self)
 static VALUE Packer_buffer(VALUE self)
 {
     msgpack_packer_t *pk = MessagePack_Packer_get(self);
+    if (!RTEST(pk->buffer_ref)) {
+        pk->buffer_ref = MessagePack_Buffer_wrap(PACKER_BUFFER_(pk), self);
+    }
     return pk->buffer_ref;
 }
 

ext/msgpack/unpacker_class.c

@@ -51,6 +51,7 @@ static void Unpacker_free(void *ptr)
 static void Unpacker_mark(void *ptr)
 {
     msgpack_unpacker_t* uk = ptr;
+    msgpack_buffer_mark(uk);
     msgpack_unpacker_mark(uk);
     msgpack_unpacker_ext_registry_mark(uk->ext_registry);
 }
@@ -117,7 +118,7 @@ VALUE MessagePack_Unpacker_initialize(int argc, VALUE* argv, VALUE self)
 
     msgpack_unpacker_t *uk = MessagePack_Unpacker_get(self);
 
-    uk->buffer_ref = MessagePack_Buffer_wrap(UNPACKER_BUFFER_(uk), self);
+    uk->buffer_ref = Qnil;
 
     MessagePack_Buffer_set_options(UNPACKER_BUFFER_(uk), io, options);
 
@@ -177,6 +178,9 @@ NORETURN(static void raise_unpacker_error(int r))
 static VALUE Unpacker_buffer(VALUE self)
 {
     msgpack_unpacker_t *uk = MessagePack_Unpacker_get(self);
+    if (!RTEST(uk->buffer_ref)) {
+        uk->buffer_ref = MessagePack_Buffer_wrap(UNPACKER_BUFFER_(uk), self);
+    }
     return uk->buffer_ref;
 }
 

lib/msgpack.rb

@@ -7,6 +7,7 @@
   require "msgpack/msgpack"
 end
 
+require "msgpack/buffer"
 require "msgpack/packer"
 require "msgpack/unpacker"
 require "msgpack/factory"

lib/msgpack/buffer.rb

@@ -0,0 +1,9 @@
+module MessagePack
+  class Buffer
+    # see ext for other methods
+
+    # The semantic of duping a buffer is just too weird.
+    undef_method :dup
+    undef_method :clone
+  end
+end

lib/msgpack/packer.rb

@@ -2,6 +2,10 @@ module MessagePack
   class Packer
     # see ext for other methods
 
+    # The semantic of duping a packer is just too weird.
+    undef_method :dup
+    undef_method :clone
+
     def registered_types
       list = []
 

lib/msgpack/unpacker.rb

@@ -2,6 +2,10 @@ module MessagePack
   class Unpacker
     # see ext for other methods
 
+    # The semantic of duping an unpacker is just too weird.
+    undef_method :dup
+    undef_method :clone
+
     def registered_types
       list = []
 

spec/cruby/buffer_spec.rb

@@ -590,6 +590,11 @@
     expect(ObjectSpace.memsize_of(buffer)).to be == empty_size
   end
 
+  it "doesn't allow #dup or #clone" do
+    expect(subject).to_not respond_to :dup
+    expect(subject).to_not respond_to :clone
+  end
+
   it "doesn't crash when marking an uninitialized buffer" do
     stress = GC.stress
     begin

spec/packer_spec.rb

@@ -573,6 +573,11 @@ def to_msgpack_ext
     end
   end
 
+  it "doesn't allow #dup or #clone" do
+    expect(subject).to_not respond_to :dup
+    expect(subject).to_not respond_to :clone
+  end
+
   it "doesn't crash when marking an uninitialized buffer" do
     stress = GC.stress
     begin