Fix render length resource limit so it doesn't multiply nested output

[dylanahsmith]

Problem

Liquid::ResourceLimits#render_length_limit seems like it would conceptually just be a limit on the size of the output buffer. However, it was actually counting rendering inside blocks multiple times based on how many times they are nested.

For example, the liquid template foo would be counted as render_length 3, but {% if true %}foo{% endif %} would be counted as render_length 6 and {% if true %}{% if true %}foo{% endif %}{% endif %} would count as 9.

This makes the limit hard to understand, hard to enforce and would be more complex to implement in a BlockBody#render implementation in liquid-c where it could be enforced when expanding the output buffer capacity before writing to it.

Solution

Stop tracking the render_length in the Liquid::ResourceLimits and just use output.bytesize to get the current render length instead.

[pushrax]

I think the original intent here was to count all generated string data, including captured content that's not included in the result. There's definitely a bug here with multiple-counting, but this fix seems like it would allow for cases that exceed the render length when rendering to a temporary buffer.

Maybe it's fine though and the render score limit is good enough?

[dylanahsmith]

I think the original intent here was to count all generated string data, including captured content that's not included in the result.

That seems like the intention of the assign score. Conceptually, I would like the assign score to be thought of as a maximum memory pool capacity if we did all our memory allocations. I think its main purpose is to limit the growth of the heap.

Maybe it's fine though and the render score limit is good enough?

The render score seems like a very inaccurate way of limiting the amount of compute work done in a deterministic way. However, it counts all nodes equally, so isn't very accurate right now.

What we end up restoring to in order to limit the amount of compute work is to use timeouts. That is something that I would like builtin support for in liquid/liquid-c so that we avoid the temptation of using Timeout.timeout or monkey patching.

So the ideal purpose for a render score would be one that could be computed based on static analysis if we had some type information and upper bounds on input collection/array/enumerator sizes. However, I don't have any plans to work on that in the foreseeable future.

[pushrax]

That seems like the intention of the assign score.

There are cases where you can defeat this, but it might run into stack limits. e.g.

recurse.liquid:

{% capture whatever %}
do something that renders just below the maximum render length
{% include 'recurse' %}
{% endcapture %}

This would allow rendering multiples of the render length limit before crashing. This would not be possible before this PR.

Timeouts are definitely going to protect against catastrophic cases though. If the intent is to change the definition of the render length limit to mean something more like a maximum response size, this PR makes sense.

I think a good option would be to sum the size of everything appended to any buffer during the render, though it's not clear to me how to implement that cleanly given we don't have a buffer class.

[dylanahsmith]

If the intent is to change the definition of the render length limit to mean something more like a maximum response size, this PR makes sense.

Yes, that is the intention.

However, you example does show that this would cause a regression in what I would like the assign_score to prevent, even though it was just doing it indirectly through the render_length.

I think a good option would be to sum the size of everything appended to any buffer during the render, though it's not clear to me how to implement that cleanly given we don't have a buffer class.

I've pushed a commit that tries to do this cleanly by adding a @last_capture_length value to ResourceLimits so that it knows whether to check the render limit or the increment the assign score and knows how much to increment the assign score.