From db00ec8b324b0144438c090dd30bf5d8ab2c37b6 Mon Sep 17 00:00:00 2001 From: Justin Li Date: Thu, 4 Dec 2014 15:50:40 -0500 Subject: [PATCH 1/8] Move resource limit tracking to its own class --- lib/liquid.rb | 1 + lib/liquid/block_body.rb | 9 ++++--- lib/liquid/context.rb | 23 +++++------------- lib/liquid/resource_limits.rb | 40 +++++++++++++++++++++++++++++++ lib/liquid/tags/assign.rb | 2 +- lib/liquid/tags/capture.rb | 2 +- lib/liquid/template.rb | 10 ++++++-- test/integration/template_test.rb | 35 ++++++++++++++------------- 8 files changed, 79 insertions(+), 43 deletions(-) create mode 100644 lib/liquid/resource_limits.rb diff --git a/lib/liquid.rb b/lib/liquid.rb index af38cd4..8e5af20 100644 --- a/lib/liquid.rb +++ b/lib/liquid.rb @@ -63,6 +63,7 @@ require 'liquid/variable' require 'liquid/variable_lookup' require 'liquid/range_lookup' require 'liquid/file_system' +require 'liquid/resource_limits' require 'liquid/template' require 'liquid/standardfilters' require 'liquid/condition' diff --git a/lib/liquid/block_body.rb b/lib/liquid/block_body.rb index 5e8a965..1d2127a 100644 --- a/lib/liquid/block_body.rb +++ b/lib/liquid/block_body.rb @@ -69,8 +69,8 @@ module Liquid def render(context) output = [] - context.resource_limits[:render_length_current] = 0 - context.resource_limits[:render_score_current] += @nodelist.length + context.resource_limits.render_length = 0 + context.resource_limits.render_score += @nodelist.length @nodelist.each do |token| # Break out if we have any unhanded interrupts. @@ -104,9 +104,8 @@ module Liquid def render_token(token, context) token_output = (token.respond_to?(:render) ? token.render(context) : token) - context.increment_used_resources(:render_length_current, token_output) - if context.resource_limits_reached? - context.resource_limits[:reached] = true + context.resource_limits.increment_render_length(token_output) + if context.resource_limits.reached? raise MemoryError.new("Memory limits exceeded".freeze) end token_output diff --git a/lib/liquid/context.rb b/lib/liquid/context.rb index 179a466..3aefd34 100644 --- a/lib/liquid/context.rb +++ b/lib/liquid/context.rb @@ -21,9 +21,12 @@ module Liquid @scopes = [(outer_scope || {})] @registers = registers @errors = [] - @resource_limits = resource_limits || Template.default_resource_limits.dup - @resource_limits[:render_score_current] = 0 - @resource_limits[:assign_score_current] = 0 + + @resource_limits = if resource_limits.is_a?(ResourceLimits) + resource_limits + else + ResourceLimits.new(resource_limits || Template.default_resource_limits) + end squash_instance_assigns_with_environments @this_stack_used = false @@ -36,20 +39,6 @@ module Liquid @filters = [] end - def increment_used_resources(key, obj) - @resource_limits[key] += if obj.kind_of?(String) || obj.kind_of?(Array) || obj.kind_of?(Hash) - obj.length - else - 1 - end - end - - def resource_limits_reached? - (@resource_limits[:render_length_limit] && @resource_limits[:render_length_current] > @resource_limits[:render_length_limit]) || - (@resource_limits[:render_score_limit] && @resource_limits[:render_score_current] > @resource_limits[:render_score_limit] ) || - (@resource_limits[:assign_score_limit] && @resource_limits[:assign_score_current] > @resource_limits[:assign_score_limit] ) - end - def strainer @strainer ||= Strainer.create(self, @filters) end diff --git a/lib/liquid/resource_limits.rb b/lib/liquid/resource_limits.rb new file mode 100644 index 0000000..8902846 --- /dev/null +++ b/lib/liquid/resource_limits.rb @@ -0,0 +1,40 @@ +module Liquid + class ResourceLimits + attr_accessor :render_length, :render_score, :assign_score, + :render_length_limit, :render_score_limit, :assign_score_limit + + def initialize(limits) + @render_length_limit = limits[:render_length_limit] + @render_score_limit = limits[:render_score_limit] + @assign_score_limit = limits[:assign_score_limit] + + # render_length is assigned by BlockBody + # @render_length = 0 + @render_score = 0 + @assign_score = 0 + end + + def reached? + (@render_length_limit && @render_length > @render_length_limit) || + (@render_score_limit && @render_score > @render_score_limit ) || + (@assign_score_limit && @assign_score > @assign_score_limit ) + end + + def increment_render_length(obj) + @render_length += increment_for(obj) + end + + def increment_render_score(obj) + @render_score += increment_for(obj) + end + + def increment_assign_score(obj) + @assign_score += increment_for(obj) + end + + private + def increment_for(obj) + obj.instance_of?(String) || obj.instance_of?(Array) || obj.instance_of?(Hash) ? obj.length : 1 + end + end +end diff --git a/lib/liquid/tags/assign.rb b/lib/liquid/tags/assign.rb index cda3878..2db8c83 100644 --- a/lib/liquid/tags/assign.rb +++ b/lib/liquid/tags/assign.rb @@ -25,7 +25,7 @@ module Liquid def render(context) val = @from.render(context) context.scopes.last[@to] = val - context.increment_used_resources(:assign_score_current, val) + context.resource_limits.increment_assign_score(val) ''.freeze end diff --git a/lib/liquid/tags/capture.rb b/lib/liquid/tags/capture.rb index 3ec0d67..4df902f 100644 --- a/lib/liquid/tags/capture.rb +++ b/lib/liquid/tags/capture.rb @@ -25,7 +25,7 @@ module Liquid def render(context) output = super context.scopes.last[@to] = output - context.increment_used_resources(:assign_score_current, output) + context.resource_limits.increment_assign_score(output) ''.freeze end diff --git a/lib/liquid/template.rb b/lib/liquid/template.rb index 8862564..277c026 100644 --- a/lib/liquid/template.rb +++ b/lib/liquid/template.rb @@ -18,7 +18,13 @@ module Liquid :locale => I18n.new } - attr_accessor :root, :resource_limits + attr_accessor :root + attr_reader :resource_limits + + def resource_limits=(limits) + @resource_limits = ResourceLimits.new(limits) + end + @@file_system = BlankFileSystem.new class TagRegistry @@ -110,7 +116,7 @@ module Liquid end def initialize - @resource_limits = self.class.default_resource_limits.dup + self.resource_limits = self.class.default_resource_limits end # Parse source code. diff --git a/test/integration/template_test.rb b/test/integration/template_test.rb index e395206..4857b6e 100644 --- a/test/integration/template_test.rb +++ b/test/integration/template_test.rb @@ -101,61 +101,62 @@ class TemplateTest < Minitest::Test t = Template.parse("0123456789") t.resource_limits = { :render_length_limit => 5 } assert_equal "Liquid error: Memory limits exceeded", t.render() - assert t.resource_limits[:reached] + assert t.resource_limits.reached? + t.resource_limits = { :render_length_limit => 10 } assert_equal "0123456789", t.render!() - refute_nil t.resource_limits[:render_length_current] + refute_nil t.resource_limits.render_length end def test_resource_limits_render_score t = Template.parse("{% for a in (1..10) %} {% for a in (1..10) %} foo {% endfor %} {% endfor %}") t.resource_limits = { :render_score_limit => 50 } assert_equal "Liquid error: Memory limits exceeded", t.render() - assert t.resource_limits[:reached] + assert t.resource_limits.reached? + t = Template.parse("{% for a in (1..100) %} foo {% endfor %}") t.resource_limits = { :render_score_limit => 50 } assert_equal "Liquid error: Memory limits exceeded", t.render() - assert t.resource_limits[:reached] + assert t.resource_limits.reached? + t.resource_limits = { :render_score_limit => 200 } assert_equal (" foo " * 100), t.render!() - refute_nil t.resource_limits[:render_score_current] + refute_nil t.resource_limits.render_score end def test_resource_limits_assign_score t = Template.parse("{% assign foo = 42 %}{% assign bar = 23 %}") t.resource_limits = { :assign_score_limit => 1 } assert_equal "Liquid error: Memory limits exceeded", t.render() - assert t.resource_limits[:reached] + assert t.resource_limits.reached? + t.resource_limits = { :assign_score_limit => 2 } assert_equal "", t.render!() - refute_nil t.resource_limits[:assign_score_current] + refute_nil t.resource_limits.assign_score end def test_resource_limits_aborts_rendering_after_first_error t = Template.parse("{% for a in (1..100) %} foo1 {% endfor %} bar {% for a in (1..100) %} foo2 {% endfor %}") t.resource_limits = { :render_score_limit => 50 } assert_equal "Liquid error: Memory limits exceeded", t.render() - assert t.resource_limits[:reached] + assert t.resource_limits.reached? end def test_resource_limits_hash_in_template_gets_updated_even_if_no_limits_are_set t = Template.parse("{% for a in (1..100) %} {% assign foo = 1 %} {% endfor %}") t.render!() - assert t.resource_limits[:assign_score_current] > 0 - assert t.resource_limits[:render_score_current] > 0 - assert t.resource_limits[:render_length_current] > 0 + assert t.resource_limits.assign_score > 0 + assert t.resource_limits.render_score > 0 + assert t.resource_limits.render_length > 0 end def test_default_resource_limits_unaffected_by_render_with_context context = Context.new t = Template.parse("{% for a in (1..100) %} {% assign foo = 1 %} {% endfor %}") t.render!(context) - assert context.resource_limits[:assign_score_current] > 0 - assert context.resource_limits[:render_score_current] > 0 - assert context.resource_limits[:render_length_current] > 0 - refute Template.default_resource_limits.key?(:assign_score_current) - refute Template.default_resource_limits.key?(:render_score_current) - refute Template.default_resource_limits.key?(:render_length_current) + assert context.resource_limits.assign_score > 0 + assert context.resource_limits.render_score > 0 + assert context.resource_limits.render_length > 0 end def test_can_use_drop_as_context From 1593b784a790b7da9ee2a0e83ebfa94be2e87e2d Mon Sep 17 00:00:00 2001 From: Justin Li Date: Thu, 4 Dec 2014 16:17:53 -0500 Subject: [PATCH 2/8] Simplify interface for setting template resource limits --- lib/liquid/context.rb | 7 +------ lib/liquid/template.rb | 6 +----- test/integration/template_test.rb | 23 +++++++++++++---------- 3 files changed, 15 insertions(+), 21 deletions(-) diff --git a/lib/liquid/context.rb b/lib/liquid/context.rb index 3aefd34..ab19071 100644 --- a/lib/liquid/context.rb +++ b/lib/liquid/context.rb @@ -21,12 +21,7 @@ module Liquid @scopes = [(outer_scope || {})] @registers = registers @errors = [] - - @resource_limits = if resource_limits.is_a?(ResourceLimits) - resource_limits - else - ResourceLimits.new(resource_limits || Template.default_resource_limits) - end + @resource_limits = resource_limits || ResourceLimits.new(Template.default_resource_limits) squash_instance_assigns_with_environments @this_stack_used = false diff --git a/lib/liquid/template.rb b/lib/liquid/template.rb index 277c026..9928c81 100644 --- a/lib/liquid/template.rb +++ b/lib/liquid/template.rb @@ -21,10 +21,6 @@ module Liquid attr_accessor :root attr_reader :resource_limits - def resource_limits=(limits) - @resource_limits = ResourceLimits.new(limits) - end - @@file_system = BlankFileSystem.new class TagRegistry @@ -116,7 +112,7 @@ module Liquid end def initialize - self.resource_limits = self.class.default_resource_limits + @resource_limits = ResourceLimits.new(self.class.default_resource_limits) end # Parse source code. diff --git a/test/integration/template_test.rb b/test/integration/template_test.rb index 4857b6e..2e923af 100644 --- a/test/integration/template_test.rb +++ b/test/integration/template_test.rb @@ -93,51 +93,54 @@ class TemplateTest < Minitest::Test def test_resource_limits_works_with_custom_length_method t = Template.parse("{% assign foo = bar %}") - t.resource_limits = { :render_length_limit => 42 } + t.resource_limits.render_length_limit = 42 assert_equal "", t.render!("bar" => SomethingWithLength.new) end def test_resource_limits_render_length t = Template.parse("0123456789") - t.resource_limits = { :render_length_limit => 5 } + t.resource_limits.render_length_limit = 5 assert_equal "Liquid error: Memory limits exceeded", t.render() assert t.resource_limits.reached? - t.resource_limits = { :render_length_limit => 10 } + t.resource_limits.render_length_limit = 10 assert_equal "0123456789", t.render!() refute_nil t.resource_limits.render_length end def test_resource_limits_render_score t = Template.parse("{% for a in (1..10) %} {% for a in (1..10) %} foo {% endfor %} {% endfor %}") - t.resource_limits = { :render_score_limit => 50 } + t.resource_limits.render_score_limit = 50 assert_equal "Liquid error: Memory limits exceeded", t.render() assert t.resource_limits.reached? t = Template.parse("{% for a in (1..100) %} foo {% endfor %}") - t.resource_limits = { :render_score_limit => 50 } + t.resource_limits.render_score_limit = 50 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 = 200 assert_equal (" foo " * 100), t.render!() refute_nil t.resource_limits.render_score end def test_resource_limits_assign_score - t = Template.parse("{% assign foo = 42 %}{% assign bar = 23 %}") - t.resource_limits = { :assign_score_limit => 1 } + markup = "{% assign foo = 42 %}{% assign bar = 23 %}" + + t = Template.parse(markup) + t.resource_limits.assign_score_limit = 1 assert_equal "Liquid error: Memory limits exceeded", t.render() assert t.resource_limits.reached? - t.resource_limits = { :assign_score_limit => 2 } + t = Template.parse(markup) + t.resource_limits.assign_score_limit = 2 assert_equal "", t.render!() refute_nil t.resource_limits.assign_score end def test_resource_limits_aborts_rendering_after_first_error t = Template.parse("{% for a in (1..100) %} foo1 {% endfor %} bar {% for a in (1..100) %} foo2 {% endfor %}") - t.resource_limits = { :render_score_limit => 50 } + t.resource_limits.render_score_limit = 50 assert_equal "Liquid error: Memory limits exceeded", t.render() assert t.resource_limits.reached? end From 742b3c69bba6f9010f9528040f7431ff7ab7aa05 Mon Sep 17 00:00:00 2001 From: Justin Li Date: Thu, 4 Dec 2014 16:30:37 -0500 Subject: [PATCH 3/8] Remove commented code --- lib/liquid/resource_limits.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/liquid/resource_limits.rb b/lib/liquid/resource_limits.rb index 8902846..3f9bdba 100644 --- a/lib/liquid/resource_limits.rb +++ b/lib/liquid/resource_limits.rb @@ -9,7 +9,6 @@ module Liquid @assign_score_limit = limits[:assign_score_limit] # render_length is assigned by BlockBody - # @render_length = 0 @render_score = 0 @assign_score = 0 end From 9f7e6011109e0f979043f89542f5bcebea51e407 Mon Sep 17 00:00:00 2001 From: Justin Li Date: Fri, 5 Dec 2014 15:17:09 -0500 Subject: [PATCH 4/8] Convert render output to strings in BlockBody --- lib/liquid/block_body.rb | 6 ++++-- lib/liquid/resource_limits.rb | 17 ----------------- lib/liquid/tags/assign.rb | 5 ++++- lib/liquid/tags/capture.rb | 2 +- 4 files changed, 9 insertions(+), 21 deletions(-) diff --git a/lib/liquid/block_body.rb b/lib/liquid/block_body.rb index 1d2127a..9999826 100644 --- a/lib/liquid/block_body.rb +++ b/lib/liquid/block_body.rb @@ -104,11 +104,13 @@ module Liquid def render_token(token, context) token_output = (token.respond_to?(:render) ? token.render(context) : token) - context.resource_limits.increment_render_length(token_output) + token_str = token_output.is_a?(Array) ? token_output.join : token_output.to_s + + context.resource_limits.render_length += token_str.length if context.resource_limits.reached? raise MemoryError.new("Memory limits exceeded".freeze) end - token_output + token_str end def create_variable(token, options) diff --git a/lib/liquid/resource_limits.rb b/lib/liquid/resource_limits.rb index 3f9bdba..6373130 100644 --- a/lib/liquid/resource_limits.rb +++ b/lib/liquid/resource_limits.rb @@ -18,22 +18,5 @@ module Liquid (@render_score_limit && @render_score > @render_score_limit ) || (@assign_score_limit && @assign_score > @assign_score_limit ) end - - def increment_render_length(obj) - @render_length += increment_for(obj) - end - - def increment_render_score(obj) - @render_score += increment_for(obj) - end - - def increment_assign_score(obj) - @assign_score += increment_for(obj) - end - - private - def increment_for(obj) - obj.instance_of?(String) || obj.instance_of?(Array) || obj.instance_of?(Hash) ? obj.length : 1 - end end end diff --git a/lib/liquid/tags/assign.rb b/lib/liquid/tags/assign.rb index 2db8c83..f1dfb19 100644 --- a/lib/liquid/tags/assign.rb +++ b/lib/liquid/tags/assign.rb @@ -25,7 +25,10 @@ module Liquid def render(context) val = @from.render(context) context.scopes.last[@to] = val - context.resource_limits.increment_assign_score(val) + + inc = val.instance_of?(String) || val.instance_of?(Array) || val.instance_of?(Hash) ? val.length : 1 + context.resource_limits.assign_score += inc + ''.freeze end diff --git a/lib/liquid/tags/capture.rb b/lib/liquid/tags/capture.rb index 4df902f..6ec8a71 100644 --- a/lib/liquid/tags/capture.rb +++ b/lib/liquid/tags/capture.rb @@ -25,7 +25,7 @@ module Liquid def render(context) output = super context.scopes.last[@to] = output - context.resource_limits.increment_assign_score(output) + context.resource_limits.assign_score += output.length ''.freeze end From c2f71ee86b848c8e33b8b2e8438a91ed82dccf5c Mon Sep 17 00:00:00 2001 From: Justin Li Date: Tue, 9 Dec 2014 17:23:07 -0500 Subject: [PATCH 5/8] Reset resource consumption before each render --- lib/liquid/block_body.rb | 1 - lib/liquid/resource_limits.rb | 9 +++++---- lib/liquid/template.rb | 3 +++ 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/liquid/block_body.rb b/lib/liquid/block_body.rb index 9999826..371e058 100644 --- a/lib/liquid/block_body.rb +++ b/lib/liquid/block_body.rb @@ -69,7 +69,6 @@ module Liquid def render(context) output = [] - context.resource_limits.render_length = 0 context.resource_limits.render_score += @nodelist.length @nodelist.each do |token| diff --git a/lib/liquid/resource_limits.rb b/lib/liquid/resource_limits.rb index 6373130..190685e 100644 --- a/lib/liquid/resource_limits.rb +++ b/lib/liquid/resource_limits.rb @@ -7,10 +7,7 @@ module Liquid @render_length_limit = limits[:render_length_limit] @render_score_limit = limits[:render_score_limit] @assign_score_limit = limits[:assign_score_limit] - - # render_length is assigned by BlockBody - @render_score = 0 - @assign_score = 0 + reset end def reached? @@ -18,5 +15,9 @@ module Liquid (@render_score_limit && @render_score > @render_score_limit ) || (@assign_score_limit && @assign_score > @assign_score_limit ) end + + def reset + @render_length = @render_score = @assign_score = 0 + end end end diff --git a/lib/liquid/template.rb b/lib/liquid/template.rb index 9928c81..2ad7c42 100644 --- a/lib/liquid/template.rb +++ b/lib/liquid/template.rb @@ -205,6 +205,9 @@ module Liquid context.add_filters(args.pop) end + # Retrying a render resets resource usage + context.resource_limits.reset + begin # render the nodelist. # for performance reasons we get an array back here. join will make a string out of it. From 4df4f218cf3a8811eb67c1ce33914b4d310a7c19 Mon Sep 17 00:00:00 2001 From: Justin Li Date: Tue, 9 Dec 2014 17:25:15 -0500 Subject: [PATCH 6/8] Use same template instance --- test/integration/template_test.rb | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/test/integration/template_test.rb b/test/integration/template_test.rb index 2e923af..f5ebb8f 100644 --- a/test/integration/template_test.rb +++ b/test/integration/template_test.rb @@ -125,14 +125,11 @@ class TemplateTest < Minitest::Test end def test_resource_limits_assign_score - markup = "{% assign foo = 42 %}{% assign bar = 23 %}" - - t = Template.parse(markup) + t = Template.parse("{% assign foo = 42 %}{% assign bar = 23 %}") t.resource_limits.assign_score_limit = 1 assert_equal "Liquid error: Memory limits exceeded", t.render() assert t.resource_limits.reached? - t = Template.parse(markup) t.resource_limits.assign_score_limit = 2 assert_equal "", t.render!() refute_nil t.resource_limits.assign_score From cc57908c0323c237f8fc64823f0a3836c55956b0 Mon Sep 17 00:00:00 2001 From: Justin Li Date: Thu, 11 Dec 2014 10:38:47 -0500 Subject: [PATCH 7/8] Add test for render_length persisting between block bodies --- test/integration/template_test.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/integration/template_test.rb b/test/integration/template_test.rb index f5ebb8f..d565a9b 100644 --- a/test/integration/template_test.rb +++ b/test/integration/template_test.rb @@ -150,6 +150,13 @@ class TemplateTest < Minitest::Test assert t.resource_limits.render_length > 0 end + def test_render_length_persists_between_blocks + t = Template.parse("{% if true %}a{% endif %}{% if true %}b{% endif %}{% if true %}a{% endif %}{% if true %}b{% endif %}{% if true %}a{% endif %}{% if true %}b{% endif %}") + t.resource_limits.render_length_limit = 5 + assert_equal "Liquid error: Memory limits exceeded", t.render() + assert t.resource_limits.reached? + end + def test_default_resource_limits_unaffected_by_render_with_context context = Context.new t = Template.parse("{% for a in (1..100) %} {% assign foo = 1 %} {% endfor %}") From 3080f95a4fb41c07ed1dbc0edc25f868fea2b6b0 Mon Sep 17 00:00:00 2001 From: Justin Li Date: Thu, 11 Dec 2014 10:41:47 -0500 Subject: [PATCH 8/8] Make render_length tests stricter --- test/integration/template_test.rb | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/test/integration/template_test.rb b/test/integration/template_test.rb index d565a9b..e0fc528 100644 --- a/test/integration/template_test.rb +++ b/test/integration/template_test.rb @@ -151,10 +151,25 @@ class TemplateTest < Minitest::Test end def test_render_length_persists_between_blocks + t = Template.parse("{% if true %}aaaa{% endif %}") + t.resource_limits.render_length_limit = 7 + assert_equal "Liquid error: Memory limits exceeded", t.render() + t.resource_limits.render_length_limit = 8 + assert_equal "aaaa", t.render() + + t = Template.parse("{% if true %}aaaa{% endif %}{% if true %}bbb{% endif %}") + t.resource_limits.render_length_limit = 13 + assert_equal "Liquid error: Memory limits exceeded", t.render() + t.resource_limits.render_length_limit = 14 + assert_equal "aaaabbb", t.render() + t = Template.parse("{% if true %}a{% endif %}{% if true %}b{% endif %}{% if true %}a{% endif %}{% if true %}b{% endif %}{% if true %}a{% endif %}{% if true %}b{% endif %}") t.resource_limits.render_length_limit = 5 assert_equal "Liquid error: Memory limits exceeded", t.render() - assert t.resource_limits.reached? + t.resource_limits.render_length_limit = 11 + assert_equal "Liquid error: Memory limits exceeded", t.render() + t.resource_limits.render_length_limit = 12 + assert_equal "ababab", t.render() end def test_default_resource_limits_unaffected_by_render_with_context