diff --git a/Gemfile b/Gemfile
index 953ae8ba..b7c9c371 100644
--- a/Gemfile
+++ b/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
diff --git a/History.md b/History.md
index f04ab16f..8e0d62f5 100644
--- a/History.md
+++ b/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]
diff --git a/README.md b/README.md
index 5066dc65..2c4362e0 100644
--- a/README.md
+++ b/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.
diff --git a/lib/liquid.rb b/lib/liquid.rb
index dce08977..bdd50583 100644
--- a/lib/liquid.rb
+++ b/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'
diff --git a/lib/liquid/range_slice.rb b/lib/liquid/range_slice.rb
new file mode 100644
index 00000000..a966b112
--- /dev/null
+++ b/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
diff --git a/lib/liquid/tags/for.rb b/lib/liquid/tags/for.rb
index cbea85bc..598d7bee 100644
--- a/lib/liquid/tags/for.rb
+++ b/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
diff --git a/lib/liquid/tags/table_row.rb b/lib/liquid/tags/table_row.rb
index b69f9148..7fd8530d 100644
--- a/lib/liquid/tags/table_row.rb
+++ b/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 << "\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 << "| "
super
output << ' | '
+ 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 << "
\n"
+ context.resource_limits.increment_write_score(output)
end
tablerowloop.send(:increment!)
@@ -117,6 +120,7 @@ module Liquid
end
output << "
\n"
+ context.resource_limits.increment_write_score(output)
output
end
diff --git a/lib/liquid/utils.rb b/lib/liquid/utils.rb
index 084739a2..c735a407 100644
--- a/lib/liquid/utils.rb
+++ b/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
diff --git a/test/integration/tags/for_tag_test.rb b/test/integration/tags/for_tag_test.rb
index 5b81e110..36510eea 100644
--- a/test/integration/tags/for_tag_test.rb
+++ b/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
diff --git a/test/integration/tags/table_row_test.rb b/test/integration/tags/table_row_test.rb
index 45628ce9..2e831f41 100644
--- a/test/integration/tags/table_row_test.rb
+++ b/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(
+ "\n| 2:1:998 | 2:2:999 |
\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(
+ "\n| 10 | 20 |
\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(
+ "\n| 10 | 20 |
\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(
+ "\n| 1 | 2 | 3 |
\n",
+ '{% tablerow i in numbers %}{{ i }}{% endtablerow %}',
+ { 'numbers' => range },
+ )
+ end
end
diff --git a/test/integration/template_test.rb b/test/integration/template_test.rb
index 1ba4bafd..1e41be06 100644
--- a/test/integration/template_test.rb
+++ b/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
diff --git a/test/test_helper.rb b/test/test_helper.rb
index a6d3e16e..326142b7 100755
--- a/test/test_helper.rb
+++ b/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,
diff --git a/test/unit/range_slice_unit_test.rb b/test/unit/range_slice_unit_test.rb
new file mode 100644
index 00000000..876c83e9
--- /dev/null
+++ b/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