bug#72692: Emacs 31.05 (40eecd594ac) get SIGSEGV on Linux (Linux 6.6.45 Kde Wayland)

[Pip Cet via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

"Eli Zaretskii" <eliz@gnu.org> writes:

>> Date: Mon, 19 Aug 2024 13:32:42 +0000
>> From: Pip Cet <pipcet@protonmail.com>
>> Cc: execvy@gmail.com, 72692@debbugs.gnu.org
>>
>> >> * modify the right frame parameter (such as alpha-background) so that
>> >>   the basic faces are re-realized ('free_realized_face' is called for
>> >>   them), but 'free_realized_faces' is not.
>> >
>> > Basic faces are routinely freed and re-realized whenever we start the
>> > display iteration, see init_iterator.
>>
>> > AFAIR, all you need to do for
>> > that is to customize some face -- doing so sets the face_change flag,
>> > and init_iterator will then normally free all the faces and realize
>> > them again.
>>
>> ... which won't trigger the bug, because it calls 'free_realized_faces'.
>
> Not necessarily.  I show below a backtrace which called
> realize_basic_faces and triggered the same freeing of the fontset of
> the ASCII face.  This was obtained by changing the color of the
> default face via Customize.

But only momentarily, right? Because we set f->face_change = true in
update_face_from_frame_parameter, under this comment:

  /* Changing a named face means that all realized faces depending on
     that face are invalid.  Since we cannot tell which realized faces
     depend on the face, make sure they are all removed.  This is done
     by setting face_change.  The next call to init_iterator will then
     free realized faces.  */

>> I specifically explained why 'free_realized_face' must be called
>> directly, not via (or after) 'free_realized_faces', to trigger the bug.
>
> Any caller of realize_face (and only those, AFAICT) will go that path.

'realize_basic_faces' doesn't call 'free_realized_faces', just
'free_realized_face', via 'realize_face' and its 'former_face_id'.

> Which is why I asked for a backtrace in your case (since I cannot
> reproduce it exactly myself).  As I explained in my other message,
> there's potentially a much more serious problem here, if indeed you
> are right.

I think it's a serious problem either way.  Leaving a pointer to freed
memory in a structure is a bug unless there's a VERY LOUD COMMENT
explaining that this is so.

>> I meant why we need at least two non-ASCII faces to trigger the bug.
>
> Which bug?  I can trigger freeing the fontset of an ASCII face while
> its non-ASCII variants are not freed without having 2 ASCII faces, see
> the backtrace below.

I can trigger that part, but not the crash, using only one non-ASCII
face.

>> Here's a reproducer hibiscus.el which uses buffer text:
>>
>> (while t
>>   (insert (concat (make-string 1 (floor (random 132000)))))
>>   (set-frame-parameter nil 'alpha-background 1.0)
>>   (sit-for 1.0))
>
> Thanks, but this doesn't help me because AFAIK alpha-background is not
> supported on Windows.

w32_frame_parm_handlers includes it by setting the
gui_set_alpha_background slot, though.

> Here's the backtrace I promised:
>
> #0  realize_face (cache=0x7c73288, attrs=0xbfb8d8, former_face_id=0)
>     at xfaces.c:6097
> #1  0x00df6b33 in realize_default_face (f=0x7c6bad8) at xfaces.c:6010
> #2  0x00df5d73 in realize_basic_faces (f=0x7c6bad8) at xfaces.c:5862
> #3  0x00def95a in update_face_from_frame_parameter (f=0x7c6bad8,
>     param=XIL(0x8940), new_value=XIL(0x8000000010805bb8)) at xfaces.c:3813

update_face_from_frame_parameter will set f->face_change = true before
it returns.

> And I wrote a simple GDB script that loops over the cached faces when
> free_realized_face is called under conditions that will cause it to
> call free_face_fontset, and got this:
>
>   face 0xbcad118(N), fontset 3, ascii 0x10643628
>   face 0x1025a7f0(N), fontset 3, ascii 0x10643628
>   face 0x101a0f50(N), fontset 3, ascii 0x10643628
>   face 0x1016a328(N), fontset 3, ascii 0x10643628
>   face 0xc02ed68(N), fontset 3, ascii 0x10643628
>   face 0xb9fb020(N), fontset 3, ascii 0x10643628
>   face 0xb98fc38(N), fontset 3, ascii 0x10643628
>   face 0xb9b1498(N), fontset 3, ascii 0x10643628
>   face 0x7c4cd48(N), fontset 3, ascii 0x10643628
>   face 0xbcbb350(N), fontset 3, ascii 0x10643628
>   face 0x107e5410(N), fontset 3, ascii 0x10643628
>   face 0x105ff8e8(N), fontset 3, ascii 0x10643628
>   face 0xbcab9f8(N), fontset 3, ascii 0x10643628
>   face 0xb9c8cd0(N), fontset 3, ascii 0x10643628
>   face 0xb99e470(N), fontset 3, ascii 0x10643628
>   face 0xb998d38(N), fontset 3, ascii 0x10643628
>   face 0xb97cbd0(N), fontset 3, ascii 0x10643628
>   face 0x104ac2b8(N), fontset 3, ascii 0x10643628
>   face 0x10167af0(N), fontset 3, ascii 0x10643628
>   face 0x10643d30(N), fontset 3, ascii 0x10643628
>   face 0x104d0c48(N), fontset 3, ascii 0x10643628
>   face 0x107e31b0(N), fontset 3, ascii 0x10643628
>   face 0xb949650(N), fontset 3, ascii 0x10643628
>   face 0xb949758(N), fontset 3, ascii 0x10643628
>   face 0x105403f0(N), fontset 3, ascii 0x10643628
>   face 0x105404f8(N), fontset 3, ascii 0x10643628
>   face 0x10540600(N), fontset 3, ascii 0x10643628
>   face 0x10540708(N), fontset 3, ascii 0x10643628
>   face 0x10540810(N), fontset 3, ascii 0x10643628
>   face 0x10540918(N), fontset 3, ascii 0x10643628
>   face 0x10540a20(N), fontset 3, ascii 0x10643628
>   face 0x10540b28(N), fontset 3, ascii 0x10643628
>   face 0x104a4fe8(N), fontset 3, ascii 0x10643628
>   face 0x104a50f0(N), fontset 3, ascii 0x10643628
>   face 0x104a51f8(N), fontset 3, ascii 0x10643628
>   face 0x104a5300(N), fontset 3, ascii 0x10643628
>
> The "(N)" part means that this face is not ASCII face (its ASCII
> parent is shown by "ascii 0xNNNNN").
>
> Except that in this case the caller sets the frame's 'face_change'
> flag, which then frees and all the non-ASCII faces the first time we
> call init_iterator.

I'll check whether setting f->face_change = true in the relevant places
is enough to avoid the bug...

Pip