Fix stack level too deep error

Gemfile

@@ -1,14 +1,14 @@
 source 'https://rubygems.org'
 
 gemspec
-
-gem 'stackprof', platforms: :mri
+gem 'stackprof', platforms: :mri_21
 
 group :benchmark, :test do
   gem 'benchmark-ips'
 end
 
 group :test do
+  gem 'spy', '0.4.1'
   gem 'rubocop', '0.34.2'
 
   platform :mri do

lib/liquid/block.rb

@@ -1,7 +1,5 @@
 module Liquid
   class Block < Tag
-    MAX_DEPTH = 100
-
     def initialize(tag_name, markup, options)
       super
       @blank = true
@@ -50,25 +48,17 @@ module Liquid
     protected
 
     def parse_body(body, tokens)
-      if parse_context.depth >= MAX_DEPTH
-        raise StackLevelError, "Nesting too deep".freeze
-      end
-      parse_context.depth += 1
-      begin
-        body.parse(tokens, parse_context) do |end_tag_name, end_tag_params|
-          @blank &&= body.blank?
+      body.parse(tokens, parse_context) do |end_tag_name, end_tag_params|
+        @blank &&= body.blank?
 
-          return false if end_tag_name == block_delimiter
-          unless end_tag_name
-            raise SyntaxError.new(parse_context.locale.t("errors.syntax.tag_never_closed".freeze, block_name: block_name))
-          end
-
-          # this tag is not registered with the system
-          # pass it to the current block for special handling or error reporting
-          unknown_tag(end_tag_name, end_tag_params, tokens)
+        return false if end_tag_name == block_delimiter
+        unless end_tag_name
+          raise SyntaxError.new(parse_context.locale.t("errors.syntax.tag_never_closed".freeze, block_name: block_name))
         end
-      ensure
-        parse_context.depth -= 1
+
+        # this tag is not registered with the system
+        # pass it to the current block for special handling or error reporting
+        unknown_tag(end_tag_name, end_tag_params, tokens)
       end
 
       true

lib/liquid/condition.rb

@@ -7,6 +7,7 @@ module Liquid
   #   c.evaluate #=> true
   #
   class Condition #:nodoc:
+    @@depth = 0
     @@operators = {
       '=='.freeze => ->(cond, left, right) {  cond.send(:equal_variables, left, right) },
       '!='.freeze => ->(cond, left, right) { !cond.send(:equal_variables, left, right) },
@@ -41,22 +42,21 @@ module Liquid
     end
 
     def evaluate(context = Context.new)
-      condition = self
-      result = nil
-      loop do
-        result = interpret_condition(condition.left, condition.right, condition.operator, context)
+      result = interpret_condition(left, right, operator, context)
 
-        case condition.child_relation
-        when :or
-          break if result
-        when :and
-          break unless result
-        else
-          break
+      case @child_relation
+      when :or
+        result || @child_condition.evaluate(context)
+      when :and
+        @@depth += 1
+        if @@depth >= 500
+          @@depth = 0
+          raise StackLevelError, "Nesting too deep".freeze
         end
-        condition = condition.child_condition
+        result && @child_condition.evaluate(context)
+      else
+        result
       end
-      result
     end
 
     def or(condition)
@@ -81,10 +81,6 @@ module Liquid
       "#<Condition #{[@left, @operator, @right].compact.join(' '.freeze)}>"
     end
 
-    protected
-
-    attr_reader :child_relation, :child_condition
-
     private
 
     def equal_variables(left, right)

lib/liquid/context.rb

@@ -89,7 +89,7 @@ module Liquid
     # Push new local scope on the stack. use <tt>Context#stack</tt> instead
     def push(new_scope = {})
       @scopes.unshift(new_scope)
-      raise StackLevelError, "Nesting too deep".freeze if @scopes.length > Block::MAX_DEPTH
+      raise StackLevelError, "Nesting too deep".freeze if @scopes.length > 100
     end
 
     # Merge a hash of variables in the current local scope

lib/liquid/parse_context.rb

@@ -1,13 +1,12 @@
 module Liquid
   class ParseContext
-    attr_accessor :locale, :line_number, :trim_whitespace, :depth
+    attr_accessor :locale, :line_number, :trim_whitespace
     attr_reader :partial, :warnings, :error_mode
 
     def initialize(options = {})
       @template_options = options ? options.dup : {}
       @locale = @template_options[:locale] ||= I18n.new
       @warnings = []
-      self.depth = 0
       self.partial = false
     end
 

lib/liquid/standardfilters.rb

@@ -33,7 +33,7 @@ module Liquid
     end
 
     def escape(input)
-      CGI.escapeHTML(input.to_s).untaint unless input.nil?
+      CGI.escapeHTML(input).untaint unless input.nil?
     end
     alias_method :h, :escape
 
@@ -42,11 +42,11 @@ module Liquid
     end
 
     def url_encode(input)
-      CGI.escape(input.to_s) unless input.nil?
+      CGI.escape(input) unless input.nil?
     end
 
     def url_decode(input)
-      CGI.unescape(input.to_s) unless input.nil?
+      CGI.unescape(input) unless input.nil?
     end
 
     def slice(input, offset, length = nil)

lib/liquid/tags/for.rb

@@ -46,9 +46,6 @@ module Liquid
   class For < Block
     Syntax = /\A(#{VariableSegment}+)\s+in\s+(#{QuotedFragment}+)\s*(reversed)?/o
 
-    attr_reader :collection_name
-    attr_reader :variable_name
-
     def initialize(tag_name, markup, options)
       super
       @from = @limit = nil
@@ -129,7 +126,7 @@ module Liquid
       end
 
       collection = context.evaluate(@collection_name)
-      collection = collection.step(1).to_a if collection.is_a?(Range)
+      collection = collection.to_a if collection.is_a?(Range)
 
       limit = context.evaluate(@limit)
       to = limit ? limit.to_i + from : nil

lib/liquid/tags/if.rb

@@ -83,20 +83,17 @@ module Liquid
 
     def strict_parse(markup)
       p = Parser.new(markup)
-      condition = parse_binary_comparisons(p)
+      condition = parse_binary_comparison(p)
       p.consume(:end_of_string)
       condition
     end
 
-    def parse_binary_comparisons(p)
+    def parse_binary_comparison(p)
       condition = parse_comparison(p)
-      first_condition = condition
-      while op = (p.id?('and'.freeze) || p.id?('or'.freeze))
-        child_condition = parse_comparison(p)
-        condition.send(op, child_condition)
-        condition = child_condition
+      if op = (p.id?('and'.freeze) || p.id?('or'.freeze))
+        condition.send(op, parse_binary_comparison(p))
       end
-      first_condition
+      condition
     end
 
     def parse_comparison(p)

test/integration/security_test.rb

@@ -63,18 +63,4 @@ class SecurityTest < Minitest::Test
 
     assert_equal [], (Symbol.all_symbols - current_symbols)
   end
-
-  def test_max_depth_nested_blocks_does_not_raise_exception
-    depth = Liquid::Block::MAX_DEPTH
-    code = "{% if true %}" * depth + "rendered" + "{% endif %}" * depth
-    assert_equal "rendered", Template.parse(code).render!
-  end
-
-  def test_more_than_max_depth_nested_blocks_raises_exception
-    depth = Liquid::Block::MAX_DEPTH + 1
-    code = "{% if true %}" * depth + "rendered" + "{% endif %}" * depth
-    assert_raises(Liquid::StackLevelError) do
-      Template.parse(code).render!
-    end
-  end
 end # SecurityTest

test/integration/standard_filter_test.rb

@@ -128,16 +128,8 @@ class StandardFiltersTest < Minitest::Test
 
   def test_escape
     assert_equal '&lt;strong&gt;', @filters.escape('<strong>')
-    assert_equal '1', @filters.escape(1)
-    assert_equal '2001-02-03', @filters.escape(Date.new(2001, 2, 3))
-    assert_nil @filters.escape(nil)
-  end
-
-  def test_h
+    assert_equal nil, @filters.escape(nil)
     assert_equal '&lt;strong&gt;', @filters.h('<strong>')
-    assert_equal '1', @filters.h(1)
-    assert_equal '2001-02-03', @filters.h(Date.new(2001, 2, 3))
-    assert_nil @filters.h(nil)
   end
 
   def test_escape_once
@@ -146,18 +138,14 @@ class StandardFiltersTest < Minitest::Test
 
   def test_url_encode
     assert_equal 'foo%2B1%40example.com', @filters.url_encode('foo+1@example.com')
-    assert_equal '1', @filters.url_encode(1)
-    assert_equal '2001-02-03', @filters.url_encode(Date.new(2001, 2, 3))
-    assert_nil @filters.url_encode(nil)
+    assert_equal nil, @filters.url_encode(nil)
   end
 
   def test_url_decode
     assert_equal 'foo bar', @filters.url_decode('foo+bar')
     assert_equal 'foo bar', @filters.url_decode('foo%20bar')
     assert_equal 'foo+1@example.com', @filters.url_decode('foo%2B1%40example.com')
-    assert_equal '1', @filters.url_decode(1)
-    assert_equal '2001-02-03', @filters.url_decode(Date.new(2001, 2, 3))
-    assert_nil @filters.url_decode(nil)
+    assert_equal nil, @filters.url_decode(nil)
   end
 
   def test_truncatewords
@@ -342,7 +330,7 @@ class StandardFiltersTest < Minitest::Test
     assert_equal "#{Date.today.year}", @filters.date('today', '%Y')
     assert_equal "#{Date.today.year}", @filters.date('Today', '%Y')
 
-    assert_nil @filters.date(nil, "%B")
+    assert_equal nil, @filters.date(nil, "%B")
 
     assert_equal '', @filters.date('', "%B")
 
@@ -355,8 +343,8 @@ class StandardFiltersTest < Minitest::Test
   def test_first_last
     assert_equal 1, @filters.first([1, 2, 3])
     assert_equal 3, @filters.last([1, 2, 3])
-    assert_nil @filters.first([])
-    assert_nil @filters.last([])
+    assert_equal nil, @filters.first([])
+    assert_equal nil, @filters.last([])
   end
 
   def test_replace

test/integration/tags/for_tag_test.rb

@@ -48,10 +48,6 @@ HERE
 
   def test_for_with_variable_range
     assert_template_result(' 1  2  3 ', '{%for item in (1..foobar) %} {{item}} {%endfor%}', "foobar" => 3)
-    assert_template_result(' 1.0  2.0  3.0 ', '{%for item in foobar %} {{item}} {%endfor%}', "foobar" => (1..3.0))
-    assert_template_result(' 1.0  2.0  3.0 ', '{%for item in foobar %} {{item}} {%endfor%}', "foobar" => (1.0..3))
-    assert_template_result(' 1.0  2.0  3.0 ', '{%for item in foobar %} {{item}} {%endfor%}', "foobar" => (1.0..3.0))
-    assert_template_result(' 1.5  2.5 ', '{%for item in foobar %} {{item}} {%endfor%}', "foobar" => (1.5..3))
   end
 
   def test_for_with_hash_value_range

test/integration/tags/include_tag_test.rb

@@ -137,7 +137,7 @@ class IncludeTagTest < Minitest::Test
 
     Liquid::Template.file_system = infinite_file_system.new
 
-    assert_raises(Liquid::StackLevelError) do
+    assert_raises(Liquid::StackLevelError, SystemStackError) do
       Template.parse("{% include 'loop' %}").render!
     end
   end

test/test_helper.rb

@@ -2,6 +2,7 @@
 
 ENV["MT_NO_EXPECTATIONS"] = "1"
 require 'minitest/autorun'
+require 'spy/integration'
 
 $LOAD_PATH.unshift(File.join(File.expand_path(__dir__), '..', 'lib'))
 require 'liquid.rb'

test/unit/condition_unit_test.rb

@@ -65,8 +65,8 @@ class ConditionUnitTest < Minitest::Test
   end
 
   def test_hash_compare_backwards_compatibility
-    assert_nil Condition.new({}, '>', 2).evaluate
-    assert_nil Condition.new(2, '>', {}).evaluate
+    assert_equal nil, Condition.new({}, '>', 2).evaluate
+    assert_equal nil, Condition.new(2, '>', {}).evaluate
     assert_equal false, Condition.new({}, '==', 2).evaluate
     assert_equal true, Condition.new({ 'a' => 1 }, '==', { 'a' => 1 }).evaluate
     assert_equal true, Condition.new({ 'a' => 2 }, 'contains', 'a').evaluate
@@ -130,6 +130,17 @@ class ConditionUnitTest < Minitest::Test
     assert_equal false, condition.evaluate
   end
 
+  def test_maximum_recursion_depth
+    condition = Condition.new(1, '==', 1)
+
+    assert_raises(Liquid::StackLevelError) do
+      (1..510).each do
+        condition.evaluate
+        condition.and Condition.new(2, '==', 2)
+      end
+    end
+  end
+
   def test_should_allow_custom_proc_operator
     Condition.operators['starts_with'] = proc { |cond, left, right| left =~ %r{^#{right}} }
 

test/unit/context_unit_test.rb

@@ -70,6 +70,10 @@ class ContextUnitTest < Minitest::Test
     @context = Liquid::Context.new
   end
 
+  def teardown
+    Spy.teardown
+  end
+
   def test_variables
     @context['string'] = 'string'
     assert_equal 'string', @context['string']
@@ -94,12 +98,12 @@ class ContextUnitTest < Minitest::Test
     assert_equal false, @context['bool']
 
     @context['nil'] = nil
-    assert_nil @context['nil']
-    assert_nil @context['nil']
+    assert_equal nil, @context['nil']
+    assert_equal nil, @context['nil']
   end
 
   def test_variables_not_existing
-    assert_nil @context['does_not_exist']
+    assert_equal nil, @context['does_not_exist']
   end
 
   def test_scoping
@@ -181,7 +185,7 @@ class ContextUnitTest < Minitest::Test
     @context['test'] = 'test'
     assert_equal 'test', @context['test']
     @context.pop
-    assert_nil @context['test']
+    assert_equal nil, @context['test']
   end
 
   def test_hierachical_data
@@ -296,7 +300,7 @@ class ContextUnitTest < Minitest::Test
     @context['hash'] = { 'first' => 'Hello' }
 
     assert_equal 1, @context['array.first']
-    assert_nil @context['array["first"]']
+    assert_equal nil, @context['array["first"]']
     assert_equal 'Hello', @context['hash["first"]']
   end
 
@@ -446,10 +450,14 @@ class ContextUnitTest < Minitest::Test
     assert_equal @context, @context['category'].context
   end
 
-  def test_interrupt_avoids_object_allocations
-    assert_no_object_allocations do
-      @context.interrupt?
-    end
+  def test_use_empty_instead_of_any_in_interrupt_handling_to_avoid_lots_of_unnecessary_object_allocations
+    mock_any = Spy.on_instance_method(Array, :any?)
+    mock_empty = Spy.on_instance_method(Array, :empty?)
+
+    @context.interrupt?
+
+    refute mock_any.has_been_called?
+    assert mock_empty.has_been_called?
   end
 
   def test_context_initialization_with_a_proc_in_environment
@@ -472,18 +480,4 @@ class ContextUnitTest < Minitest::Test
     context = Context.new
     assert_equal 'hi', context.apply_global_filter('hi')
   end
-
-  private
-
-  def assert_no_object_allocations
-    unless RUBY_ENGINE == 'ruby'
-      skip "stackprof needed to count object allocations"
-    end
-    require 'stackprof'
-
-    profile = StackProf.run(mode: :object) do
-      yield
-    end
-    assert_equal 0, profile[:samples]
-  end
 end # ContextTest