bug#74274: [PATCH] Revert part of d3f8ed730f to avoid segmentation fault

bug#74274: [PATCH] Revert part of d3f8ed730f to avoid segmentation fault

[Gong Qijian]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1930 bytes --]

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@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

bug#74274: [PATCH] Revert part of d3f8ed730f to avoid segmentation fault

[Gerd Möllmann]

Gong Qijian <gongqijian@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@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.

bug#74274: [PATCH] Revert part of d3f8ed730f to avoid segmentation fault

[qijian gong]

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

Gerd Möllmann <gerd.moellmann@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.

>

[-- Attachment #2: Type: text/html, Size: 1054 bytes --]

bug#74274: [PATCH] Revert part of d3f8ed730f to avoid segmentation fault

[Gerd Möllmann]

qijian gong <gongqijian@gmail.com> writes:

> Gerd Möllmann <gerd.moellmann@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.

bug#74274: [PATCH] Revert part of d3f8ed730f to avoid segmentation fault

[Gerd Möllmann]

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

> qijian gong <gongqijian@gmail.com> writes:
>
>> Gerd Möllmann <gerd.moellmann@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;
 }

bug#74274: [PATCH] Revert part of d3f8ed730f to avoid segmentation fault

[Gerd Möllmann]

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

> Which is fixed for me with

Also committed to savannah now.

bug#74274: [PATCH] Revert part of d3f8ed730f to avoid segmentation fault

[Eli Zaretskii]

> Cc: Gerd Möllmann <gerd@gnu.org>, 74274@debbugs.gnu.org
> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Date: Sat, 09 Nov 2024 08:23:44 +0100
> 
> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
> 
> > qijian gong <gongqijian@gmail.com> writes:
> >
> >> Gerd Möllmann <gerd.moellmann@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.

bug#74274: [PATCH] Revert part of d3f8ed730f to avoid segmentation fault

[Gerd Möllmann]

Eli Zaretskii <eliz@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.

bug#74274: [PATCH] Revert part of d3f8ed730f to avoid segmentation fault

[Gerd Möllmann]

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

> Eli Zaretskii <eliz@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.

bug#74274: [PATCH] Revert part of d3f8ed730f to avoid segmentation fault

[Gerd Möllmann]

Gerd Möllmann <gerd.moellmann@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.