Merge pull request #332 from Shopify/infinite-symbol-recusrsion

Fix an infinite recursion issue when registering a Symbol type with a `nil` packer

Author: byroot

ChangeLog

@@ -1,3 +1,5 @@
+* Fix an infinite recursion issue when registering a Symbol type with a `nil` packer.
+
 2023-03-29 1.7.0:
 
 * Fix a possible double-free issue when GC triggers inside `_msgpack_rmem_alloc2`.

ext/java/org/msgpack/jruby/ExtensionRegistry.java

@@ -141,11 +141,11 @@ public boolean isRecursive() {
     }
 
     public boolean hasPacker() {
-      return packerProc != null;
+      return packerProc != null && !packerProc.isNil();
     }
 
     public boolean hasUnpacker() {
-      return unpackerProc != null;
+      return unpackerProc != null && !unpackerProc.isNil();
     }
 
     public IRubyObject getPackerProc() {

ext/java/org/msgpack/jruby/Factory.java

@@ -100,7 +100,13 @@ public IRubyObject registerType(ThreadContext ctx, IRubyObject[] args) {
       if (args[args.length - 1] instanceof RubyHash) {
         options = (RubyHash) args[args.length - 1];
         packerArg = options.fastARef(runtime.newSymbol("packer"));
+        if (packerArg != null && packerArg.isNil()) {
+          packerArg = null;
+        }
         unpackerArg = options.fastARef(runtime.newSymbol("unpacker"));
+        if (unpackerArg != null && unpackerArg.isNil()) {
+          unpackerArg = null;
+        }
         IRubyObject optimizedSymbolsParsingArg = options.fastARef(runtime.newSymbol("optimized_symbols_parsing"));
         if (optimizedSymbolsParsingArg != null && optimizedSymbolsParsingArg.isTrue()) {
           throw runtime.newArgumentError("JRuby implementation does not support the optimized_symbols_parsing option");
@@ -147,7 +153,7 @@ public IRubyObject registerType(ThreadContext ctx, IRubyObject[] args) {
 
     extensionRegistry.put(extModule, (int) typeId, recursive, packerProc, packerArg, unpackerProc, unpackerArg);
 
-    if (extModule == runtime.getSymbol()) {
+    if (extModule == runtime.getSymbol() && !packerProc.isNil()) {
       hasSymbolExtType = true;
     }
 

ext/java/org/msgpack/jruby/Packer.java

@@ -128,7 +128,7 @@ public IRubyObject registerType(ThreadContext ctx, IRubyObject[] args, final Blo
 
     registry.put(extModule, (int) typeId, false, proc, arg, null, null);
 
-    if (extModule == runtime.getSymbol()) {
+    if (extModule == runtime.getSymbol() && !proc.isNil()) {
       encoder.hasSymbolExtType = true;
     }
 

ext/msgpack/factory_class.c

@@ -222,11 +222,12 @@ static VALUE Factory_register_type(int argc, VALUE* argv, VALUE self)
         unpacker_arg = ID2SYM(rb_intern("from_msgpack_ext"));
         break;
     case 3:
-        /* register_type(0x7f, Time, packer: proc-like, unapcker: proc-like) */
+        /* register_type(0x7f, Time, packer: proc-like, unpacker: proc-like) */
         options = argv[2];
         if(rb_type(options) != T_HASH) {
             rb_raise(rb_eArgError, "expected Hash but found %s.", rb_obj_classname(options));
         }
+
         packer_arg = rb_hash_aref(options, ID2SYM(rb_intern("packer")));
         unpacker_arg = rb_hash_aref(options, ID2SYM(rb_intern("unpacker")));
         break;
@@ -266,7 +267,9 @@ static VALUE Factory_register_type(int argc, VALUE* argv, VALUE self)
     }
 
     if(ext_module == rb_cSymbol) {
-        fc->has_symbol_ext_type = true;
+        if(NIL_P(options) || RTEST(rb_hash_aref(options, ID2SYM(rb_intern("packer"))))) {
+            fc->has_symbol_ext_type = true;
+        }
         if(RTEST(options) && RTEST(rb_hash_aref(options, ID2SYM(rb_intern("optimized_symbols_parsing"))))) {
             fc->optimized_symbol_ext_type = true;
         }

spec/factory_spec.rb

@@ -249,6 +249,12 @@ class MyType2 < MyType
       my.b.should == 2
     end
 
+    it 'handles Symbol type with `packer: nil`' do
+      factory = MessagePack::Factory.new
+      factory.register_type(0x00, Symbol, packer: nil)
+      expect(factory.load(factory.dump(:foo))).to be == "foo"
+    end
+
     describe "registering multiple ext type for the same class" do
       let(:payload) do
         factory = MessagePack::Factory.new