Re: [Chicken-hackers] PATCH: allow signal handlers to be called from any thread.

[Jörg F . Wittenberger]

Argh,  this is still no good.

Now the pending_interrupts_count will grow beyond bounds.

Use the attached version instead.

Sorry.

/Jörg



Am 02.12.2015 um 13:28 schrieb Jörg F. Wittenberger:
> Hi all,
> 
> I guess I must apologize for first posting an almost good patch and then
> once I found this cut&paste error, I became confused and made a mess out
> of it.
> 
> Am 30.11.2015 um 12:44 schrieb Jörg F. Wittenberger:
>> Am 29.11.2015 um 17:55 schrieb Jörg F. Wittenberger:
>>> Am 26.11.2015 um 11:29 schrieb Jörg F. Wittenberger:
>>> The signal handler in the other thread will happily set the global
>>> variable C_stack_limit to point 1000 word off the stack pointer at the
>>> other thread.
>>
>> Perhaps the attached patch is the smallest to avoid both the problems: a
>> increasing leak on C_stack_limit if the signals arrive at the chicken
>> thread before being handled and the confusion caused when signals are
>> dispatched to another threads.
>>
>> Does two things:
>>
>> a) Don't overwrite saved_stack_limit multiple times.
> 
> At the first glance the code as is seems to not try to do this.  However
> I inserted a trace print like this
> 
>       fprintf(stderr, "Reg signal new limit %p, saved %p\n",
> C_stack_limit, saved_stack_limit);
> 
> into C_raise_interrupt just after C_stack_limit is adjusted and this
> shows clearly that it happens:
> 
> With the current code this results in:
> 
> Reg signal new limit 0xbecebab8, saved 0xbecc0198
> ...
> Reg signal new limit 0xbece8b30, saved 0xbecc0198
> Reg signal new limit 0xbecc25d8, saved 0xbecc0198
> fusermount: entry for /home/u/Askemos not found in /etc/mtab
> Reg signal new limit 0x40df8958, saved 0xbecc0198
> Segmentation fault
> 
> 
> This is where the signal is dispatched to the other thread.  Hence the
> weird new limit.  Given this limit I'd expect no garbage collection for
> a dangerous long time.
> 
> To combat the good part of the patch, coming back to the multiple
> assignment later (as we have not yet seen it).
> 
>> b) Use a known global value `stack_bottom` as the temporary value for
>> C_stack_limit to trigger garbage collection and interrupt dispatch.
> 
>      C_stack_limit = stack_bottom;
> 
> Instead of the C_stack_pointer arithmetic.  All this assignment want to
> achieve is make the next C_stack_probe fail, correct?  Hence the 1000
> offset seem to be risky in the first place.  If stack_bottom was at the
> last/first page of the virtual memory and a signal is dispatched just
> after garbage collection (when less the 1000 words of the stack are
> used), then the calculation should overflow.
> 
> Assigning stack_bottom should trigger the the stack probe to fail in any
> case.
> 
> Now back to the multiple overwrite of saved_stack_limit.  Using the
> modified code we see that the new limit is fixed as it should.  However
> at a point we observe that saved_stack_limit is written over:
> 
> Reg signal new limit 0xbeb1f190, saved 0xbeadf198
> ...
> Reg signal new limit 0xbeb1f190, saved 0xbeadf198
> Reg signal new limit 0xbeb1f190, saved 0xbeb1f190
> Reg signal new limit 0xbeb1f190, saved 0xbeb1f190
> Reg signal new limit 0xbeb1f190, saved 0xbeb1f190
> 
> which results in an endless loop around gc.
> 
> This means we need to do something to protect against re-entry of the
> first branch (pending_interrupts_count == 0 && !handling_interrupts) in
> C_raise_interrupt from signals dispatched in C_cpu_milliseconds, doesn't it?
> 
> However there should be a better way than tracking saved_stack_limit all
> over the place.  It is much simpler to pull the increment of
> pending_interrupts_count before the call to C_cpu_milliseconds.
> 
> The attached patch does just that.
> 
> However this may still have a race condition.  It assumes that
> 
>   c = pending_interrupts_count++;
> 
> will atomically increment the counter.
> 
> I first tried to simply re-order the code as below.  (Separate test and
> increment.)  This *did* still write over saved_stack_limit.  Thus we
> would actually need an documented-to-be-atomic increment on
> pending_interrupts_count or not have a system call in the interrupt
> handler at all.
> 
> 
> Best
> 
> /Jörg
> 
> 
> C_regparm void C_fcall C_raise_interrupt(int reason)
> {
>   if(C_interrupts_enabled) {
>     if(pending_interrupts_count == 0 && !handling_interrupts) {
>      /* Immediately increment pending_interrupts_count to ensure
>        signals dispatched in C_cpu_milliseconds take the alternative
> path. NOTE: This factually FAILED; this path was observed to be entered
> multiple times.*/
>       pending_interrupts[ pending_interrupts_count++ ] = reason;
> 
>       /* Force the next stack check to fail by faking a "full" stack.
>          That causes save_and_reclaim() to be called, which will
>          invoke handle_interrupt() (which restores the stack limit). */
>       saved_stack_limit = C_stack_limit;
>       C_stack_limit = stack_bottom;
>       interrupt_time = C_cpu_milliseconds();
>     } else if(pending_interrupts_count < MAX_PENDING_INTERRUPTS) {
>       int i;
>       /*
>        * Drop signals if too many, but don't queue up multiple entries
>        * for the same signal.
>        */
>       for (i = 0; i < pending_interrupts_count; ++i) {
>         if (pending_interrupts[i] == reason)
>           return;
>       }
>       pending_interrupts[ pending_interrupts_count++ ] = reason;
>     }
>   }
> }
> 
> 
> 
> _______________________________________________
> Chicken-hackers mailing list
> address@hidden
> https://lists.nongnu.org/mailman/listinfo/chicken-hackers
>

0004-allowsignalhandlerstobecalledfromanythread.patch
Description: Text Data