GNU bug report logs - #74274
[PATCH] Revert part of d3f8ed730f to avoid segmentation fault

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 74274 in the body.
You can then email your comments to 74274 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Gong Qijian <gongqijian <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Cc: Gerd Möllmann <gerd <at> gnu.org>,
 Gong Qijian <gongqijian <at> gmail.com>
Subject: [PATCH] Revert part of d3f8ed730f to avoid segmentation fault
Date: Fri,  8 Nov 2024 14:31:50 +0800

Patch for branch scratch/tty-child-frames to avoid segmentation fault.

The issue can be triggered by the message function when creating a tty
child frame during the initialization process.

Reproduce:

  $ src/emacs -nw -Q --eval "\
    (progn
      (require 'cl-lib)
      (require 'tty-tip)
      (advice-add 'tty-tip--compute-position :around
       (defun tty-tip--compute-position <at> fix-nil-error (&rest args)
         (cl-letf ((orig-mouse-position (symbol-function #'mouse-position))
                   ((symbol-function #'mouse-position)
                    (lambda ()
                      (if (terminal-parameter nil 'xterm-mouse-x)
                          (funcall orig-mouse-position)
                        (cons (window-frame) (posn-x-y (posn-at-point)))))))
           (apply args))))

      (tty-tip--create-frame \"line1\nline2\")
      (message \"tty-type: %S\" (tty-type)))"
  Fatal error 11: Segmentation fault
  ^[[Ifish: Job 1, 'src/emacs -nw -Q --eval "\…' terminated by signal (pro… (SIGABRT)
  fish: Job Abort, '' terminated by signal  ()

---
 src/term.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/term.c b/src/term.c
index a7f7baa6e3..8aeabd76b7 100644
--- a/src/term.c
+++ b/src/term.c
@@ -781,7 +781,11 @@ tty_write_glyphs (struct frame *f, struct glyph *string, int len)
     {
       /* Identify a run of glyphs with the same face.  */
       int face_id = string->face_id;
-      struct frame *face_id_frame = string->frame;
+      /* FIXME/tty: it happens that a single glyph's frame is NULL.  It
+	 might depend on a tab bar line being present, then switching
+	 from a buffer without header line to one with header line and
+	 opening a child frame.  */
+      struct frame *face_id_frame = string->frame ? string->frame : f;
 
       for (n = 1; n < stringlen; ++n)
 	if (string[n].face_id != face_id || string[n].frame != face_id_frame)
-- 
2.42.0

Message #8 received at 74274 <at> debbugs.gnu.org (full text, mbox):

From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
To: Gong Qijian <gongqijian <at> gmail.com>
Cc: Gerd Möllmann <gerd <at> gnu.org>, 74274 <at> debbugs.gnu.org
Subject: Re: bug#74274: [PATCH] Revert part of d3f8ed730f to avoid
 segmentation fault
Date: Sat, 09 Nov 2024 05:15:29 +0100

Gong Qijian <gongqijian <at> gmail.com> writes:

> Patch for branch scratch/tty-child-frames to avoid segmentation fault.
>
> The issue can be triggered by the message function when creating a tty
> child frame during the initialization process.
>
> Reproduce:
>
>   $ src/emacs -nw -Q --eval "\
>     (progn
>       (require 'cl-lib)
>       (require 'tty-tip)
>       (advice-add 'tty-tip--compute-position :around
>        (defun tty-tip--compute-position <at> fix-nil-error (&rest args)
>          (cl-letf ((orig-mouse-position (symbol-function #'mouse-position))
>                    ((symbol-function #'mouse-position)
>                     (lambda ()
>                       (if (terminal-parameter nil 'xterm-mouse-x)
>                           (funcall orig-mouse-position)
>                         (cons (window-frame) (posn-x-y (posn-at-point)))))))
>            (apply args))))
>
>       (tty-tip--create-frame \"line1\nline2\")
>       (message \"tty-type: %S\" (tty-type)))"
>   Fatal error 11: Segmentation fault
>   ^[[Ifish: Job 1, 'src/emacs -nw -Q --eval "\…' terminated by signal (pro… (SIGABRT)
>   fish: Job Abort, '' terminated by signal  ()

Thanks Gong Qijian, I think I can reproduce this here. Before debugging
this, I'd like to understand your test case a bit better, though. (And
I'm procrastinating a bit :-).)

Do I understand this right, that the advice you add to
tty-tip-compute-position only serves the purpose of being able to pop up
a tip frame early, when mouse-position doesn't really have a position to
report? Or does it also serve another purpose?

Another interesting question would be in which other, let's say more
normal, circumstances you observe this segfault.

Message #11 received at 74274 <at> debbugs.gnu.org (full text, mbox):

From: qijian gong <gongqijian <at> gmail.com>
To: Gerd Möllmann <gerd.moellmann <at> gmail.com>
Cc: Gerd Möllmann <gerd <at> gnu.org>, 74274 <at> debbugs.gnu.org
Subject: Re: bug#74274: [PATCH] Revert part of d3f8ed730f to avoid
 segmentation fault
Date: Sat, 9 Nov 2024 14:10:26 +0800

[Message part 1 (text/plain, inline)]

Gerd Möllmann <gerd.moellmann <at> gmail.com>于2024年11月9日 周六下午12:15写道：

Do I understand this right, that the advice you add to
> tty-tip-compute-position only serves the purpose of being able to pop up
> a tip frame early, when mouse-position doesn't really have a position to
> report? Or does it also serve another purpose?
>

Yes, that's correct, there is no other purpose.  It seems mouse-position
doesn't really have a position until the mouse was moved by user.

>

[Message part 2 (text/html, inline)]

Message #14 received at 74274 <at> debbugs.gnu.org (full text, mbox):

From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
To: qijian gong <gongqijian <at> gmail.com>
Cc: Gerd Möllmann <gerd <at> gnu.org>, 74274 <at> debbugs.gnu.org
Subject: Re: bug#74274: [PATCH] Revert part of d3f8ed730f to avoid
 segmentation fault
Date: Sat, 09 Nov 2024 07:59:53 +0100

qijian gong <gongqijian <at> gmail.com> writes:

> Gerd Möllmann <gerd.moellmann <at> gmail.com>于2024年11月9日 周六下午12:15写道：
>
>  Do I understand this right, that the advice you add to
>  tty-tip-compute-position only serves the purpose of being able to pop up
>  a tip frame early, when mouse-position doesn't really have a position to
>  report? Or does it also serve another purpose?
>
> Yes, that's correct, there is no other purpose. It seems
> mouse-position doesn't really have a position until the mouse was
> moved by user.

Ok, thanks.

I think I know what this is. It's the very particular case of the very
first redisplay + presence of a child freame, which copies glyphs from a
current matrix that is still clear, i.e. zeroed, so that glyph::frame
is zero and so on.

I wonder a bit - how did you come up with such a test case? :-)
Did you perhaps see this happening elsewhere? That would be interesting.

Message #17 received at 74274 <at> debbugs.gnu.org (full text, mbox):

From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
To: qijian gong <gongqijian <at> gmail.com>
Cc: Gerd Möllmann <gerd <at> gnu.org>, 74274 <at> debbugs.gnu.org
Subject: Re: bug#74274: [PATCH] Revert part of d3f8ed730f to avoid
 segmentation fault
Date: Sat, 09 Nov 2024 08:23:44 +0100

Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:

> qijian gong <gongqijian <at> gmail.com> writes:
>
>> Gerd Möllmann <gerd.moellmann <at> gmail.com>于2024年11月9日 周六下午12:15写道：
>>
>>  Do I understand this right, that the advice you add to
>>  tty-tip-compute-position only serves the purpose of being able to pop up
>>  a tip frame early, when mouse-position doesn't really have a position to
>>  report? Or does it also serve another purpose?
>>
>> Yes, that's correct, there is no other purpose. It seems
>> mouse-position doesn't really have a position until the mouse was
>> moved by user.
>
> Ok, thanks.
>
> I think I know what this is. It's the very particular case of the very
> first redisplay + presence of a child freame, which copies glyphs from a
> current matrix that is still clear, i.e. zeroed, so that glyph::frame
> is zero and so on.

Which is fixed for me with

1 file changed, 6 insertions(+), 3 deletions(-)
src/dispnew.c | 9 ++++++---

modified   src/dispnew.c
@@ -3544,9 +3544,12 @@ prepare_desired_root_row (struct frame *root, int y)
   if (!root_row->enabled_p)
     {
       struct glyph_row *from = MATRIX_ROW (root->current_matrix, y);
-      memcpy (root_row->glyphs[0], from->glyphs[0],
-	      root->current_matrix->matrix_w * sizeof (struct glyph));
-      root_row->enabled_p = true;
+      if (from->enabled_p)
+	{
+	  memcpy (root_row->glyphs[0], from->glyphs[0],
+		  root->current_matrix->matrix_w * sizeof (struct glyph));
+	  root_row->enabled_p = true;
+	}
     }
   return root_row;
 }

Message #20 received at 74274 <at> debbugs.gnu.org (full text, mbox):

From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
To: qijian gong <gongqijian <at> gmail.com>
Cc: Gerd Möllmann <gerd <at> gnu.org>, 74274 <at> debbugs.gnu.org
Subject: Re: bug#74274: [PATCH] Revert part of d3f8ed730f to avoid
 segmentation fault
Date: Sat, 09 Nov 2024 09:02:34 +0100

Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:

> Which is fixed for me with

Also committed to savannah now.

Message #23 received at 74274 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Gerd Möllmann <gerd.moellmann <at> gmail.com>
Cc: gerd <at> gnu.org, gongqijian <at> gmail.com, 74274 <at> debbugs.gnu.org
Subject: Re: bug#74274: [PATCH] Revert part of d3f8ed730f to avoid segmentation
 fault
Date: Sat, 09 Nov 2024 10:17:23 +0200

> Cc: Gerd Möllmann <gerd <at> gnu.org>, 74274 <at> debbugs.gnu.org
> From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
> Date: Sat, 09 Nov 2024 08:23:44 +0100
> 
> Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:
> 
> > qijian gong <gongqijian <at> gmail.com> writes:
> >
> >> Gerd Möllmann <gerd.moellmann <at> gmail.com>于2024年11月9日 周六下午12:15写道：
> >>
> >>  Do I understand this right, that the advice you add to
> >>  tty-tip-compute-position only serves the purpose of being able to pop up
> >>  a tip frame early, when mouse-position doesn't really have a position to
> >>  report? Or does it also serve another purpose?
> >>
> >> Yes, that's correct, there is no other purpose. It seems
> >> mouse-position doesn't really have a position until the mouse was
> >> moved by user.
> >
> > Ok, thanks.
> >
> > I think I know what this is. It's the very particular case of the very
> > first redisplay + presence of a child freame, which copies glyphs from a
> > current matrix that is still clear, i.e. zeroed, so that glyph::frame
> > is zero and so on.
> 
> Which is fixed for me with
> 
> 1 file changed, 6 insertions(+), 3 deletions(-)
> src/dispnew.c | 9 ++++++---
> 
> modified   src/dispnew.c
> @@ -3544,9 +3544,12 @@ prepare_desired_root_row (struct frame *root, int y)
>    if (!root_row->enabled_p)
>      {
>        struct glyph_row *from = MATRIX_ROW (root->current_matrix, y);
> -      memcpy (root_row->glyphs[0], from->glyphs[0],
> -	      root->current_matrix->matrix_w * sizeof (struct glyph));
> -      root_row->enabled_p = true;
> +      if (from->enabled_p)
> +	{
> +	  memcpy (root_row->glyphs[0], from->glyphs[0],
> +		  root->current_matrix->matrix_w * sizeof (struct glyph));
> +	  root_row->enabled_p = true;
> +	}
>      }
>    return root_row;
>  }

Should this perhaps have an eassert which verifies that every glyph
has a valid frame pointer?  At the very least please add a comment
there explaining the need for the enabled_p test and mentioning the
frame pointer of the glyphs.

Thanks.

Message #26 received at 74274 <at> debbugs.gnu.org (full text, mbox):

From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: gerd <at> gnu.org, gongqijian <at> gmail.com, 74274 <at> debbugs.gnu.org
Subject: Re: bug#74274: [PATCH] Revert part of d3f8ed730f to avoid
 segmentation fault
Date: Sat, 09 Nov 2024 09:26:23 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

> Should this perhaps have an eassert which verifies that every glyph
> has a valid frame pointer?  At the very least please add a comment
> there explaining the need for the enabled_p test and mentioning the
> frame pointer of the glyphs.

I don't know. It's one of the most basic things about the meaning of
enabled_p in current glyphs, and it's not limited the frame pointer in
any way.

Message #29 received at 74274 <at> debbugs.gnu.org (full text, mbox):

From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: gerd <at> gnu.org, gongqijian <at> gmail.com, 74274 <at> debbugs.gnu.org
Subject: Re: bug#74274: [PATCH] Revert part of d3f8ed730f to avoid
 segmentation fault
Date: Sun, 10 Nov 2024 11:56:07 +0100

Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:

> Eli Zaretskii <eliz <at> gnu.org> writes:
>
>> Should this perhaps have an eassert which verifies that every glyph
>> has a valid frame pointer?  At the very least please add a comment
>> there explaining the need for the enabled_p test and mentioning the
>> frame pointer of the glyphs.
>
> I don't know. It's one of the most basic things about the meaning of
> enabled_p in current glyphs, and it's not limited the frame pointer in
> any way.

Found another case of copying from non-enabled glyphs. Reproducable with
emacs -q -l with a file containing

  (with-current-buffer (get-buffer-create "1")
    (setq header-line-format
          '((:eval (format "*package*: -  symbol-packages: - lexical-binding: %s"
                           lexical-binding)))))
  (with-current-buffer (get-buffer-create "2")
    (insert "something"))

  (let ((w1 (selected-window))
        (w2 (split-window-right)))
    (set-window-buffer w1 (get-buffer "1"))
    (set-window-buffer w2 (get-buffer "2"))
    (message "message"))

It's a 30 years old bug, so it isn't something urgent to fix.

I'll port something to savannah as soon as I find the time.

Message #32 received at 74274 <at> debbugs.gnu.org (full text, mbox):

From: Gerd Möllmann <gerd.moellmann <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: gerd <at> gnu.org, gongqijian <at> gmail.com, 74274 <at> debbugs.gnu.org
Subject: Re: bug#74274: [PATCH] Revert part of d3f8ed730f to avoid
 segmentation fault
Date: Sun, 10 Nov 2024 20:06:58 +0100

Gerd Möllmann <gerd.moellmann <at> gmail.com> writes:

> I'll port something to savannah as soon as I find the time.

Done, and closing because I don't expect further input.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.