bug#21380: 25.0.50; GTK-induced segfault when scheduling timer from window-configuration-change-hook

[Pip Cet]

On Wed, Sep 2, 2015 at 3:08 PM, Eli Zaretskii <eliz@gnu.org> wrote:

> Date: Tue, 1 Sep 2015 20:48:18 +0000
> From: Pip Cet <pipcet@gmail.com>
> Cc: 21380@debbugs.gnu.org
>
>     Alternatively, we could turn off atimers (by calling turn_on_atimers)
>     while Fcopy_sequence runs.
>
>
> I think that would be a better solution. I've done a quick grep for the current
> atimers and at first glance they appear to be okay, but obviously that's no
> guarantee for the future. It might be worth thinking about
> block_input_and_atimers ().
>
> I think it's safe to assume that Lisp timers are only checked if atimers are
> enabled.

Those are two completely separate and independent features, so no,
it's not safe to make that assumption.  Not sure why you need to
assume that, though.

So we can call turn_on_atimers (true) without potentially enabling atimers in a critical section.

My assumption was that the reason we have both Lisp timers and atimers is that atimers run strictly more often than Lisp timers.

 

> If it isn't, I think the best way forward is to write
> block_input_and_atimers () and lock atimers with a counter just like input is.

Not sure I follow you.  Are you saying that just calling block_input
followed by turn_on_atimers is somehow not enough to prevent some Lisp
from changing Vtimer_list under our feet?

I'm not saying that, no, but if another function disables atimers, then runs Lisp timers, then does something critical that needs atimers to be disabled, it might break.

> --- a/src/fns.c
> +++ b/src/fns.c
> @@ -744,6 +744,9 @@ concat (ptrdiff_t nargs, Lisp_Object *args,
>           /* Store this element into the result.  */
>           if (toindex < 0)
>             {
> +                if (NILP (tail))
> +                  break;
> +

Is this part still needed?

As far as I know, the other two fixes are sufficient. It's needed in case someone calls copy_sequence on a list that's messed with by code run from a hook from QUIT, and merely succeeds in avoiding a segfault and producing incorrect results instead, so I'm not at all sure it should go in.