bug#56095: 29.0.50; nsterm.m, use after free

bug#56095: 29.0.50; nsterm.m, use after free

[Gerd Möllmann]

So, I'm trying Emacs on MacOS now, get some non-reproducible
crashes, built master with ASAN, and the first thing it found is this:

==61522==ERROR: AddressSanitizer: heap-use-after-free on address 0x00012d7deb90 at pc 0x0001008c1514 bp 0x00016fdf7230 sp 0x00016fdf7228
WRITE of size 8 at 0x00012d7deb90 thread T0
==61522==WARNING: Can't read from symbolizer at fd 25
==61522==WARNING: Can't read from symbolizer at fd 26
==61522==WARNING: Can't read from symbolizer at fd 27
==61522==WARNING: Can't read from symbolizer at fd 28
==61522==WARNING: Failed to use and restart external symbolizer!
    #0 0x1008c1510 in wset_vertical_scroll_bar+0x4c (/Users/gerd/repos/emacs/src/emacs:arm64+0x1008c1510)
    #1 0x1008c19a0 in -[EmacsScroller judge]+0x360 (/Users/gerd/repos/emacs/src/emacs:arm64+0x1008c19a0)
    #2 0x1008d641c in ns_judge_scroll_bars+0x224 (/Users/gerd/repos/emacs/src/emacs:arm64+0x1008d641c)
    #3 0x1000fa4ec in redisplay_internal+0x4ca4 (/Users/gerd/repos/emacs/src/emacs:arm64+0x1000fa4ec)
...

0x00012d7deb90 is located 656 bytes inside of 4096-byte region [0x00012d7de900,0x00012d7df900)
freed by thread T0 here:
    #0 0x1031c7c94 in wrap_free+0x98 (/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/13.1.6/lib/darwin/libclang_rt.asan_osx_dynamic.dylib:arm64e+0x3fc94)
    #1 0x1009aec74 in rpl_free+0x7c (/Users/gerd/repos/emacs/src/emacs:arm64+0x1009aec74)
    #2 0x100598488 in xfree+0x38 (/Users/gerd/repos/emacs/src/emacs:arm64+0x100598488)
    #3 0x1005bad4c in sweep_vectors+0x2f4 (/Users/gerd/repos/emacs/src/emacs:arm64+0x1005bad4c)
    #4 0x1005acf58 in gc_sweep+0x20 (/Users/gerd/repos/emacs/src/emacs:arm64+0x1005acf58)
    #5 0x1005ab1a4 in garbage_collect+0x9f0 (/Users/gerd/repos/emacs/src/emacs:arm64+0x1005ab1a4)
    #6 0x1005aa720 in maybe_garbage_collect+0x28 (/Users/gerd/repos/emacs/src/emacs:arm64+0x1005aa720)
    #7 0x100641714 in maybe_gc+0x54 (/Users/gerd/repos/emacs/src/emacs:arm64+0x100641714)
    #8 0x10063a9f0 in Ffuncall+0x3c8 (/Users/gerd/repos/emacs/src/emacs:arm64+0x10063a9f0)
    #9 0x10063d468 in internal_condition_case_n+0x1d4 (/Users/gerd/repos/emacs/src/emacs:arm64+0x10063d468)
    #10 0x1000d52b8 in safe__call+0x16a8 (/Users/gerd/repos/emacs/src/emacs:arm64+0x1000d52b8)
    #11 0x1000d3b60 in safe_call+0x164 (/Users/gerd/repos/emacs/src/emacs:arm64+0x1000d3b60)
    #12 0x1000d542c in safe_call1+0x28 (/Users/gerd/repos/emacs/src/emacs:arm64+0x1000d542c)
    #13 0x10019c5b8 in handle_fontified_prop+0xb04 (/Users/gerd/repos/emacs/src/emacs:arm64+0x10019c5b8)
    #14 0x100196e0c in handle_stop+0x324 (/Users/gerd/repos/emacs/src/emacs:arm64+0x100196e0c)
    #15 0x1001a9294 in next_element_from_buffer+0xa18 (/Users/gerd/repos/emacs/src/emacs:arm64+0x1001a9294)
    #16 0x1000a639c in get_next_display_element+0x29c (/Users/gerd/repos/emacs/src/emacs:arm64+0x1000a639c)
    #17 0x10011344c in display_line+0x1dd4 (/Users/gerd/repos/emacs/src/emacs:arm64+0x10011344c)
    #18 0x1001104e4 in try_window+0x564 (/Users/gerd/repos/emacs/src/emacs:arm64+0x1001104e4)
    #19 0x1001e6c28 in redisplay_window+0x70e0 (/Users/gerd/repos/emacs/src/emacs:arm64+0x1001e6c28)
...

previously allocated by thread T0 here:
    #0 0x1031c7b58 in wrap_malloc+0x94 (/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/13.1.6/lib/darwin/libclang_rt.asan_osx_dynamic.dylib:arm64e+0x3fb58)
    #1 0x100598138 in lmalloc+0x44 (/Users/gerd/repos/emacs/src/emacs:arm64+0x100598138)
    #2 0x100598054 in xmalloc+0x40 (/Users/gerd/repos/emacs/src/emacs:arm64+0x100598054)
    #3 0x1005b28f4 in allocate_vector_block+0x20 (/Users/gerd/repos/emacs/src/emacs:arm64+0x1005b28f4)
    #4 0x1005b2640 in allocate_vector_from_block+0x2a0 (/Users/gerd/repos/emacs/src/emacs:arm64+0x1005b2640)
    #5 0x1005a4c54 in allocate_vectorlike+0x70 (/Users/gerd/repos/emacs/src/emacs:arm64+0x1005a4c54)
    #6 0x1005a4b40 in allocate_pseudovector+0x38 (/Users/gerd/repos/emacs/src/emacs:arm64+0x1005a4b40)
    #7 0x1002838cc in allocate_window+0x18 (/Users/gerd/repos/emacs/src/emacs:arm64+0x1002838cc)
    #8 0x100288a78 in make_parent_window+0x3c (/Users/gerd/repos/emacs/src/emacs:arm64+0x100288a78)
    #9 0x100287508 in Fsplit_window_internal+0xbc0 (/Users/gerd/repos/emacs/src/emacs:arm64+0x100287508)
...

That is, EmacsScroller modifies a struct window that has already been
free'd during a GC that was triggered during redisplay.

AFAICS, EmacsScroller is part of ns_display_info and hold a pointer to a
struct window.  AFAICS, nothing is marking that window during GC, so...

Sorry, no patch because I don't really know what I'm doing ;-).

bug#56095: 29.0.50; nsterm.m, use after free

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

> So, I'm trying Emacs on MacOS now, get some non-reproducible
> crashes, built master with ASAN, and the first thing it found is this:
>
> ==61522==ERROR: AddressSanitizer: heap-use-after-free on address 0x00012d7deb90 at pc 0x0001008c1514 bp 0x00016fdf7230 sp 0x00016fdf7228
> WRITE of size 8 at 0x00012d7deb90 thread T0
> ==61522==WARNING: Can't read from symbolizer at fd 25
> ==61522==WARNING: Can't read from symbolizer at fd 26
> ==61522==WARNING: Can't read from symbolizer at fd 27
> ==61522==WARNING: Can't read from symbolizer at fd 28
> ==61522==WARNING: Failed to use and restart external symbolizer!
>     #0 0x1008c1510 in wset_vertical_scroll_bar+0x4c (/Users/gerd/repos/emacs/src/emacs:arm64+0x1008c1510)
>     #1 0x1008c19a0 in -[EmacsScroller judge]+0x360 (/Users/gerd/repos/emacs/src/emacs:arm64+0x1008c19a0)
>     #2 0x1008d641c in ns_judge_scroll_bars+0x224 (/Users/gerd/repos/emacs/src/emacs:arm64+0x1008d641c)
>     #3 0x1000fa4ec in redisplay_internal+0x4ca4 (/Users/gerd/repos/emacs/src/emacs:arm64+0x1000fa4ec)
> ...

Isn't that a bug, since scroll bars should only be judged for windows
that are still reachable?

bug#56095: 29.0.50; nsterm.m, use after free

[Gerd Möllmann]

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

On 20. Jun 2022, 03:22 +0200, Po Lu <luangruo@yahoo.com>, wrote:
>
> Isn't that a bug, since scroll bars should only be judged for windows
> that are still reachable?

I'd say no:  Ns_judge_scroll_bars is called for a frame, and loops over EmascScroller subviews of that frame. I don't see how it is could be certain that all EmascScrollers reference a live Emacs window at that point.

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

bug#56095: 29.0.50; nsterm.m, use after free

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

> I'd say no: Ns_judge_scroll_bars is called for a frame, and loops over
> EmascScroller subviews of that frame. I don't see how it is could be
> certain that all EmascScrollers reference a live Emacs window at that
> point.

I see.  Still, the window should always be reachable through the struct
scroll_bar, which is scanned by GC and ought to be alive as long as the
EmacsScroller is.

But it seems there is no struct scroll_bar at all in the NS port!  (Why
does it always have to do things differently?)  Does anyone want me to
fix that?

In the meantime, a loop through all the scroll bars in a mark_nsterm
function would suffice.

bug#56095: 29.0.50; nsterm.m, use after free

[Gerd Möllmann]

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

On 20. Jun 2022, 12:21 +0200, Po Lu <luangruo@yahoo.com>, wrote:
> But it seems there is no struct scroll_bar at all in the NS port! (Why
> does it always have to do things differently?)
Think Different (tm) :-).
> Does anyone want me to
> fix that?
I have no opinion on that.
>
> In the meantime, a loop through all the scroll bars in a mark_nsterm
> function would suffice.
Please find a patch for that attached.

As an aside, if anyone has heard anything about building GDB on an M1 Mac, I'd be grateful for a pointer. I'm asking because I'm currently stuck with lldb on my machine.  GDB doesn't seem to build - it doesn't produce an executable, but apparently, if I'm not blind, without saying what the problem is.

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

[-- Attachment #2: 0001-Prevent-GC-of-window-referenced-from-EmacsScroller.patch --]
[-- Type: application/octet-stream, Size: 2486 bytes --]

From 8842a994fabf65197fa48beed8bd88066f1655e2 Mon Sep 17 00:00:00 2001
From: Gerd Moellmann <gerd.moellmann@gmail.com>
Date: Tue, 21 Jun 2022 15:49:44 +0200
Subject: [PATCH] Prevent GC of window referenced from EmacsScroller

* src/nsterm.m (EmacsScroller.mark, mark_nsterm): New functions.
* src/nsterm.h (EmacsScroller.mark, mark_nsterm): Declare.
* src/alloc.c (garbage_collect) [MAVE_NS]: Call mark_nsterm.
---
 src/alloc.c  |  4 ++++
 src/nsterm.h |  4 ++++
 src/nsterm.m | 30 ++++++++++++++++++++++++++++++
 3 files changed, 38 insertions(+)

diff --git a/src/alloc.c b/src/alloc.c
index 55e18ecd77..f115a3ceba 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -6204,6 +6204,10 @@ garbage_collect (void)
   mark_xterm ();
 #endif
 
+#ifdef HAVE_NS
+  mark_nsterm ();
+#endif
+
   /* Everything is now marked, except for the data in font caches,
      undo lists, and finalizers.  The first two are compacted by
      removing an items which aren't reachable otherwise.  */
diff --git a/src/nsterm.h b/src/nsterm.h
index c4fdc7054f..7a097b3248 100644
--- a/src/nsterm.h
+++ b/src/nsterm.h
@@ -724,6 +724,7 @@ #define NSTRACE_UNSILENCE()
    int em_whole;
    }
 
+- (void) mark;
 - (instancetype) initFrame: (NSRect )r window: (Lisp_Object)win;
 - (void)setFrame: (NSRect)r;
 
@@ -1373,4 +1374,7 @@ #define NSControlStateValueOff NSOffState
 #define NSBezelStyleRounded NSRoundedBezelStyle
 #define NSButtonTypeMomentaryPushIn NSMomentaryPushInButton
 #endif
+
+extern void mark_nsterm (void);
+
 #endif	/* HAVE_NS */
diff --git a/src/nsterm.m b/src/nsterm.m
index 514b790b15..ad50630763 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -9909,6 +9909,16 @@ -(bool)judge
   return ret;
 }
 
+- (void) mark
+{
+  if (window)
+    {
+      Lisp_Object win;
+      XSETWINDOW (win, window);
+      mark_object (win);
+    }
+}
+
 
 - (void)resetCursorRects
 {
@@ -10650,6 +10660,26 @@ Convert an X font name (XLFD) to an NS font name.
   return ret;
 }
 
+void
+mark_nsterm (void)
+{
+  NSTRACE ("mark_nsterm");
+  Lisp_Object tail, frame;
+  FOR_EACH_FRAME (tail, frame)
+    {
+      struct frame *f = XFRAME (frame);
+      if (FRAME_NS_P (f))
+	{
+	  NSArray *subviews = [[FRAME_NS_VIEW (f) superview] subviews];
+	  for (int i = [subviews count] - 1; i >= 0; --i)
+	    {
+	      id scroller = [subviews objectAtIndex: i];
+	      if ([scroller isKindOfClass: [EmacsScroller class]])
+                  [scroller mark];
+	    }
+	}
+    }
+}
 
 void
 syms_of_nsterm (void)
-- 
2.36.1

bug#56095: 29.0.50; nsterm.m, use after free

[Eli Zaretskii]

> Cc: 56095@debbugs.gnu.org
> Date: Tue, 21 Jun 2022 16:25:19 +0200
> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> 
> As an aside, if anyone has heard anything about building GDB on an M1 Mac, I'd be grateful for a pointer. I'm
> asking because I'm currently stuck with lldb on my machine.  GDB doesn't seem to build - it doesn't produce
> an executable, but apparently, if I'm not blind, without saying what the problem is.

Which version of GDB?  If you didn't try the latest v12.1, please do.
If you did, and it still gives you trouble, suggest to ask a question
on the GDB mailing list (gdb@sourceware.org), I think someone of the
GDB development team actively works (or worked?) on the macOS port,
and might be able to help you.  In any case, that's where the GDB
developers are, so they might have some good advice regardless.

bug#56095: 29.0.50; nsterm.m, use after free

[Gerd Möllmann]

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

On 21. Jun 2022, 17:43 +0200, Eli Zaretskii <eliz@gnu.org>, wrote:
> > Which version of GDB? If you didn't try the latest v12.1, please do.
> >
It's v12.1.
> > If you did, and it still gives you trouble, suggest to ask a question
> > on the GDB mailing list (gdb@sourceware.org),

Thanks, I'll try that.

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

bug#56095: 29.0.50; nsterm.m, use after free

[Gerd Möllmann]

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

On 22. Jun 2022, 07:26 +0200, Gerd Möllmann <gerd.moellmann@gmail.com>, wrote:
> > > If you did, and it still gives you trouble, suggest to ask a question
> > > on the GDB mailing list (gdb@sourceware.org),
>
> Thanks, I'll try that.
Got the reply that lldb is the way to go on macOS.

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

bug#56095: 29.0.50; nsterm.m, use after free

[Eli Zaretskii]

> Date: Wed, 22 Jun 2022 11:19:56 +0200
> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: luangruo@yahoo.com, 56095@debbugs.gnu.org
> 
> On 22. Jun 2022, 07:26 +0200, Gerd Möllmann <gerd.moellmann@gmail.com>, wrote:
> 
>  If you did, and it still gives you trouble, suggest to ask a question
>  on the GDB mailing list (gdb@sourceware.org), 
> 
>  Thanks, I'll try that.
> 
> Got the reply that lldb is the way to go on macOS.

Ah, that platform is not even supported...

Unfortunately, using LLDB for debugging Emacs means you won't be able
to use the commands defined in src/.gdbinit, which makes examining
Lisp data very inconvenient, to say the least.  Unless, that is, you
or someone else rewrites those commands in some script LLDB supports;
no one has done that yet, AFAIK.

bug#56095: 29.0.50; nsterm.m, use after free

[Gerd Möllmann]

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

On 22. Jun 2022, 15:21 +0200, Eli Zaretskii <eliz@gnu.org>, wrote:
> >
> > Ah, that platform is not even supported...
> >
> > Unfortunately, using LLDB for debugging Emacs means you won't be able
> > to use the commands defined in src/.gdbinit, which makes examining
> > Lisp data very inconvenient, to say the least.
Yeah, to say the least :-).
> > Unless, that is, you
> > or someone else rewrites those commands in some script LLDB supports;
> > no one has done that yet, AFAIK.
Looks like that.  I couldn't find anything on the web either.

I see LDB has Python scripting.  Maybe it's not so hard after all, then.

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

bug#56095: 29.0.50; nsterm.m, use after free

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

> Please find a patch for that attached.

LGTM, thanks.

bug#56095: 29.0.50; nsterm.m, use after free

[Eli Zaretskii]

> Cc: 56095@debbugs.gnu.org
> Date: Wed, 22 Jun 2022 09:30:08 +0800
> From:  Po Lu via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
> 
> > Please find a patch for that attached.
> 
> LGTM, thanks.

Thanks, I installed this on master (after adding the bug number to the
commit log message).

Btw, Gerd, I think you still have write access to the project's
repository, so in principle you could install changes yourself.
(If that doesn't work, let me know.)

bug#56095: 29.0.50; nsterm.m, use after free

[Gerd Möllmann]

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

On 22. Jun 2022, 15:53 +0200, Eli Zaretskii <eliz@gnu.org>, wrote:
> > Cc: 56095@debbugs.gnu.org
> > Date: Wed, 22 Jun 2022 09:30:08 +0800
> > From: Po Lu via "Bug reports for GNU Emacs,
> > the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> >
> > Gerd Möllmann <gerd.moellmann@gmail.com> writes:
> >
> > > Please find a patch for that attached.
> >
> > LGTM, thanks.
> >
> > Thanks, I installed this on master (after adding the bug number to the
> > commit log message).
> >
Thanks
> >
> > Btw, Gerd, I think you still have write access to the project's
> > repository, so in principle you could install changes yourself.
> > (If that doesn't work, let me know.)
For now I'd rather submit patches.  I'm so completely out of the loop for so long that I don't feel confident to do the right thing.

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

bug#56095: 29.0.50; nsterm.m, use after free

[Eli Zaretskii]

> Date: Wed, 22 Jun 2022 16:15:16 +0200
> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: 56095-done@debbugs.gnu.org
> 
>  Btw, Gerd, I think you still have write access to the project's
>  repository, so in principle you could install changes yourself.
>  (If that doesn't work, let me know.)
> 
> For now I'd rather submit patches.  I'm so completely out of the loop for so long that I don't feel confident to
> do the right thing.

Fair enough.

bug#56095: Patch

[Gerd Möllmann]

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

I'm now using the patch attached.

Why the window pointer  is needed in EmacsScroller at all escapes me, ATM. Someone who knows nsterm.c should probably check if it is needed.



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

[-- Attachment #2: patch --]
[-- Type: application/octet-stream, Size: 861 bytes --]

diff --git a/src/nsterm.m b/src/nsterm.m
index 891d52ea3f..514b790b15 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -9869,14 +9869,6 @@ - (void)setFrame: (NSRect)newRect
 - (void)dealloc
 {
   NSTRACE ("[EmacsScroller dealloc]");
-  if (window)
-    {
-      if (horizontal)
-        wset_horizontal_scroll_bar (window, Qnil);
-      else
-        wset_vertical_scroll_bar (window, Qnil);
-    }
-  window = 0;
   [super dealloc];
 }
 
@@ -9909,13 +9901,6 @@ -(bool)judge
       view = (EmacsView *)FRAME_NS_VIEW (frame);
       if (view != nil)
         view->scrollbarsNeedingUpdate++;
-      if (window)
-        {
-          if (horizontal)
-            wset_horizontal_scroll_bar (window, Qnil);
-          else
-            wset_vertical_scroll_bar (window, Qnil);
-        }
       window = 0;
       [self removeFromSuperview];
       [self release];

bug#56095: 29.0.50; nsterm.m, use after free

[Lars Ingebrigtsen]

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

> I'm now using the patch attached.
>
> Why the window pointer  is needed in EmacsScroller at all escapes me, ATM.
> Someone who knows nsterm.c should probably check if it is needed.

I've added Alan to the CCs; perhaps he knows.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no