search

Shopify / liquid-c

Liquid performance extension in C.
MIT License
120 stars 25 forks source link

Copy filter arguments to stack #191

Closed peterzhu2118 closed 1 year ago

peterzhu2118 commented 1 year ago

There's a bug in 50f65e42e960d7bb47d6ba4711d88bdf9280425a. The stack of the liquid VM does get modified by the filter, so it's not safe to call vm_stack_pop_n after invoking the filter.

casperisfine commented 1 year ago

@dylanahsmith it's unclear to us as well as to how the stack get modified exactly, but we were able to reproduce it in the liquid-c test suite.

Perhaps, you'd be able to figure out what modify it? @peterzhu2118 can you share the branch with the modification check?

peterzhu2118 commented 1 year ago

I moved popping the arguments to vm_invoke_filter instead of in vm_render_until_error because memory allocated by alloca is not freed until the function returns, which means that it could potentially cause a stack overflow if it's in vm_render_until_error. I can't reproduce a stack overflow on my machine because it takes too long to execute before a stack overflow can occur, but I can see that alloca does not reuse memory that's gone out of scope. Better safer than sorry.