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

[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;
    }
  }
}

0003-allowsignalhandlerstobecalledfromanythread.patch
Description: Text Data