Shopify / liquid-c

Liquid performance extension in C.
MIT License
119 stars 24 forks source link

Reset VM stack when an exception occurs in Expression#evaluate #192

Closed dylanahsmith closed 1 year ago

dylanahsmith commented 1 year ago

Problem

Looks like liquid_vm_evaluate (e.g. for Liquid::C::Expression#evaluate) was leaving values on the VM stack when it fails with an exception.

I've included a regression test, that resulted in the following assertion failure when run on CI (https://github.com/Shopify/liquid-c/actions/runs/4576027701/jobs/8079617028):

ruby: ../../../../ext/liquid_c/vm.c:593: liquid_vm_render: Assertion `rescue_args.old_stack_byte_size == c_buffer_size(&vm->stack)' failed.

That regression test only really demonstrates a temporary memory leak, which would go away at the end of rendering. However, I'm more concerned about this leading to the wrong values being popped from the VM stack if this recursive use of liquid_vm_evaluate can happen where values were expected to be left on the stack.

Solution

Similar to liquid_vm_render, I used rb_rescue in liquid_vm_evaluate to cleanup anything remaining on the stack when it fails with an exception.