[Chicken-hackers] Release blocker? Stack checks completely FUBAR? Or am I just paranoid?

[Peter Bex]

Hi all,

Today I noticed this error appearing out of nowhere:
http://salmonella-freebsd-x86-64.call-cc.org/master/clang/freebsd/x86-64/2016/02/25/salmonella-report/test/scsh-process.html
Nothing has changed in scsh-process, and compared to yesterday
I don't think we pushed any changes that could be responsible
for breaking this, as such.  AFAICT the only change is that we
now load lolevel inside the compiler, which shouldn't even affect
tests in csi.

After taking a look at how a "stack overflow" error can get
triggered, there seems to be basically one way: through C, by the
C_stack_overflow() function which only gets called through the
C_stack_overflow_check macro.

I'm a bit ill at the moment, so maybe it's just my fevered brain
talking, but if I understand correctly there are two problems here:

1) Generated CPS functions will allocate what they need on the
   stack for local variables up front, then do a C_stack_probe()
   or a C_demand, but they will allocate an arbitrary amount of
   words for the argvectors *WITHOUT ANY CHECK WHETHER IT WILL FIT*.

2) C_stack_probe() and C_demand() will check if the stack pointer exceeds
   C_stack_limit and trigger a GC.  However, C_stack_overflow_check
   will call C_stack_check1() which adds C_STACK_RESERVE.
   That means that a C_demand() check might say "yeah, sure, it fits in
   the stack", whereas the function that it calls will do a
   C_stack_overflow_check which says "nope, it won't fit in the stack
   minus the stack reserve".

These two probably need to be fixed before we can do a release.
The second was always broken but it might not have been such a big
deal because we never allocated as much on the stack(?).  The first
is new with argvectors.

Like I said, I'm a bit ill so this may all be just a fever dream, but
the proper solution eludes me.  A fix for 2) might involve collecting
all the possible function calls a generated function will perform and
then pre-allocating the av2 (but only when needed), and then checking
whether that would cause the stack to overflow.

A fix for 1), if I'm not mistaken, would be to swap the way the stack
checks are done: the C_demand and C_stack_probe should observe the
reserve, while C_stack_check1() should only look at the hard limit.

Besides, I think C_stack_check1 looks too complicated; why does it
create the temporary *_sp variable?  Is that to avoid a pointer
overflow/underflow?  Couldn't it just do a check like this:

if ((C_byte *)C_stack_pointer < ((C_byte *)C_stack_limit - C_STACK_RESERVE)) 
err;

Cheers,
Peter

signature.asc
Description: Digital signature