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

[Pip Cet <pipcet@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4740 bytes --]

No segfault*.

(* - well, one segfault. But I attribute that to extraordinarily bizarre
actions even by my standards: attempting to display an unprintable ASCII
control character in the echo area. It seems we never make sure that
non-standard faces are cached when redisplaying the echo area. Usually this
is fine because propertized strings never end up in the echo area (I
hope)...)

Revised patch attached (use NILP, fix bug number in changelog entry).


On Tue, Sep 1, 2015 at 10:20 AM, Pip Cet <pipcet@gmail.com> wrote:

> On Mon, Aug 31, 2015 at 2:31 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>
>> > Yes for this particular segfault.
>>
>> Can you show a patch that fixes the original segfault in your use
>> case?
>
>
> Attached. Note that either one of those changes should work. I'll test
> this patch some more using my original code and see whether it blows up.
>
> I'm afraid I lost track, what with all the different scenarios
>> and potential solutions being thrown at this.  We should install the
>> fix, assuming it's clean.
>>
>
> I think we should fix three things:
>  - concat shouldn't rely on its argument remaining unchanged in length
>  - the timer list copy should happen with block_input/unblock_input
> wrapped around it
>  - we shouldn't call do_pending_window_change from QUIT [already
> installed. Thanks, martin!]
>
> Any one of these is enough to prevent the original segfault. All but the
> second also prevent the bizarre-elisp-induced segfault I came up with
> later. And I'm perfectly happy for today with the number of hooks called
> from QUIT reduced by one, rather than insisting on reducing them to zero
> right away.
>
> > No* for similar segfaults that I think pose equally severe problems: if
>> any other function calls concat/copy-sequence on data that is modified by
>> window-configuration-change-hook, it should* still be possible to produce
>> the segfault.
>>
>> Emacs gives you enough rope to hang yourself; there's nothing new
>> here.  We should strive to protect the Emacs internals so that they
>> won't cause segfaults, but in user code any bets are off, and "don't
>> do that" is a valid response to whoever does such things.
>>
>
> It's always good to know what the philosophy is behind the way the code
> works, so thank you for that, really.
>
> > So it wouldn't even be safe for window-configuration-change-hook to add
>> a timer to the timer list, because the outer frame might be in the middle
>> of creating a copy of the timer list for some Lisp code that hasn't blocked
>> input. (As in my example below)
>>
>> Futzing with timers from within some hooks is indeed fundamentally
>> dangerous.
>
>
> Well, doing anything from window-configuration-change-hook is dangerous.
> My idea was to schedule an immediate timer from it to get out of the danger
> zone to do the actual work, but that backfired...
>
>
>> But we should still try to minimize the probability of a
>> crash, especially when it's Emacs itself who makes the offending copy,
>> because people do dangerous things all the time, and expect them to
>> work.  In this case, blocking input should do, I think.
>>
>
> I agree.
>
> > I really don't think QUIT should run any Lisp hooks, to be honest.
>>
>> I don't think this limitation could fly.  It will disable a lot of
>> useful use patterns, and the outcry will be loud and clear.
>>
>
> Okay.
>
> > If I'm wrong and QUIT should be able to run Lisp hooks, concat needs to
>> be fixed not to rely on its argument's size being unchanged after the
>> make_sequence call.
>>
>> That can't do any harm, so let's do it, too.
>>
>
> Cool.
>
>
>> > As far as I can tell, that should be reproducible. Also as far as I can
>> tell, it's merely a matter of luck that an X resize doesn't happen at the
>> point where I interrupted the program to artificially trigger the segfault.
>> However, I admit that it is a separate issue, less likely to occur in
>> practice, and I'll open another bug for it if that's the way you prefer
>> things.
>>
>> But if input is blocked, as it would be in the case of copying
>> timer-list inside timer_check, the X events will not be acted upon,
>> and the problem will not happen, right?
>>
>
> Indeed, that relies on bizarre elisp code deliberately doing silly
> things...
>
>
>> IOW, the above situation is a case of a user shooting herself in the
>> foot by having that particular function in the hook and that
>> particular code that copies timer-list (which is an internal variable
>> unwise users should not touch).  Am I right?
>>
>
> I think you are. I'm not sure whether the timer code in timer.el does
> anything to the timer list that might count as dangerous, but that's
> possibly the only legitimate Lisp user of timer-list.
>

[-- Attachment #1.2: Type: text/html, Size: 7133 bytes --]

[-- Attachment #2: 0001-Fix-potential-race-conditions-Bug-21380.patch --]
[-- Type: text/x-patch, Size: 1376 bytes --]

From 7dc8c9d7a4997781c04001371ac384db24c37e59 Mon Sep 17 00:00:00 2001
From: Philip <pipcet@gmail.com>
Date: Tue, 1 Sep 2015 10:12:31 +0000
Subject: [PATCH] Fix potential race conditions (Bug#21380)

        * keyboard.c (timer_check): Call `block_input' around the creation
	of the temporary timer list copy.

	* fns.c (concat): Don't assume argument size remains unchanged
	after call to `Fmake_list'.
---
 src/fns.c      | 3 +++
 src/keyboard.c | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/src/fns.c b/src/fns.c
index 26a98ab..15d9e64 100644
--- 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;
+
 		XSETCAR (tail, elt);
 		prev = tail;
 		tail = XCDR (tail);
diff --git a/src/keyboard.c b/src/keyboard.c
index dab32b1..4b7d675 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -4560,6 +4560,7 @@ timer_check (void)
 
   Lisp_Object tem = Vinhibit_quit;
   Vinhibit_quit = Qt;
+  block_input ();
 
   /* We use copies of the timers' lists to allow a timer to add itself
      again, without locking up Emacs if the newly added timer is
@@ -4573,6 +4574,7 @@ timer_check (void)
   else
     idle_timers = Qnil;
 
+  unblock_input ();
   Vinhibit_quit = tem;
 
   do
-- 
2.5.0