bug#21394: 25.0.50; Segfault when displaying unprintable character in echo area while frames are being created

[Pip Cet <pipcet@gmail.com>]

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

This code in xdisp.c looks very suspicious to me:

------
static int last_escape_glyph_face_id = (1 << FACE_ID_BITS);
static int last_escape_glyph_merged_face_id = 0;

static int
merge_escape_glyph_face (struct it *it)
{
  int face_id;

  if (it->f == last_escape_glyph_frame
      && it->face_id == last_escape_glyph_face_id)
    face_id = last_escape_glyph_merged_face_id;
  else
    {
      /* Merge the `escape-glyph' face into the current face.  */
      face_id = merge_faces (it->f, Qescape_glyph, 0, it->face_id);
      last_escape_glyph_frame = it->f;
      last_escape_glyph_face_id = it->face_id;
      last_escape_glyph_merged_face_id = face_id;
    }
  return face_id;
}
------

That caches a face id in last_escape_glyph_merged_face_id, which is cleared
only in redisplay_internal(). But message() doesn't call
redisplay_internal(), it calls try_window() directly (xdisp.c:10687) (and
resize_window before that, which blows up).

This patch appears, so far, to run without a segfault (I neglected to
record timings so can't give you a p-value, but back of the envelope it's
>99% that it fixes the segfault), and it fixes what I think is a possible
explanation for the segfault, so my current plan is to leave it running
overnight and ask for inclusion (of this or an equivalent, cleaner patch,
of course) if it doesn't blow up.

However, I have yet to look at the other bugs and doubt this is the whole
story.


On Tue, Sep 1, 2015 at 7:37 PM, Eli Zaretskii <eliz@gnu.org> wrote:

> > Date: Tue, 1 Sep 2015 18:52:44 +0000
> > From: Pip Cet <pipcet@gmail.com>
> > Cc: 21394@debbugs.gnu.org
> >
> >     I see the problematic face always has face ID of 18, and the 'used'
> >     field is always 15 when the segfault strikes. So I guess the next
> >     step is to make the breakpoint in cache_face conditional on i being
> >     18,
> >
> > i is used in two different ways in that function, as a face hash and as
> an
> > index into faces_by_id. I assume you mean the latter?
>
> Yes.
>
> >     and then see whether c->used is set to 19 during that call to
> >     cache_face. If it does, then a watchpoint (by location) on c->used
> >     should show which code makes the value smaller.
> >
> > So I wrote a perl script to set a watchpoint on c->used whenever we
> allocate a
> > new face cache c in make_face_cache, and clear the watchpoint when we hit
> > free_face_cache. Output attached, but do let me know what else you would
> like
> > watched. I think that has all the information your approach would have
> given
> > us.
>
> Sounds like the face ID of 18 is "remembered" somewhere, and not
> "forgotten" when the face cache is free'd?
>

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

[-- Attachment #2: 0001-Forget-cached-face-ids-when-displaying-echo-area-mes.patch --]
[-- Type: text/x-patch, Size: 1177 bytes --]

From 04d6faa3146e585a3e37a801954ad25b5d7e1792 Mon Sep 17 00:00:00 2001
From: Philip <pipcet@gmail.com>
Date: Tue, 1 Sep 2015 20:10:06 +0000
Subject: [PATCH] Forget cached face ids when displaying echo area messages
 (Bug#21394)

---
 src/xdisp.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/xdisp.c b/src/xdisp.c
index 9ff9f6c..86b7ea2 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -10676,7 +10676,14 @@ display_echo_area_1 (ptrdiff_t a1, Lisp_Object a2)
   /* Do this before displaying, so that we have a large enough glyph
      matrix for the display.  If we can't get enough space for the
      whole text, display the last N lines.  That works by setting w->start.  */
-  bool window_height_changed_p = resize_mini_window (w, false);
+  bool window_height_changed_p;
+
+  last_escape_glyph_frame = NULL;
+  last_escape_glyph_face_id = (1 << FACE_ID_BITS);
+  last_glyphless_glyph_frame = NULL;
+  last_glyphless_glyph_face_id = (1 << FACE_ID_BITS);
+
+  window_height_changed_p = resize_mini_window (w, false);
 
   /* Use the starting position chosen by resize_mini_window.  */
   SET_TEXT_POS_FROM_MARKER (start, w->start);
-- 
2.5.0