bug#26097: Segmentation fault due to missing faces in face_cache.

bug#26097: Segmentation fault due to missing faces in face_cache.

[Codrut Gusoi]

[-- Attachment #1: Type: text/plain, Size: 7869 bytes --]

Hello,

This is a bit weird to describe so please bear with me.

I am using emacs for my day to day editing, and using Spacemacs
(http://spacemacs.org/) as a bundle of predefined configurations. Some
times, while opening files trough helm I would get a segfault. I took
a look in the coredump and also attached gdb to emacs and this is the
backtrace:
```
#0  0x00000000004ab2e0 in turn_on_face (f=0xc1c680, face_id=33) at term.c:1895
#1  0x00000000004af432 in tty_write_glyphs (f=<optimized out>,
string=<optimized out>, len=<optimized out>) at term.c:767
#2  0x000000000041c2ea in update_frame_line (f=f@entry=0xc1c680,
vpos=vpos@entry=29) at dispnew.c:4838
#3  0x000000000041f9e6 in update_frame_1 (f=f@entry=0xc1c680,
force_p=force_p@entry=true, inhibit_id_p=<optimized out>,
    inhibit_id_p@entry=false, set_cursor_p=set_cursor_p@entry=true) at
dispnew.c:4540
#4  0x00000000004210fe in update_frame (f=f@entry=0xc1c680,
force_p=<optimized out>, force_p@entry=false,
inhibit_hairy_id_p=inhibit_hairy_id_p@entry=false)
    at dispnew.c:3122
#5  0x0000000000452ebc in redisplay_internal () at xdisp.c:14085
#6  0x0000000000454ad5 in redisplay () at xdisp.c:13255
#7  0x00000000004f3d1b in read_char (commandflag=commandflag@entry=1,
map=map@entry=28177075, prev_event=0,
used_mouse_menu=used_mouse_menu@entry=0x7fffe9ed04db,
end_time=end_time@entry=0x0) at keyboard.c:2482
#8  0x00000000004f66f0 in read_key_sequence
(keybuf=keybuf@entry=0x7fffe9ed05b0, prompt=prompt@entry=0,
dont_downcase_last=dont_downcase_last@entry=false,
can_return_switch_frame=can_return_switch_frame@entry=true,
fix_current_buffer=fix_current_buffer@entry=true,
prevent_redisplay=prevent_redisplay@entry=false, bufsize=30)
    at keyboard.c:9068
#9  0x00000000004f8216 in command_loop_1 () at keyboard.c:1370
#10 0x0000000000559742 in internal_condition_case
(bfun=bfun@entry=0x4f8020 <command_loop_1>,
handlers=handlers@entry=19104, hfun=hfun@entry=0x4eece0 <cmd_error>)
    at eval.c:1315
#11 0x00000000004ea3fc in command_loop_2 (ignore=ignore@entry=0) at
keyboard.c:1112
#12 0x00000000005596e3 in internal_catch (tag=tag@entry=45936,
func=func@entry=0x4ea3e0 <command_loop_2>, arg=arg@entry=0) at
eval.c:1080
#13 0x00000000004ea3b9 in command_loop () at keyboard.c:1091
#14 0x00000000004ee8e7 in recursive_edit_1 () at keyboard.c:697
#15 0x00000000004eec28 in Frecursive_edit () at keyboard.c:768
#16 0x00000000004182bb in main (argc=3, argv=0x7fffe9ed0908) at emacs.c:1629
```

This led me to the term.c file at 1895 (emacs 25.1.90), inside the
turn_on_face function.
Notice face_id=33 on frame 0 args. That was equal to
f->face_cache->used which means FACE_FROM_ID returns NULL and causes
the segfault in turn_on_face.

I tried looking around to understand why this was happening but I did
not uncover much.
Then I tried finding a fix for the problem at hand, more exactly what
to do when FACE_FROM_ID returns a NULL and how to deal with it so that
it would no longer segfault. Here is what I came up with:
* I could simply return; from turn_on_face (and the complementary
turn_off_face) function on NULL. From what I understand this means
leave text on the screen as is, don't redraw. This is inside the
attached 001.patch
* I could return the defualt face from FACE_FROM_ID. This would mean
text would be displayed on screen with an unexpected default face, but
at least it would not crash. The fix is inside the attached 002.patch
(emacs 25) and 004.patch (emacs 26).
* I could get the default face directly in the turn_on_face function
in case of NULL, in case modifying the behaviour of FACE_FROM_ID is
unwanted. This is inside the attached 003.patch.

Now let me explain how exactly you can reproduce the bug:

* First you will need a linux environment. This bug should be
reproducible on any linux. You could do it on your own linux or you
could get my current setup: VirtualBox and
https://manjaro.org/get-manjaro/ xfce.
* Then you will need emacs and xclip installed. You can install them
from your package manager or from source, I've reproduced the bug with
the following versions: 25.1.1, 25.1.90 and when building master from
source.
* Open a terminal and either make it fullscreen or maximized.
* To install spacemacs you first need to remove ~/.emacs.d and
~/.emacs and then execute the following commands:

git clone https://github.com/syl20bnr/spacemacs ~/.emacs.d
curl -fSLo ~/.spacemacs
https://raw.githubusercontent.com/sdwolf/dotfiles/master/.spacemacs

* Now you must start emacs inside a relatively large git repository. I
have chosen emacs's repo for this.

cd emacs
emacs -nw .

* You now have to wait untill emacs gets all the packages. when this
is done you will have exactly the same setup as I have on all my
machines.
* You have to stop emacs and start it again. You can either do C-x C-c
or :qa! since it used evil-mode (vim style).
* When you have it started again you should see 2 thigs. First on the
left neotree opened with the contents of the current directlry, which
should be the contents of the emacs repo. On the right would be dired
with the same content.
* From this place you need to press <spacebar> p f (that is spacebar
followed by 'p' followed by 'f' on separate key strokes, do not hold
down the keys, just press). This will bring up helm in the lower part
of the screen. The idea is to open a relatively large file so that
emacs has a lot of work to do and potentially trigger the segfault.
The cursor will be on helm's search so you can type "src/term.c"
(without the quotes) then press return (RET). If this does not crash
emacs, you will have to move to a relatively large line number, ex
1800 and close emacs. This means that next time you open this file,
emacs will open it at that line number. To move to that line number
just type ":1800" and pres return (RET). To close emacs from here you
can type ":qa!" and press return (RET).
* You will have to open emacs again and do the above step again. This
manages to trigger the segfault every time in my case. Note that it
should take a relatively long time to open this file (6--10 seconds),
so please wait patiently.

Some things i've observed while digging with gdb:
- Before opening helm in the setup above there are 28 faces in the face_cache.
- After I open helm the cache fills with around 48 faces.
- When i hit return after selecting a file, the cache fills up with
more faces, until it gets to displaying the frame corresponding to the
file (buffer). Then the face_cache drops to 33 (f->used == 33).
- When it gets to the line where it must redisplay helm (vpos == 29 on
dgb frame 2 above), it wants to redisplay the faces for helm but they
are no longer in the cache, so FACE_FROM_ID returns NULL and a crash
happens when the code dereferences the porinter a line below.
- Since helm dissapears a few moments later, it's no problem if you
just skip over this with a return or if you use a default face.
- On emacs 25 with any of my patches applied, for a fraction of a
second after pressing return and helm closing, you can still see the
lower left corner of the screen (where the neotree area resides) still
having the contents of helm. But that part is imediatly redrawn with
the content of neotree. On emacs 26 that part does not get redrawn,
untill you do an action like moving up/down one page or switch to
neotree. This might be an inconvenience but in my opinion it's better
than having a segfault.

Hopefully I've managed to describe the problem (and potentially the
patches I've attached are helpfull). Feel free to ask if you need
further clarifications. Right now I am using my patched version of
emacs to do my work and it would be great if a fix or workaround for
this would be available in the default versions.

For reference, here is an issue on github direclty related to this
problem: https://github.com/syl20bnr/spacemacs/issues/8197

Thank you,
Codrut

[-- Attachment #2: 001.patch --]
[-- Type: text/x-patch, Size: 1107 bytes --]

From 11bceb610280688d82960eb34cfd3cb633fd83fe Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Codru=C8=9B=20Constantin=20Gu=C8=99oi?=
 <codrut.gusoi@gmail.com>
Date: Sat, 11 Mar 2017 20:12:33 +0200
Subject: [PATCH] Fixes segfaults caused by missing faces

---
 src/term.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/term.c b/src/term.c
index 4397210965..47ac19e89f 100644
--- a/src/term.c
+++ b/src/term.c
@@ -1892,6 +1892,10 @@ static void
 turn_on_face (struct frame *f, int face_id)
 {
   struct face *face = FACE_FROM_ID (f, face_id);
+
+  /* Guards agains NULL returned from the `FACE_FROM_ID` macro. */
+  if (face == NULL) return;
+
   unsigned long fg = face->foreground;
   unsigned long bg = face->background;
   struct tty_display_info *tty = FRAME_TTY (f);
@@ -1952,6 +1956,10 @@ static void
 turn_off_face (struct frame *f, int face_id)
 {
   struct face *face = FACE_FROM_ID (f, face_id);
+
+  /* Guards agains NULL returned from the `FACE_FROM_ID` macro. */
+  if (face == NULL) return;
+
   struct tty_display_info *tty = FRAME_TTY (f);
 
   eassert (face != NULL);
-- 
2.12.0


[-- Attachment #3: 002.patch --]
[-- Type: text/x-patch, Size: 1244 bytes --]

From c452fd02995d22899b2fd1717fab2b6ae4f69cd6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Codru=C8=9B=20Constantin=20Gu=C8=99oi?=
 <codrut.gusoi@gmail.com>
Date: Mon, 13 Mar 2017 19:04:18 +0200
Subject: [PATCH] Fixes segfaults caused by missing faces

---
 src/dispextern.h | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index 7b9ae78dcd..0df778db2f 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -1812,13 +1812,14 @@ struct face_cache
   bool_bf menu_face_changed_p : 1;
 };
 
-/* Return a pointer to the face with ID on frame F, or null if such a
-   face doesn't exist.  */
-
-#define FACE_FROM_ID(F, ID)				\
-     (UNSIGNED_CMP (ID, <, FRAME_FACE_CACHE (F)->used)	\
-      ? FRAME_FACE_CACHE (F)->faces_by_id[ID]		\
-      : NULL)
+/* Return a pointer to the face with ID on frame F. In case the face
+   ID is greater that the number of elements in the face cache, it
+   uses a default face ID. */
+
+#define FACE_FROM_ID(F, ID)				    \
+     (UNSIGNED_CMP (ID, <, FRAME_FACE_CACHE (F)->used)	    \
+      ? FRAME_FACE_CACHE (F)->faces_by_id[ID]		    \
+      : FRAME_FACE_CACHE (F)->faces_by_id[DEFAULT_FACE_ID])
 
 #ifdef HAVE_WINDOW_SYSTEM
 
-- 
2.12.0


[-- Attachment #4: 003.patch --]
[-- Type: text/x-patch, Size: 1275 bytes --]

From 40a854a7d92da54b568745f8f6d5c2cebfa916bb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Codru=C8=9B=20Constantin=20Gu=C8=99oi?=
 <codrut.gusoi@gmail.com>
Date: Mon, 13 Mar 2017 19:17:07 +0200
Subject: [PATCH] Fixes segfaults caused by missing faces

---
 src/term.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/src/term.c b/src/term.c
index 4397210965..fe3df9dac7 100644
--- a/src/term.c
+++ b/src/term.c
@@ -1892,6 +1892,12 @@ static void
 turn_on_face (struct frame *f, int face_id)
 {
   struct face *face = FACE_FROM_ID (f, face_id);
+
+  /* The face might have been removed from the cache, using
+     the default face to prevent a segfault. */
+  if (face == NULL)
+    face = FACE_FROM_ID (f, DEFAULT_FACE_ID);
+
   unsigned long fg = face->foreground;
   unsigned long bg = face->background;
   struct tty_display_info *tty = FRAME_TTY (f);
@@ -1952,6 +1958,12 @@ static void
 turn_off_face (struct frame *f, int face_id)
 {
   struct face *face = FACE_FROM_ID (f, face_id);
+
+  /* The face might have been removed from the cache, using
+     the default face to prevent a segfault. */
+  if (face == NULL)
+    face = FACE_FROM_ID (f, DEFAULT_FACE_ID);
+
   struct tty_display_info *tty = FRAME_TTY (f);
 
   eassert (face != NULL);
-- 
2.12.0


[-- Attachment #5: 004.patch --]
[-- Type: text/x-patch, Size: 1314 bytes --]

From 48f9557c9e5e709e02f9ec538401bddb69dc2b24 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Codru=C8=9B=20Constantin=20Gu=C8=99oi?=
 <codrut.gusoi@gmail.com>
Date: Mon, 13 Mar 2017 19:24:48 +0200
Subject: [PATCH] Fixes segfaults caused by missing faces

---
 src/dispextern.h | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index 679820d506..cbf2e857d8 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -1812,11 +1812,14 @@ struct face_cache
   bool_bf menu_face_changed_p : 1;
 };
 
-/* Return a non-null pointer to the cached face with ID on frame F.  */
-
-#define FACE_FROM_ID(F, ID)					\
-  (eassert (UNSIGNED_CMP (ID, <, FRAME_FACE_CACHE (F)->used)),	\
-   FRAME_FACE_CACHE (F)->faces_by_id[ID])
+/* Return a non-null pointer to the cached face with ID on frame F.
+   In case the face ID is greater that the number of elements in the
+   face cache, it uses a default face ID. */
+
+#define FACE_FROM_ID(F, ID)                               \
+  (UNSIGNED_CMP (ID, <, FRAME_FACE_CACHE (F)->used)       \
+   ? FRAME_FACE_CACHE (F)->faces_by_id[ID]               \
+   : FRAME_FACE_CACHE (F)->faces_by_id[DEFAULT_FACE_ID])
 
 /* Return a pointer to the face with ID on frame F, or null if such a
    face doesn't exist.  */
-- 
2.12.0

bug#26097: Segmentation fault due to missing faces in face_cache.

[Eli Zaretskii]

> From: Codrut Gusoi <codrut.gusoi@gmail.com>
> Date: Tue, 14 Mar 2017 15:49:49 +0200
> 
> This is a bit weird to describe so please bear with me.

Thanks for your investigations.

This should not happen: the display engine attempts to prevent faces
from being released while it works on redrawing a frame.  So something
doesn't quite work as planned here.

However, the patches you propose don't really solve the problem, they
are just band-aids.  I'd like to try to find the real reason for the
problem and fix it.

> - Before opening helm in the setup above there are 28 faces in the face_cache.
> - After I open helm the cache fills with around 48 faces.
> - When i hit return after selecting a file, the cache fills up with
> more faces, until it gets to displaying the frame corresponding to the
> file (buffer). Then the face_cache drops to 33 (f->used == 33).

Can you show the backtrace from where the face use count drops?  One
way of doing that is to put a conditional watchpoint on the cache's
'used' count with the condition that it becomes smaller (or maybe
zero).  Make sure you use "watch -l" to avoid automatic deletion of
the watchpoint when some variable goes out of scope.

> - When it gets to the line where it must redisplay helm (vpos == 29 on
> dgb frame 2 above), it wants to redisplay the faces for helm but they
> are no longer in the cache, so FACE_FROM_ID returns NULL and a crash
> happens when the code dereferences the porinter a line below.

The display engine tries to prevent this by setting
inhibit_free_realized_faces to a non-zero value, but I guess something
doesn't quite work in that regard.  I wonder what that is.

Thanks.

bug#26097: Segmentation fault due to missing faces in face_cache.

[Eli Zaretskii]

[Please keep the bug address on the CC list.]

> From: Codrut Gusoi <codrut.gusoi@gmail.com>
> Date: Mon, 20 Mar 2017 14:16:29 +0200
> 
> I've tried watching the `used` count with `watch -l` and there is no
> place where the font cache shrinks for the current frame. I believe
> what I experineced previously was the `used` count from different
> frames (since I added a breakpoint but did not look at the frame
> address).

For the bug you describe to happen, the 'used' count of a frame's face
cache _must_ be reset to zero at some point.  There simply isn't any
other way to "lose" faces from the cache.  So if you don't see that,
it means you didn't succeed to reproduce the problem.

> Have you managed to reproduce it locally?

No.

> If not, what do you say I set up a VM in AWS and give you the IP and
> login details to take a look.

That could work if there's a more-or-less reliable recipe for
reproducing the problem.  Is there?

If there is not recipe, I won't be able to investigate the path to
disaster, and when the segfault happens, it's already too late.

bug#26097: Segmentation fault due to missing faces in face_cache.

[Codrut Gusoi]

Yes there is a reliable recipe to reproduce.

I've prepared the VM, just let me know how I should send you the login
credentials (without the whole internet getting their hands on them)
since I can not write them here.

The exact recipe is described in my first email, here is what you need
to do on the VM now that I set it up:

First make sure your terminal is either maximized or fullscreen. Then

cd ~/emacs
emacs -nw .

Now that you have emacs running you should wait untill you see 2
windows: neotree on the left and dired on the right. You need to type:
<SPACE> p f. That is 'spacebar' followed by a 'p' followed by an 'f',
lower case. This will open helm on the lower part of the screen. From
here type what's between the quotes here: "src/term.c" then press
return (RET). You will have to wait a few seconds, then the segfault
will happen.

This is an Ubuntu 14.04 VM, you should be able to do anything you
please with it.

I am also available to chat on any platform you desire if you need
real time feedback from me.

On Mon, Mar 20, 2017 at 8:46 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> [Please keep the bug address on the CC list.]
>
>> From: Codrut Gusoi <codrut.gusoi@gmail.com>
>> Date: Mon, 20 Mar 2017 14:16:29 +0200
>>
>> I've tried watching the `used` count with `watch -l` and there is no
>> place where the font cache shrinks for the current frame. I believe
>> what I experineced previously was the `used` count from different
>> frames (since I added a breakpoint but did not look at the frame
>> address).
>
> For the bug you describe to happen, the 'used' count of a frame's face
> cache _must_ be reset to zero at some point.  There simply isn't any
> other way to "lose" faces from the cache.  So if you don't see that,
> it means you didn't succeed to reproduce the problem.
>
>> Have you managed to reproduce it locally?
>
> No.
>
>> If not, what do you say I set up a VM in AWS and give you the IP and
>> login details to take a look.
>
> That could work if there's a more-or-less reliable recipe for
> reproducing the problem.  Is there?
>
> If there is not recipe, I won't be able to investigate the path to
> disaster, and when the segfault happens, it's already too late.

bug#26097: Segmentation fault due to missing faces in face_cache.

[Eli Zaretskii]

> From: Codrut Gusoi <codrut.gusoi@gmail.com>
> Date: Mon, 20 Mar 2017 21:40:30 +0200
> Cc: 26097@debbugs.gnu.org
> 
> Yes there is a reliable recipe to reproduce.
> 
> I've prepared the VM, just let me know how I should send you the login
> credentials (without the whole internet getting their hands on them)
> since I can not write them here.
> 
> The exact recipe is described in my first email, here is what you need
> to do on the VM now that I set it up:
> 
> First make sure your terminal is either maximized or fullscreen. Then
> 
> cd ~/emacs
> emacs -nw .
> 
> Now that you have emacs running you should wait untill you see 2
> windows: neotree on the left and dired on the right. You need to type:
> <SPACE> p f. That is 'spacebar' followed by a 'p' followed by an 'f',
> lower case. This will open helm on the lower part of the screen. From
> here type what's between the quotes here: "src/term.c" then press
> return (RET). You will have to wait a few seconds, then the segfault
> will happen.

Thanks, this is now fixed on the master branch (and on your machine,
so you can now install the fixed binary).

Thank you very much for letting me debug this on your system.

bug#26097: Segmentation fault due to missing faces in face_cache.

[Codrut Gusoi]

Just tried it on my laptop, it works great.

I have to thank you for dedicating your time and effort into fixing
this. Your work is greatly appreciated!

Regards,
Codrut


On Sat, Mar 25, 2017 at 4:01 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Codrut Gusoi <codrut.gusoi@gmail.com>
>> Date: Mon, 20 Mar 2017 21:40:30 +0200
>> Cc: 26097@debbugs.gnu.org
>>
>> Yes there is a reliable recipe to reproduce.
>>
>> I've prepared the VM, just let me know how I should send you the login
>> credentials (without the whole internet getting their hands on them)
>> since I can not write them here.
>>
>> The exact recipe is described in my first email, here is what you need
>> to do on the VM now that I set it up:
>>
>> First make sure your terminal is either maximized or fullscreen. Then
>>
>> cd ~/emacs
>> emacs -nw .
>>
>> Now that you have emacs running you should wait untill you see 2
>> windows: neotree on the left and dired on the right. You need to type:
>> <SPACE> p f. That is 'spacebar' followed by a 'p' followed by an 'f',
>> lower case. This will open helm on the lower part of the screen. From
>> here type what's between the quotes here: "src/term.c" then press
>> return (RET). You will have to wait a few seconds, then the segfault
>> will happen.
>
> Thanks, this is now fixed on the master branch (and on your machine,
> so you can now install the fixed binary).
>
> Thank you very much for letting me debug this on your system.

bug#26097: Segmentation fault due to missing faces in face_cache.

[Eli Zaretskii]

> From: Codrut Gusoi <codrut.gusoi@gmail.com>
> Date: Sat, 25 Mar 2017 18:46:42 +0200
> Cc: 26097-done@debbugs.gnu.org
> 
> Just tried it on my laptop, it works great.

Thanks for testing.

> I have to thank you for dedicating your time and effort into fixing
> this. Your work is greatly appreciated!

You are welcome.  It was a tricky problem, caused by -- what else? --
linum-mode, which kicked in once src/term.c was visited.