From 722c45f64110be876d83e7f9a22592aa954886c1 Mon Sep 17 00:00:00 2001
From: Aaron Lipman <alipman88@gmail.com>
Date: Mon, 8 Jul 2019 13:55:28 -0400
Subject: [PATCH] Omit marshal_dump & _dump from delegate_missing_to

Exclude missing marshal_dump and _dump methods from being delegated to
an object's delegation target via the delegate_missing_to extension.
This avoids unintentionally adding instance variables to an object
during marshallization, should the delegation target be a method which
would otherwise add them.

In current versions of Ruby, a bug exists in the way objects are
marshalled, allowing for instance variables to be added or removed
during marshallization (see https://bugs.ruby-lang.org/issues/15968).
This results in a corrupted serialized byte stream, causing an object's
instance variables to "leak" into subsequent serialized objects during
demarshallization.

In Rails, this behavior may be triggered when marshalling an object that
uses the delegate_missing_to extension, if the delegation target is a
method which adds or removes instance variables to an object being
marshalled - when calling Marshal.dump(object), Ruby's built in behavior
will check whether the object responds to :marshal_dump or :_dump, which
in turn triggers the delegation target method in the
responds_to_missing? function defined in
activesupport/lib/active_support/core_ext/module/delegation.rb

While future versions of Ruby will resolve this bug by raising a
RuntimeError, the underlying cause of this error may not be readily
apparent when encountered by Rails developers. By excluding marshal_dump
and _dump from being delegated to an object's target, this commit
eliminates a potential cause of unexpected behavior and/or
RuntimeErrors.

Fixes #36522
---
 activesupport/CHANGELOG.md                    | 11 +++++--
 .../core_ext/module/delegation.rb             |  6 ++++
 activesupport/test/core_ext/module_test.rb    | 29 +++++++++++++++++++
 3 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md
index d56d4c22de..1776972dd7 100644
--- a/activesupport/CHANGELOG.md
+++ b/activesupport/CHANGELOG.md
@@ -1,5 +1,12 @@
-*   Allow the `on_rotation` proc used when decrypting/verifying a message to be
-    passed at the constructor level.
+*   Do not delegate missing `marshal_dump` and `_dump` methods via the
+    `delegate_missing_to` extension. This avoids unintentionally adding instance
+    variables when calling `Marshal.dump(object)`, should the delegation target of
+    `object` be a method which would otherwise add them. Fixes #36522.
+
+    *Aaron Lipman*
+
+*   Allow the on_rotation proc used when decrypting/verifying a message to be
+    be passed at the constructor level.
 
     Before:
 
diff --git a/activesupport/lib/active_support/core_ext/module/delegation.rb b/activesupport/lib/active_support/core_ext/module/delegation.rb
index 54271a3970..14d7f0c484 100644
--- a/activesupport/lib/active_support/core_ext/module/delegation.rb
+++ b/activesupport/lib/active_support/core_ext/module/delegation.rb
@@ -276,6 +276,11 @@ def delegate(*methods, to: nil, prefix: nil, allow_nil: nil, private: nil)
   # The delegated method must be public on the target, otherwise it will
   # raise +DelegationError+. If you wish to instead return +nil+,
   # use the <tt>:allow_nil</tt> option.
+  #
+  # The <tt>marshal_dump</tt> and <tt>_dump</tt> methods are exempt from
+  # delegation due to possible interference when calling
+  # <tt>Marshal.dump(object)</tt>, should the delegation target method
+  # of <tt>object</tt> add or remove instance variables.
   def delegate_missing_to(target, allow_nil: nil)
     target = target.to_s
     target = "self.#{target}" if DELEGATION_RESERVED_METHOD_NAMES.include?(target)
@@ -285,6 +290,7 @@ def respond_to_missing?(name, include_private = false)
         # It may look like an oversight, but we deliberately do not pass
         # +include_private+, because they do not get delegated.
 
+        return false if name == :marshal_dump || name == :_dump
         #{target}.respond_to?(name) || super
       end
 
diff --git a/activesupport/test/core_ext/module_test.rb b/activesupport/test/core_ext/module_test.rb
index ec9ecd06ee..dd36a9373a 100644
--- a/activesupport/test/core_ext/module_test.rb
+++ b/activesupport/test/core_ext/module_test.rb
@@ -111,6 +111,24 @@ def initialize(kase)
   end
 end
 
+class Maze
+  attr_accessor :cavern, :passages
+end
+
+class Cavern
+  delegate_missing_to :target
+
+  attr_reader :maze
+
+  def initialize(maze)
+    @maze = maze
+  end
+
+  def target
+    @maze.passages = :twisty
+  end
+end
+
 class Block
   def hello?
     true
@@ -411,6 +429,17 @@ def test_delegate_missing_to_respects_superclass_missing
     assert_respond_to DecoratedTester.new(@david), :extra_missing
   end
 
+  def test_delegate_missing_to_does_not_interfere_with_marshallization
+    maze = Maze.new
+    maze.cavern = Cavern.new(maze)
+
+    array = [maze, nil]
+    serialized_array = Marshal.dump(array)
+    deserialized_array = Marshal.load(serialized_array)
+
+    assert_nil deserialized_array[1]
+  end
+
   def test_delegate_with_case
     event = Event.new(Tester.new)
     assert_equal 1, event.foo