Re: [Chicken-hackers] Saver a patch: Re: Release blocker? Stack checks completely FUBAR? Or am I just paranoid?

[Jörg F . Wittenberger]

Hi,

sorry for the disturbance.  I did not expect those to be unwelcome.

I assumed that there was a problem and a fix too, it might be a good thing.

Am 28.02.2016 um 17:49 schrieb Peter Bex:
> On Sun, Feb 28, 2016 at 03:58:11PM +0100, Jörg F. Wittenberger wrote:
>> Hi,
>>
>> the patch I sent is harmful.  I forgot to reset chicken.h to "backward
>> compatible".
>>
>> Attached a better version.
> 
> I'd really appreciate it if you could stop bombarding the mailing list
> with comments and patches.  There's already an accumulated backlog of
> patches that I need to look at, and the issue at hand (stack overflows)
> is actually orthogonal to the idea of reusing argvectors more.

I understood your original message as saying "when allocating an
argvector we might overflow the stack".

So if I miss-understood your statement, then yes, those things are
orthogonal and we should not discuss the argvector changes now.

But if the overflow happens because of an argvector allocation on the
stack - then knowing that we do *not* allocate fresh memory for the
argvector would IMHO solve this. Doesn't it?

> Because these two things are orthogonal I would strongly prefer to fix
> the stack overflow issue with the smallest possible patch and then make
> a release.

That's been the idea of mine too.

> Besides, you're wasting time until this has been fixed because the
> argvector code might get modified a lot, so that means your patches

Originally I did not propose this to go into the current release.  And
the main point of the patch was to make upcoming changes easier by
collecting argvector management into macros.  Plus: using a argvector
with a special structure (I first asserted that those would carry a
fixed tag guard instead of the length) made it much easier for me to debug.

> will probably conflict after the actual bug has been fixed.

I had preferred to discuss in the sort term how to fix the bug and in
the long term how to change the argvector code.  At this point I don't
mind those changes to conflict.  That's what happens during the search
for the best solution.

Now the point wrt. the bug is: which bug?  With the diff I sent
yesterday there is no stack space required for the argvector ever.

Sure the diff is nowhere near minimal.  I could not have made the change
without first sorting out what C_alloc is argvector related and what is
not.  But it contains IMHO the solution to the problem at hand.  The
latter could be ported back to master with a much smaller diff.  That's
what I'd be happy to do and had planned to look into today.  But I would
not bother if you folks don't like the change for some reason.

Now I wonder what I should do.

The diff I'd suggest would introduce one new global C_default_argvector
and C_alloc tempstack space to this address in CHICKEN_run after setting
C_restart.  All cases where the code now uses C_alloc to create an
argvector would have to be changed to return C_default_argvector.  Watch
out for the exception in C_context_switch that is.

Cheers

/Jörg

PS: no diff attached