[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Chicken-hackers] PATCH: allow signal handlers to be called from any
From: |
Jörg F . Wittenberger |
Subject: |
Re: [Chicken-hackers] PATCH: allow signal handlers to be called from any thread. |
Date: |
Wed, 2 Dec 2015 13:52:11 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux armv7l; rv:38.0) Gecko/20100101 Icedove/38.3.0 |
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