Bound resource use when iterating ranges (#2117)

Gemfile

@@ -32,7 +32,7 @@ group :test do
 end
 
 group :spec do
-  # Includes the merged specs from https://github.com/Shopify/liquid-spec/pull/144.
-  gem 'liquid-spec', github: 'Shopify/liquid-spec', ref: '8a308cb199a7d4e635ae399797560875732c8839'
+  # Includes the range resource-limit specs from https://github.com/Shopify/liquid-spec/pull/165.
+  gem 'liquid-spec', github: 'Shopify/liquid-spec', ref: '84bf25e0edbca5f7e530574788b0f875ab331f50'
   gem 'activesupport', require: false
 end

History.md

@@ -1,5 +1,9 @@
 # Liquid Change Log
 
+## Unreleased
+
+* Avoid materializing integer ranges in `for` and `tablerow` loops, and account for each visited range item.
+
 ## 5.13.0
 
 * Add TruffleRuby in CI [Benoit Daloze]

README.md

@@ -149,6 +149,13 @@ template.render!({ 'x' => 1}, { strict_variables: true })
 #=> Liquid::UndefinedVariable: Liquid error: undefined variable y
 ```
 
+### Resource limits
+
+`render_score_limit` and `cumulative_render_score_limit` account for each item visited by
+integer-range `for` and `tablerow` loops, including loops with empty bodies. This bounds range
+iteration work when a score limit is configured; `render_length_limit` only bounds generated
+output and does not by itself limit CPU work for output-free loops.
+
 ### Usage tracking
 
 To help track usages of a feature or code path in production, we have released opt-in usage tracking. To enable this, we provide an empty `Liquid:: Usage.increment` method which you can customize to your needs. The feature is well suited to https://github.com/Shopify/statsd-instrument. However, the choice of implementation is up to you.

lib/liquid.rb

@@ -83,6 +83,7 @@ require 'liquid/resource_limits'
 require 'liquid/expression'
 require 'liquid/template'
 require 'liquid/condition'
+require 'liquid/range_slice'
 require 'liquid/utils'
 require 'liquid/tokenizer'
 require 'liquid/parse_context'

lib/liquid/range_slice.rb

@@ -0,0 +1,44 @@
+# frozen_string_literal: true
+
+module Liquid
+  class RangeSlice
+    attr_reader :length
+
+    def initialize(range, from, to, resource_limits)
+      range_length = range.end - range.begin
+      range_length += 1 unless range.exclude_end?
+      range_length = 0 if range_length.negative?
+
+      start = [from, 0].max
+      finish = [to || range_length, range_length].min
+
+      @first = range.begin + start
+      @length = [finish - start, 0].max
+      @direction = 1
+      @resource_limits = resource_limits
+    end
+
+    def empty?
+      @length.zero?
+    end
+
+    def each
+      return enum_for(:each) unless block_given?
+
+      value = @first
+      @length.times do
+        @resource_limits.increment_render_score(1)
+        yield value
+        value += @direction
+      end
+    end
+
+    def reverse!
+      unless empty?
+        @first += @direction * (@length - 1)
+        @direction = -@direction
+      end
+      self
+    end
+  end
+end

lib/liquid/tags/for.rb

@@ -130,7 +130,6 @@ module Liquid
       end
 
       collection = context.evaluate(@collection_name)
-      collection = collection.to_a if collection.is_a?(Range)
 
       limit_value = context.evaluate(@limit)
       to = if limit_value.nil?
@@ -139,7 +138,9 @@ module Liquid
         Utils.to_integer(limit_value) + from
       end
 
-      segment = Utils.slice_collection(collection, from, to)
+      segment = Utils.slice_collection_for_iteration(
+        collection, from, to, context.resource_limits, use_range_to_a: true
+      )
       segment.reverse! if @reversed
 
       offsets[@name] = from + segment.length

lib/liquid/tags/table_row.rb

@@ -85,12 +85,13 @@ module Liquid
       from = @attributes.key?('offset') ? to_integer(context.evaluate(@attributes['offset'])) : 0
       to = @attributes.key?('limit') ? from + to_integer(context.evaluate(@attributes['limit'])) : nil
 
-      collection = Utils.slice_collection(collection, from, to)
+      collection = Utils.slice_collection_for_iteration(collection, from, to, context.resource_limits, allow_endless: true)
       length     = collection.length
 
       cols = @attributes.key?('cols') ? to_integer(context.evaluate(@attributes['cols'])) : length
 
       output << "<tr class=\"row1\">\n"
+      context.resource_limits.increment_write_score(output)
       context.stack do
         tablerowloop = Liquid::TablerowloopDrop.new(length, cols)
         context['tablerowloop'] = tablerowloop
@@ -101,6 +102,7 @@ module Liquid
           output << "<td class=\"col#{tablerowloop.col}\">"
           super
           output << '</td>'
+          context.resource_limits.increment_write_score(output)
 
           # Handle any interrupts if they exist.
           if context.interrupt?
@@ -110,6 +112,7 @@ module Liquid
 
           if tablerowloop.col_last && !tablerowloop.last
             output << "</tr>\n<tr class=\"row#{tablerowloop.row + 1}\">"
+            context.resource_limits.increment_write_score(output)
           end
 
           tablerowloop.send(:increment!)
@@ -117,6 +120,7 @@ module Liquid
       end
 
       output << "</tr>\n"
+      context.resource_limits.increment_write_score(output)
       output
     end
 

lib/liquid/utils.rb

@@ -13,6 +13,26 @@ module Liquid
       end
     end
 
+    # This is intentionally separate from slice_collection, whose Array-returning
+    # behavior is used outside of the iteration tags.
+    def self.slice_collection_for_iteration(
+      collection, from, to, resource_limits, allow_endless: false, use_range_to_a: false
+    )
+      if integer_range?(collection)
+        RangeSlice.new(collection, from, to, resource_limits)
+      elsif collection.is_a?(Range)
+        if use_range_to_a && range_method_overridden?(collection, :to_a)
+          # For historically honored custom Range#to_a. Charge the resulting
+          # selection before buffering it, just as for a custom #each.
+          slice_collection_for_iteration_using_each(collection.to_a, from, to, resource_limits)
+        else
+          slice_range_using_each(collection, from, to, resource_limits, allow_endless: allow_endless)
+        end
+      else
+        slice_collection(collection, from, to)
+      end
+    end
+
     def self.slice_collection_using_each(collection, from, to)
       segments = []
       index    = 0
@@ -38,6 +58,55 @@ module Liquid
       segments
     end
 
+    # Arithmetic slicing must not bypass a Range subclass's custom #each.
+    def self.integer_range?(collection)
+      collection.instance_of?(Range) && collection.begin.is_a?(Integer) && collection.end.is_a?(Integer)
+    end
+    private_class_method :integer_range?
+
+    # Preserve support for Ruby-supplied string ranges and custom Range#each.
+    # Their selected length cannot be inferred from integer bounds, but the tags
+    # need it before rendering for loop metadata, continuation offsets, and columns.
+    # Buffer the selection so we do not have to replay a potentially custom iterator.
+    def self.slice_range_using_each(collection, from, to, resource_limits, allow_endless:)
+      # TableRow historically accepted an endless subclass when its custom #each
+      # was finite, while For historically raised through Range#to_a.
+      if collection.end.nil? && !(allow_endless && (!to.nil? || range_method_overridden?(collection, :each)))
+        raise RangeError, "cannot convert endless range to an array"
+      end
+      if collection.begin.nil? && !range_method_overridden?(collection, :each)
+        raise TypeError, "can't iterate from NilClass"
+      end
+
+      slice_collection_for_iteration_using_each(collection, from, to, resource_limits)
+    end
+    private_class_method :slice_range_using_each
+
+    # Custom Range#each can make a nominally beginless range finite; standard
+    # beginless ranges were rejected before reaching this budgeted traversal.
+    def self.slice_collection_for_iteration_using_each(collection, from, to, resource_limits)
+      return [] if to && to <= from
+
+      segments = []
+      index = 0
+      collection.each do |item|
+        break if to && to <= index
+
+        # Charge preparation, including skipped offsets, before buffering; checking
+        # only while rendering the buffered values would leave this work unbudgeted.
+        resource_limits.increment_render_score(1)
+        segments << item if from <= index
+        index += 1
+      end
+      segments
+    end
+    private_class_method :slice_collection_for_iteration_using_each
+
+    def self.range_method_overridden?(collection, method_name)
+      collection.method(method_name).owner != Range.instance_method(method_name).owner
+    end
+    private_class_method :range_method_overridden?
+
     def self.to_integer(num)
       return num if num.is_a?(Integer)
       num = num.to_s

test/integration/tags/for_tag_test.rb

@@ -465,4 +465,85 @@ HERE
 
     assert(context.registers[:for_stack].empty?)
   end
+
+  def test_integer_range_is_not_materialized_and_charges_empty_iterations
+    range = bounded_integer_range_with_tripwires
+    template = Template.parse('{% for i in numbers %}{% endfor %}')
+    template.resource_limits.render_score_limit = 3
+
+    assert_raises(Liquid::MemoryError) { template.render!('numbers' => range) }
+
+    assert(template.resource_limits.reached?)
+    assert_equal(4, template.resource_limits.render_score)
+  end
+
+  def test_integer_range_uses_arithmetic_offsets_reversal_and_metadata
+    range = bounded_integer_range_with_tripwires
+    template = Template.parse(
+      '{% for i in numbers reversed offset:997 limit:2 %}{{ forloop.length }}:{{ i }}{% endfor %}',
+    )
+    template.resource_limits.render_score_limit = 10
+
+    assert_equal('2:9992:998', template.render!('numbers' => range))
+  end
+
+  def test_range_scores_cannot_be_bypassed_by_repeated_renders
+    template = Template.parse('{% for i in (1..2) %}{% endfor %}')
+    template.resource_limits.cumulative_render_score_limit = 3
+    assert_equal('', template.render!)
+    assert_raises(Liquid::MemoryError) { template.render! }
+  end
+
+  def test_range_break_charges_only_visited_items_and_preserves_full_metadata
+    range = bounded_integer_range_with_tripwires
+    template = Template.parse(
+      '{% for i in numbers reversed %}{{ forloop.length }}:{{ i }}{% break %}{% endfor %}',
+    )
+    template.resource_limits.render_score_limit = 10
+
+    assert_equal('1000:1000', template.render!('numbers' => range))
+    assert_operator(template.resource_limits.render_score, :<=, 10)
+  end
+
+  def test_range_subclass_uses_its_custom_each
+    range = Class.new(Range) do
+      def each
+        yield 10
+        yield 20
+      end
+    end.new(nil, 3)
+
+    assert_template_result('1020', '{% for i in numbers %}{{ i }}{% endfor %}', { 'numbers' => range })
+  end
+
+  def test_range_subclass_custom_to_a_is_honored_for_finite_and_open_bounds
+    range_class = Class.new(Range) do
+      def to_a
+        [42]
+      end
+    end
+
+    [range_class.new(1, 3), range_class.new(nil, 3), range_class.new(1, nil)].each do |range|
+      assert_template_result('42', '{% for i in numbers %}{{ i }}{% endfor %}', { 'numbers' => range })
+    end
+  end
+
+  def test_endless_range_remains_unsupported_with_a_limit
+    template = Template.parse('{% for i in numbers limit:2 %}{{ i }}{% endfor %}')
+
+    assert_raises(RangeError) { template.render!('numbers' => (1..)) }
+  end
+
+  def test_endless_range_subclass_with_custom_each_remains_unsupported
+    range = Class.new(Range) do
+      def each
+        yield 10
+        yield 20
+      end
+    end.new(1, nil)
+
+    assert_raises(RangeError) do
+      Template.parse('{% for i in numbers %}{{ i }}{% endfor %}').render!('numbers' => range)
+    end
+  end
 end

test/integration/tags/table_row_test.rb

@@ -465,4 +465,94 @@ class TableRowTest < Minitest::Test
       assert_match(/Unexpected character =/, error.message)
     end
   end
+
+  def test_integer_range_is_not_materialized_and_charges_empty_iterations
+    range = bounded_integer_range_with_tripwires
+    template = Template.parse('{% tablerow i in numbers %}{% endtablerow %}')
+    template.resource_limits.render_score_limit = 3
+
+    assert_raises(Liquid::MemoryError) { template.render!('numbers' => range) }
+
+    assert(template.resource_limits.reached?)
+    assert_equal(4, template.resource_limits.render_score)
+  end
+
+  def test_integer_range_uses_arithmetic_offsets_limit_and_full_metadata
+    range = bounded_integer_range_with_tripwires
+    template = Template.parse(
+      '{% tablerow i in numbers offset:997 limit:2 %}{{ tablerowloop.length }}:{{ tablerowloop.index }}:{{ i }}{% endtablerow %}',
+    )
+
+    assert_equal(
+      "<tr class=\"row1\">\n<td class=\"col1\">2:1:998</td><td class=\"col2\">2:2:999</td></tr>\n",
+      template.render!('numbers' => range),
+    )
+  end
+
+  def test_tablerow_checks_generated_output_during_empty_body_iteration
+    template = Template.parse('{% tablerow i in (1..100) %}{% endtablerow %}')
+    template.resource_limits.render_length_limit = 40
+    checked_lengths = []
+    limits = template.resource_limits
+    original_increment_write_score = limits.method(:increment_write_score)
+    limits.define_singleton_method(:increment_write_score) do |output|
+      checked_lengths << output.bytesize
+      original_increment_write_score.call(output)
+    end
+
+    assert_equal('Liquid error: Memory limits exceeded', template.render)
+    assert(template.resource_limits.reached?)
+    assert_operator(checked_lengths.last, :<, 1000)
+  end
+
+  def test_tablerow_range_scores_persist_across_renders
+    template = Template.parse('{% tablerow i in (1..2) %}{% endtablerow %}')
+    template.resource_limits.cumulative_render_score_limit = 3
+    template.render!
+    assert_raises(Liquid::MemoryError) { template.render! }
+  end
+
+  def test_range_subclass_uses_its_custom_each_with_beginless_bounds
+    range = Class.new(Range) do
+      def each
+        yield 10
+        yield 20
+      end
+    end.new(nil, 3)
+
+    assert_template_result(
+      "<tr class=\"row1\">\n<td class=\"col1\">10</td><td class=\"col2\">20</td></tr>\n",
+      '{% tablerow i in numbers %}{{ i }}{% endtablerow %}',
+      { 'numbers' => range },
+    )
+  end
+
+  def test_endless_range_subclass_uses_its_custom_each_without_a_limit
+    range = Class.new(Range) do
+      def each
+        yield 10
+        yield 20
+      end
+    end.new(1, nil)
+
+    assert_template_result(
+      "<tr class=\"row1\">\n<td class=\"col1\">10</td><td class=\"col2\">20</td></tr>\n",
+      '{% tablerow i in numbers %}{{ i }}{% endtablerow %}',
+      { 'numbers' => range },
+    )
+  end
+
+  def test_range_subclass_custom_to_a_is_not_used
+    range = Class.new(Range) do
+      def to_a
+        [42]
+      end
+    end.new(1, 3)
+
+    assert_template_result(
+      "<tr class=\"row1\">\n<td class=\"col1\">1</td><td class=\"col2\">2</td><td class=\"col3\">3</td></tr>\n",
+      '{% tablerow i in numbers %}{{ i }}{% endtablerow %}',
+      { 'numbers' => range },
+    )
+  end
 end

test/integration/template_test.rb

@@ -132,7 +132,7 @@ class TemplateTest < Minitest::Test
     assert_equal("Liquid error: Memory limits exceeded", t.render)
     assert(t.resource_limits.reached?)
 
-    t.resource_limits.render_score_limit = 200
+    t.resource_limits.render_score_limit = 201
     assert_equal(" foo " * 100, t.render!)
     refute_nil(t.resource_limits.render_score)
   end

test/test_helper.rb

@@ -32,6 +32,15 @@ module Minitest
   module Assertions
     include Liquid
 
+    # Exact Range fixture for fast-path tests; singleton tripwires must remain
+    # untouched because the arithmetic path does not materialize or traverse it.
+    def bounded_integer_range_with_tripwires
+      range = (1..1000).dup
+      range.define_singleton_method(:to_a) { raise 'range was materialized' }
+      range.define_singleton_method(:each) { raise 'range was traversed' }
+      range
+    end
+
     def assert_template_result(
       expected, template, assigns = {},
       message: nil, partials: nil, error_mode: Liquid::Environment.default.error_mode, render_errors: false,

test/unit/range_slice_unit_test.rb

@@ -0,0 +1,85 @@
+# frozen_string_literal: true
+
+require 'test_helper'
+
+class RangeSliceUnitTest < Minitest::Test
+  def test_selects_and_reverses_an_integer_range_without_enumerating_it
+    limits = Liquid::ResourceLimits.new({})
+    slice = Liquid::RangeSlice.new(bounded_integer_range_with_tripwires, 997, 999, limits)
+
+    assert_equal(2, slice.length)
+    refute(slice.empty?)
+    slice.reverse!
+    assert_equal([999, 998], slice.each.to_a)
+    assert_equal(2, limits.render_score)
+  end
+
+  def test_charges_only_values_yielded_before_a_break
+    limits = Liquid::ResourceLimits.new({})
+    slice = Liquid::RangeSlice.new(bounded_integer_range_with_tripwires, 0, nil, limits)
+
+    slice.each { break }
+
+    assert_equal(1, limits.render_score)
+  end
+
+  def test_non_integer_ranges_are_sliced_without_to_a_and_charge_visited_values
+    limits = Liquid::ResourceLimits.new({})
+    range = Class.new(Range) do
+      def to_a
+        raise 'range was materialized'
+      end
+    end.new('a', 'c')
+
+    assert_equal(['b', 'c'], Liquid::Utils.slice_collection_for_iteration(range, 1, nil, limits))
+    assert_equal(3, limits.render_score)
+
+    limited = Liquid::ResourceLimits.new(render_score_limit: 2)
+    assert_raises(Liquid::MemoryError) do
+      Liquid::Utils.slice_collection_for_iteration('a'..'z', 10, 11, limited)
+    end
+  end
+
+  def test_non_integer_range_empty_windows_do_not_visit_a_sentinel_value
+    limits = Liquid::ResourceLimits.new({})
+
+    assert_equal([], Liquid::Utils.slice_collection_for_iteration('a'..'z', 2, 2, limits))
+    assert_equal(0, limits.render_score)
+    assert_equal(['a', 'b'], Liquid::Utils.slice_collection_for_iteration('a'..'z', 0, 2, limits))
+    assert_equal(2, limits.render_score)
+  end
+
+  def test_standard_beginless_range_raises_for_empty_windows
+    [[0, 0], [1, 0]].each do |from, to|
+      assert_raises(TypeError) do
+        Liquid::Utils.slice_collection_for_iteration(Range.new(nil, 3), from, to, Liquid::ResourceLimits.new({}))
+      end
+    end
+  end
+
+  def test_custom_range_to_a_is_sliced_with_a_budget
+    range = Class.new(Range) do
+      def to_a
+        [1, 2, 3]
+      end
+    end.new(nil, 3)
+    limits = Liquid::ResourceLimits.new(render_score_limit: 2)
+
+    assert_raises(Liquid::MemoryError) do
+      Liquid::Utils.slice_collection_for_iteration(range, 1, 3, limits, use_range_to_a: true)
+    end
+    assert_equal(3, limits.render_score)
+  end
+
+  def test_preserves_slice_bounds_for_negative_offsets_and_limits
+    limits = Liquid::ResourceLimits.new({})
+
+    assert_equal([1, 2], Liquid::RangeSlice.new(1..5, -2, 2, limits).each.to_a)
+    empty = Liquid::RangeSlice.new(1..5, 2, 1, limits)
+    assert(empty.empty?)
+    assert_equal([], empty.each.to_a)
+    assert_equal(2, limits.render_score) # the empty window performs no work
+    assert(Liquid::RangeSlice.new(5..1, 0, nil, limits).empty?)
+    assert_equal([1, 2, 3, 4], Liquid::RangeSlice.new(1...5, 0, nil, limits).each.to_a)
+  end
+end