From bd48dc15cc26f4eb556068afaafd2ab18271d8d3 Mon Sep 17 00:00:00 2001
From: Yorick Peterse <yorickpeterse@gmail.com>
Date: Mon, 7 Sep 2015 14:02:31 +0200
Subject: [PATCH] Evaluate compiled blocks in an isolated Binding

Re-using the Binding of the XPath::Compiler#compile method would lead to
race conditions, and possibly a memory leak due to the Binding sticking
around for compiled Proc's lifetime.

By using a dedicated class (and its corresponding Binding) we can work
around this. Access to this class is not synchronized as compiled Procs
don't mutate their enclosing environment.

The race condition can be demonstrated using code such as the
following:

    xml = <<-EOF
    <people>
      <person>
        <name>Alice</name>
      </person>

      <person>
        <name>Bob</name>
      </person>

      <person>
        <name>Eve</name>
      </person>
    </people>
    EOF

    4.times.map do
      Thread.new do
        10_000.times do
          document = Oga.parse_xml(xml)

          document.at_xpath('people/person/name').text
        end
      end
    end.each(&:join)

Running this code would result in NoMethodErrors due to "at_xpath"
returning a NilClass opposed to an Oga::XML::Element.
---
 lib/oga.rb                      |  1 +
 lib/oga/xpath/compiler.rb       |  7 ++++++-
 lib/oga/xpath/context.rb        | 17 +++++++++++++++++
 spec/oga/xpath/compiler_spec.rb | 22 ++++++++++++++++++++++
 spec/oga/xpath/context_spec.rb  | 23 +++++++++++++++++++++++
 5 files changed, 69 insertions(+), 1 deletion(-)
 create mode 100644 lib/oga/xpath/context.rb
 create mode 100644 spec/oga/xpath/context_spec.rb

diff --git a/lib/oga.rb b/lib/oga.rb
index 089a43a..d87cc87 100644
--- a/lib/oga.rb
+++ b/lib/oga.rb
@@ -55,6 +55,7 @@ require 'oga/ruby/generator'
 
 require 'oga/xpath/lexer'
 require 'oga/xpath/parser'
+require 'oga/xpath/context'
 require 'oga/xpath/compiler'
 require 'oga/xpath/conversion'
 
diff --git a/lib/oga/xpath/compiler.rb b/lib/oga/xpath/compiler.rb
index a0a4eca..2fc22cf 100644
--- a/lib/oga/xpath/compiler.rb
+++ b/lib/oga/xpath/compiler.rb
@@ -12,6 +12,11 @@ module Oga
       # @return [Oga::LRU]
       CACHE = LRU.new
 
+      # Context for compiled Procs. As compiled Procs do not mutate the
+      # enclosing environment we can just re-use the same instance without
+      # synchronization.
+      CONTEXT = Context.new
+
       # Wildcard for node names/namespace prefixes.
       STAR = '*'
 
@@ -86,7 +91,7 @@ module Oga
         generator = Ruby::Generator.new
         source    = generator.process(proc_ast)
 
-        eval(source)
+        CONTEXT.evaluate(source)
       ensure
         reset
       end
diff --git a/lib/oga/xpath/context.rb b/lib/oga/xpath/context.rb
new file mode 100644
index 0000000..59834c8
--- /dev/null
+++ b/lib/oga/xpath/context.rb
@@ -0,0 +1,17 @@
+module Oga
+  module XPath
+    # Class used as the context for compiled XPath Procs.
+    #
+    # The binding of this class is used for the binding of Procs compiled by
+    # {Compiler}. Not using a specific binding would result in the procs
+    # using the binding of {Compiler#compile}, which could lead to race
+    # conditions.
+    class Context
+      # @param [String] string
+      # @return [Proc]
+      def evaluate(string)
+        binding.eval(string)
+      end
+    end # Context
+  end # XPath
+end # Oga
diff --git a/spec/oga/xpath/compiler_spec.rb b/spec/oga/xpath/compiler_spec.rb
index 0d690ce..d982672 100644
--- a/spec/oga/xpath/compiler_spec.rb
+++ b/spec/oga/xpath/compiler_spec.rb
@@ -49,5 +49,27 @@ describe Oga::XPath::Compiler do
         block.call(doc).should be_an_instance_of(Oga::XML::NodeSet)
       end
     end
+
+    # This test relies on Binding#receiver (Binding#self on Rubinius), which
+    # sadly isn't available on all tested Ruby versions.
+    #
+    # Procs should have their own Binding as otherwise concurrent usage could
+    # lead to race conditions due to variable scopes and such being re-used.
+    [:receiver, :self].each do |binding_method|
+      if Binding.method_defined?(binding_method)
+        describe 'the returned Proc' do
+          it 'uses an isolated Binding' do
+            ast   = parse_xpath('foo')
+            block = @compiler.compile(ast)
+
+            binding_context = block.binding.send(binding_method)
+
+            binding_context.should be_an_instance_of(Oga::XPath::Context)
+          end
+        end
+
+        break
+      end
+    end
   end
 end
diff --git a/spec/oga/xpath/context_spec.rb b/spec/oga/xpath/context_spec.rb
new file mode 100644
index 0000000..6c9ca98
--- /dev/null
+++ b/spec/oga/xpath/context_spec.rb
@@ -0,0 +1,23 @@
+require 'spec_helper'
+
+describe Oga::XPath::Context do
+  describe '#evaluate' do
+    it 'returns the result of eval()' do
+      context = described_class.new
+
+      context.evaluate('10').should == 10
+    end
+
+    describe 'assigning a variable in a lambda' do
+      it 'assigns the variable locally to the lambda' do
+        context = described_class.new
+
+        block = context.evaluate('lambda { number = 10 }')
+
+        block.call
+
+        expect { context.evaluate('number') }.to raise_error(NameError)
+      end
+    end
+  end
+end