Pull request #102 introduced a memory safety regression that can be reproduced using the following script, where valgrind can be used to detect the read after free
require 'liquid/c'
StubFileSystem = Struct.new(partials) do
def read_template_file(template_path)
partials.fetch(template_path)
end
end
Liquid::Template.file_system = StubFileSystem.new(
'invalid' => "{%%}",
'a' => '{% include "b" %}',
'b' => '...',
)
template = Liquid::Template.parse(<<~LIQUID)
{% include 'invalid' %}
{% include 'a' %}
LIQUID
puts template.render
valgrind error
```
Invalid read of size 1
at 0xB7C36E6: vm_render_until_error (vm.c:241)
by 0x138650: rb_vrescue2 (eval.c:990)
by 0x13886D: rb_rescue2 (eval.c:967)
by 0xB7C4375: liquid_vm_render (vm.c:550)
by 0xB7BB045: block_body_render_to_output_buffer (block.c:355)
by 0x2F572F: vm_call_cfunc_with_frame (vm_insnhelper.c:2514)
by 0x2F572F: vm_call_cfunc (vm_insnhelper.c:2539)
by 0x30D3C6: vm_sendish (vm_insnhelper.c:4023)
by 0x30D3C6: vm_exec_core (insns.def:801)
by 0x2FFBC3: rb_vm_exec (vm.c:1920)
by 0x30523D: vm_call0_body (vm_eval.c:136)
by 0x30798C: rb_vm_call0 (vm_eval.c:52)
by 0x30798C: rb_vm_call_kw (vm_eval.c:268)
by 0x30798C: rb_call0 (vm_eval.c:392)
by 0x30855F: rb_call (vm_eval.c:718)
by 0x30855F: rb_funcall (vm_eval.c:942)
by 0xB7C3C87: vm_render_until_error (vm.c:364)
Address 0x9043b31 is 33 bytes inside a block of size 64 free'd
at 0x483DFAF: realloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x15DC3E: objspace_xrealloc (gc.c:9932)
by 0x15DC3E: ruby_sized_xrealloc (gc.c:10133)
by 0x15DC3E: ruby_xrealloc_body (gc.c:10139)
by 0x15DC3E: ruby_xrealloc (gc.c:12014)
by 0xB7BBC7F: c_buffer_expand_for_write (c_buffer.c:14)
by 0xB7BBD6C: c_buffer_reserve_for_write (c_buffer.c:33)
by 0xB7BCF17: c_buffer_extend_for_write (c_buffer.h:49)
by 0xB7BD1B1: document_body_write_block_body (document_body.c:66)
by 0xB7BAF5E: block_body_freeze (block.c:332)
by 0x2F572F: vm_call_cfunc_with_frame (vm_insnhelper.c:2514)
by 0x2F572F: vm_call_cfunc (vm_insnhelper.c:2539)
by 0x30D3C6: vm_sendish (vm_insnhelper.c:4023)
by 0x30D3C6: vm_exec_core (insns.def:801)
by 0x2FFBC3: rb_vm_exec (vm.c:1920)
by 0x30523D: vm_call0_body (vm_eval.c:136)
by 0x30798C: rb_vm_call0 (vm_eval.c:52)
by 0x30798C: rb_vm_call_kw (vm_eval.c:268)
by 0x30798C: rb_call0 (vm_eval.c:392)
Block was alloc'd at
at 0x483DFAF: realloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x15DC3E: objspace_xrealloc (gc.c:9932)
by 0x15DC3E: ruby_sized_xrealloc (gc.c:10133)
by 0x15DC3E: ruby_xrealloc_body (gc.c:10139)
by 0x15DC3E: ruby_xrealloc (gc.c:12014)
by 0xB7BBC7F: c_buffer_expand_for_write (c_buffer.c:14)
by 0xB7BBD6C: c_buffer_reserve_for_write (c_buffer.c:33)
by 0xB7BBD9A: c_buffer_write (c_buffer.c:39)
by 0xB7BCF75: c_buffer_concat (c_buffer.h:73)
by 0xB7BD24C: document_body_write_block_body (document_body.c:76)
by 0xB7BAF5E: block_body_freeze (block.c:332)
by 0x2F572F: vm_call_cfunc_with_frame (vm_insnhelper.c:2514)
by 0x2F572F: vm_call_cfunc (vm_insnhelper.c:2539)
by 0x30D3C6: vm_sendish (vm_insnhelper.c:4023)
by 0x30D3C6: vm_exec_core (insns.def:801)
by 0x2FFBC3: rb_vm_exec (vm.c:1920)
by 0x30523D: vm_call0_body (vm_eval.c:136)
```
What is happening is that the first included partial has a syntax error, which leaves the document body on the parse context. As a result, the following two partials get compiled into the same document body, where the include of the last partial ('b') results in the document body being instruction buffer being reallocated to expand it for the write, meaning the rendering of partial 'a' continues by reading from the freed instructions memory allocation.
Solution
I used an ensure block to cleanup the parse context, so we avoid trying to re-use the document body across templates.
This means we should also have a check to make sure we aren't rendering a block before the document body is finished being compiled, so this PR freezes the document body object and checks for it before rendering it.
I also turned the reproduction script into a test so that there is a test for this code path.
Problem
Pull request #102 introduced a memory safety regression that can be reproduced using the following script, where valgrind can be used to detect the read after free
valgrind error
``` Invalid read of size 1 at 0xB7C36E6: vm_render_until_error (vm.c:241) by 0x138650: rb_vrescue2 (eval.c:990) by 0x13886D: rb_rescue2 (eval.c:967) by 0xB7C4375: liquid_vm_render (vm.c:550) by 0xB7BB045: block_body_render_to_output_buffer (block.c:355) by 0x2F572F: vm_call_cfunc_with_frame (vm_insnhelper.c:2514) by 0x2F572F: vm_call_cfunc (vm_insnhelper.c:2539) by 0x30D3C6: vm_sendish (vm_insnhelper.c:4023) by 0x30D3C6: vm_exec_core (insns.def:801) by 0x2FFBC3: rb_vm_exec (vm.c:1920) by 0x30523D: vm_call0_body (vm_eval.c:136) by 0x30798C: rb_vm_call0 (vm_eval.c:52) by 0x30798C: rb_vm_call_kw (vm_eval.c:268) by 0x30798C: rb_call0 (vm_eval.c:392) by 0x30855F: rb_call (vm_eval.c:718) by 0x30855F: rb_funcall (vm_eval.c:942) by 0xB7C3C87: vm_render_until_error (vm.c:364) Address 0x9043b31 is 33 bytes inside a block of size 64 free'd at 0x483DFAF: realloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) by 0x15DC3E: objspace_xrealloc (gc.c:9932) by 0x15DC3E: ruby_sized_xrealloc (gc.c:10133) by 0x15DC3E: ruby_xrealloc_body (gc.c:10139) by 0x15DC3E: ruby_xrealloc (gc.c:12014) by 0xB7BBC7F: c_buffer_expand_for_write (c_buffer.c:14) by 0xB7BBD6C: c_buffer_reserve_for_write (c_buffer.c:33) by 0xB7BCF17: c_buffer_extend_for_write (c_buffer.h:49) by 0xB7BD1B1: document_body_write_block_body (document_body.c:66) by 0xB7BAF5E: block_body_freeze (block.c:332) by 0x2F572F: vm_call_cfunc_with_frame (vm_insnhelper.c:2514) by 0x2F572F: vm_call_cfunc (vm_insnhelper.c:2539) by 0x30D3C6: vm_sendish (vm_insnhelper.c:4023) by 0x30D3C6: vm_exec_core (insns.def:801) by 0x2FFBC3: rb_vm_exec (vm.c:1920) by 0x30523D: vm_call0_body (vm_eval.c:136) by 0x30798C: rb_vm_call0 (vm_eval.c:52) by 0x30798C: rb_vm_call_kw (vm_eval.c:268) by 0x30798C: rb_call0 (vm_eval.c:392) Block was alloc'd at at 0x483DFAF: realloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) by 0x15DC3E: objspace_xrealloc (gc.c:9932) by 0x15DC3E: ruby_sized_xrealloc (gc.c:10133) by 0x15DC3E: ruby_xrealloc_body (gc.c:10139) by 0x15DC3E: ruby_xrealloc (gc.c:12014) by 0xB7BBC7F: c_buffer_expand_for_write (c_buffer.c:14) by 0xB7BBD6C: c_buffer_reserve_for_write (c_buffer.c:33) by 0xB7BBD9A: c_buffer_write (c_buffer.c:39) by 0xB7BCF75: c_buffer_concat (c_buffer.h:73) by 0xB7BD24C: document_body_write_block_body (document_body.c:76) by 0xB7BAF5E: block_body_freeze (block.c:332) by 0x2F572F: vm_call_cfunc_with_frame (vm_insnhelper.c:2514) by 0x2F572F: vm_call_cfunc (vm_insnhelper.c:2539) by 0x30D3C6: vm_sendish (vm_insnhelper.c:4023) by 0x30D3C6: vm_exec_core (insns.def:801) by 0x2FFBC3: rb_vm_exec (vm.c:1920) by 0x30523D: vm_call0_body (vm_eval.c:136) ```What is happening is that the first included partial has a syntax error, which leaves the document body on the parse context. As a result, the following two partials get compiled into the same document body, where the include of the last partial ('b') results in the document body being instruction buffer being reallocated to expand it for the write, meaning the rendering of partial 'a' continues by reading from the freed instructions memory allocation.
Solution
I used an ensure block to cleanup the parse context, so we avoid trying to re-use the document body across templates.
This means we should also have a check to make sure we aren't rendering a block before the document body is finished being compiled, so this PR freezes the document body object and checks for it before rendering it.
I also turned the reproduction script into a test so that there is a test for this code path.