bug#38564: 27.0.50; macOS "emacs -nw" git-gutter-mode segfault

bug#38564: 27.0.50; macOS "emacs -nw" git-gutter-mode segfault

[Mike Hamrick]

Hi there,

Here is how I got emacs to segfault and drop be back to the shell:
- compile emacs 27.0.50 under macOS
- install the git-gutter package
- set up an after-init-hook for global-git-gutter-mode
- emacs -nw /some/file/under/version/control

The relevant part of my init.el looks like:
> (use-package git-gutter
>   :ensure t
>   :init
>  (add-hook 'after-init-hook 'global-git-gutter-mode))

I built emacs with debugging symbols, and ran it in the lldb debugger,
here is the stack trace:

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x40)
    frame #0: 0x000000010416ab3e emacs`ns_lookup_indexed_color(idx=18446744073709551613, f=0x00007facab80e210) at nsterm.m:2097:64
   2094 NSColor *
   2095 ns_lookup_indexed_color (unsigned long idx, struct frame *f)
   2096 {
-> 2097   struct ns_color_table *color_table = FRAME_DISPLAY_INFO (f)->color_table;
   2098   if (idx < 1 || idx >= color_table->avail)
   2099     return nil;
   2100   return color_table->colors[idx];
Likely cause: f->output_data.tty[29]->display_info->terminal accessed 0x40
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x40)
  * frame #0: 0x000000010416ab3e emacs`ns_lookup_indexed_color(idx=18446744073709551613, f=0x00007facab80e210) at nsterm.m:2097:64
    frame #1: 0x000000010416b5fc emacs`ns_color_index_to_rgba(idx=-3, f=0x00007facab80e210) at nsterm.m:2299:9
    frame #2: 0x0000000103e88e23 emacs`extend_face_to_end_of_line(it=0x00007ffeebdf5800) at xdisp.c:21833:7
    frame #3: 0x0000000103e5783a emacs`display_line(it=0x00007ffeebdf5800, cursor_vpos=3) at xdisp.c:23478:4
    frame #4: 0x0000000103e5468b emacs`try_window(window=0x00007facab80e445, pos=(charpos = 1, bytepos = 1), flags=1) at xdisp.c:19005:11
    frame #5: 0x0000000103e92e6f emacs`redisplay_window(window=0x00007facab80e445, just_this_one_p=false) at xdisp.c:18426:8
    frame #6: 0x0000000103e90f8d emacs`redisplay_window_0(window=0x00007facab80e445) at xdisp.c:16147:5
    frame #7: 0x000000010406083a emacs`internal_condition_case_1(bfun=(emacs`redisplay_window_0 at xdisp.c:16145), arg=0x00007facab80e445, handlers=0x00000001060f9d6b, hfun=(emacs`redisplay_window_error at xdisp.c:16138)) at eval.c:1379:25
    frame #8: 0x0000000103e8f87b emacs`redisplay_windows(window=0x00007facab80e445) at xdisp.c:16127:4
    frame #9: 0x0000000103e4f012 emacs`redisplay_internal at xdisp.c:15595:5
    frame #10: 0x0000000103e53110 emacs`redisplay_preserve_echo_area(from_where=2) at xdisp.c:15948:5
    frame #11: 0x0000000103e0914a emacs`Fredisplay(force=0x0000000000000000) at dispnew.c:6066:3
    frame #12: 0x0000000104068003 emacs`funcall_subr(subr=0x0000000104209680, numargs=0, args=0x00007ffeebdfc398) at eval.c:2867:19
    frame #13: 0x0000000104066e24 emacs`Ffuncall(nargs=1, args=0x00007ffeebdfc390) at eval.c:2794:11
    frame #14: 0x00000001040d7ef8 emacs`exec_byte_code(bytestr=0x0000000105c2125c, vector=0x0000000105c21015, maxdepth=0x000000000000001e, args_template=0x0000000000000c06, nargs=1, args=0x00007ffeebdfcad0) at bytecode.c:633:12
    frame #15: 0x000000010406847c emacs`funcall_lambda(fun=0x0000000105c20fe5, nargs=1, arg_vector=0x00007ffeebdfcac8) at eval.c:2989:11
    frame #16: 0x0000000104066e6e emacs`Ffuncall(nargs=2, args=0x00007ffeebdfcac0) at eval.c:2796:11
    frame #17: 0x00000001040d7ef8 emacs`exec_byte_code(bytestr=0x0000000105d67b44, vector=0x0000000105d67645, maxdepth=0x000000000000003e, args_template=0x0000000000000c06, nargs=3, args=0x00007ffeebdfd478) at bytecode.c:633:12
    frame #18: 0x000000010406847c emacs`funcall_lambda(fun=0x0000000105d664ed, nargs=3, arg_vector=0x00007ffeebdfd460) at eval.c:2989:11
    frame #19: 0x0000000104066e6e emacs`Ffuncall(nargs=4, args=0x00007ffeebdfd458) at eval.c:2796:11
    frame #20: 0x0000000104055bda emacs`Ffuncall_interactively(nargs=4, args=0x00007ffeebdfd458) at callint.c:254:32
    frame #21: 0x0000000104067f2b emacs`funcall_subr(subr=0x0000000104212f20, numargs=4, args=0x00007ffeebdfd458) at eval.c:2847:12
    frame #22: 0x0000000104066e24 emacs`Ffuncall(nargs=5, args=0x00007ffeebdfd450) at eval.c:2794:11
    frame #23: 0x0000000104066c04 emacs`Fapply(nargs=3, args=0x00007ffeebdfdc70) at eval.c:2424:24
    frame #24: 0x000000010405600e emacs`Fcall_interactively(function=0x000000000173af88, record_flag=0x0000000000000000, keys=0x00000001064ffb35) at callint.c:342:36
    frame #25: 0x000000010406805e emacs`funcall_subr(subr=0x0000000104212ef0, numargs=3, args=0x00007ffeebdfde40) at eval.c:2872:19
    frame #26: 0x0000000104066e24 emacs`Ffuncall(nargs=4, args=0x00007ffeebdfde38) at eval.c:2794:11
    frame #27: 0x00000001040d7ef8 emacs`exec_byte_code(bytestr=0x0000000105d1d5fc, vector=0x0000000105d1d09d, maxdepth=0x0000000000000036, args_template=0x0000000000001006, nargs=1, args=0x00007ffeebdfe5a8) at bytecode.c:633:12
    frame #28: 0x000000010406847c emacs`funcall_lambda(fun=0x0000000105d1d06d, nargs=1, arg_vector=0x00007ffeebdfe5a0) at eval.c:2989:11
    frame #29: 0x0000000104066e6e emacs`Ffuncall(nargs=2, args=0x00007ffeebdfe598) at eval.c:2796:11
    frame #30: 0x000000010406794f emacs`call1(fn=0x0000000000003ae0, arg1=0x000000000173af88) at eval.c:2654:10
    frame #31: 0x0000000103f643e9 emacs`command_loop_1 at keyboard.c:1458:13
    frame #32: 0x000000010406077f emacs`internal_condition_case(bfun=(emacs`command_loop_1 at keyboard.c:1236), handlers=0x0000000000000090, hfun=(emacs`cmd_error at keyboard.c:919)) at eval.c:1355:25
    frame #33: 0x0000000103f7bbcc emacs`command_loop_2(ignore=0x0000000000000000) at keyboard.c:1091:11
    frame #34: 0x00000001040600ea emacs`internal_catch(tag=0x000000000000c8a0, func=(emacs`command_loop_2 at keyboard.c:1087), arg=0x0000000000000000) at eval.c:1116:25
    frame #35: 0x0000000103f62ee8 emacs`command_loop at keyboard.c:1070:2
    frame #36: 0x0000000103f62d20 emacs`recursive_edit_1 at keyboard.c:714:9
    frame #37: 0x0000000103f630b9 emacs`Frecursive_edit at keyboard.c:786:3
    frame #38: 0x0000000103f60531 emacs`main(argc=3, argv=0x00007ffeebdfed58) at emacs.c:2054:3
    frame #39: 0x00007fff5cf7e3d5 libdyld.dylib`start + 1
    frame #40: 0x00007fff5cf7e3d5 libdyld.dylib`start + 1

I was able to prevent emacs from crashing with this rather naive patch.

diff --git a/src/nsterm.m b/src/nsterm.m
index c415159890..ea2b141d95 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -2094,6 +2094,8 @@ so some key presses (TAB) are swallowed by the system.  */
 NSColor *
 ns_lookup_indexed_color (unsigned long idx, struct frame *f)
 {
+  if (FRAME_DISPLAY_INFO (f) == nil)
+    return nil;
   struct ns_color_table *color_table = FRAME_DISPLAY_INFO (f)->color_table;
   if (idx < 1 || idx >= color_table->avail)
     return nil;

Here is more information about my setup post patch:

In GNU Emacs 27.0.50 (build 2, x86_64-apple-darwin18.7.0, NS appkit-1671.60 Version 10.14.6 (Build 18G1012))
 of 2019-12-10 built on st-mikeh1
Repository revision: ea93326cc046cb1beb7535cdf6d69b216b767685
Repository branch: master
System Description:  Mac OS X 10.14.6

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.

Configured using:
 'configure --prefix=/Users/mikeh'

Configured features:
RSVG GLIB NOTIFY KQUEUE ACL GNUTLS LIBXML2 ZLIB TOOLKIT_SCROLL_BARS NS
MODULES THREADS JSON PDUMPER LCMS2

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Fundamental

Minor modes in effect:
  global-git-gutter-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
format-spec rfc822 mml mml-sec epa derived epg epg-config gnus-util
rmail rmail-loaddefs text-property-search time-date mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils term/xterm
xterm git-gutter advice cl-extra help-mode use-package-ensure
use-package-core finder-inf info package easymenu browse-url
url-handlers url-parse auth-source cl-seq eieio eieio-core cl-macs
eieio-loaddefs password-cache json subr-x map url-vars seq byte-opt gv
bytecomp byte-compile cconv cl-loaddefs cl-lib tooltip eldoc electric
uniquify ediff-hook vc-hooks lisp-float-type mwheel term/ns-win ns-win
ucs-normalize mule-util term/common-win tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode elisp-mode
lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow isearch
timer select scroll-bar mouse jit-lock font-lock syntax facemenu
font-core term/tty-colors frame minibuffer cl-generic cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese eucjp-ms cp51932 hebrew greek romanian slovak czech european
ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray
cl-preloaded nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote threads kqueue cocoa ns
lcms2 multi-tty make-network-process emacs)

Memory information:
((conses 16 80357 7586)
 (symbols 48 9267 1)
 (strings 32 27500 1423)
 (string-bytes 1 893518)
 (vectors 16 12896)
 (vector-slots 8 135599 5856)
 (floats 8 39 323)
 (intervals 56 179 0)
 (buffers 1000 12))

bug#38564: 27.0.50; macOS "emacs -nw" git-gutter-mode segfault

[Robert Pluim]

>>>>> On Tue, 10 Dec 2019 17:33:37 -0800, Mike Hamrick <mikeh@muppetlabs.com> said:

    Mike> Hi there,
    Mike> Here is how I got emacs to segfault and drop be back to the shell:
    Mike> - compile emacs 27.0.50 under macOS
    Mike> - install the git-gutter package
    Mike> - set up an after-init-hook for global-git-gutter-mode
    Mike> - emacs -nw /some/file/under/version/control

Iʼm surprised nobody's seen this before.

    Mike> The relevant part of my init.el looks like:
    >> (use-package git-gutter
    >> :ensure t
    >> :init
    >> (add-hook 'after-init-hook 'global-git-gutter-mode))

    Mike>   * frame #0: 0x000000010416ab3e emacs`ns_lookup_indexed_color(idx=18446744073709551613, f=0x00007facab80e210) at nsterm.m:2097:64
    Mike>     frame #1: 0x000000010416b5fc emacs`ns_color_index_to_rgba(idx=-3, f=0x00007facab80e210) at nsterm.m:2299:9

    Mike> diff --git a/src/nsterm.m b/src/nsterm.m
    Mike> index c415159890..ea2b141d95 100644
    Mike> --- a/src/nsterm.m
    Mike> +++ b/src/nsterm.m
    Mike> @@ -2094,6 +2094,8 @@ so some key presses (TAB) are swallowed by the system.  */
    Mike>  NSColor *
    Mike>  ns_lookup_indexed_color (unsigned long idx, struct frame *f)
    Mike>  {
    Mike> +  if (FRAME_DISPLAY_INFO (f) == nil)
    Mike> +    return nil;
    Mike>    struct ns_color_table *color_table = FRAME_DISPLAY_INFO (f)->color_table;
    Mike>    if (idx < 1 || idx >= color_table->avail)
    Mike>      return nil;

Right idea, but one layer lower than needed. ns_color_index_to_rgba
should not be calling ns_lookup_indexed_color in non-gui mode. Try
this instead:

diff --git a/src/nsterm.m b/src/nsterm.m
index 52a9830be8..814a090370 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -2290,19 +2290,24 @@ so some key presses (TAB) are swallowed by the system.  */
 
 /* Convert an index into the color table into an RGBA value.  Used in
    xdisp.c:extend_face_to_end_of_line when comparing faces and frame
-   color values.  */
+   color values.  No-op on non-gui frames*/
 
 unsigned long
 ns_color_index_to_rgba(int idx, struct frame *f)
 {
-  NSColor *col;
-  col = ns_lookup_indexed_color (idx, f);
+  if (FRAME_DISPLAY_INFO (f))
+    {
+      NSColor *col;
+      col = ns_lookup_indexed_color (idx, f);
 
-  EmacsCGFloat r, g, b, a;
-  [col getRed: &r green: &g blue: &b alpha: &a];
+      EmacsCGFloat r, g, b, a;
+      [col getRed: &r green: &g blue: &b alpha: &a];
 
-  return ARGB_TO_ULONG((int)(a*255),
-                       (int)(r*255), (int)(g*255), (int)(b*255));
+      return ARGB_TO_ULONG((int)(a*255),
+                           (int)(r*255), (int)(g*255), (int)(b*255));
+    }
+  else
+    return idx;
 }
 
 void

bug#38564: 27.0.50; macOS "emacs -nw" git-gutter-mode segfault

[Mike Hamrick]

Robert Pluim writes:
> Iʼm surprised nobody's seen this before.

I was able to find a couple of references to the bug that were reported
downstream.

https://github.com/d12frosted/homebrew-emacs-plus/issues/112
https://github.com/hlissner/doom-emacs/issues/1170

> Right idea, but one layer lower than needed. ns_color_index_to_rgba
> should not be calling ns_lookup_indexed_color in non-gui mode. Try
> this instead:

Your patch does indeed fix the bug for me! It also explains why this
issue doesn't exist in 26.2, as the ns_color_index_to_rgba function was
added a little over a year ago. I'm a bit shocked at how quickly my bug
report reached someone intimately familiar with the underlying code.

bug#38564: 27.0.50; macOS "emacs -nw" git-gutter-mode segfault

[Robert Pluim]

tags 38564 fixed
close 38564 27.1
quit

>>>>> On Wed, 11 Dec 2019 07:15:56 -0800, Mike Hamrick <mikeh@muppetlabs.com> said:

    Mike> Robert Pluim writes:
    >> Iʼm surprised nobody's seen this before.

    Mike> I was able to find a couple of references to the bug that were reported
    Mike> downstream.

    Mike> https://github.com/d12frosted/homebrew-emacs-plus/issues/112
    Mike> https://github.com/hlissner/doom-emacs/issues/1170

Ah. Iʼm a bit sad they didnʼt report that back to us. Adding it to
'known upstream bugs' on github doesnʼt really help get things fixed.

    >> Right idea, but one layer lower than needed. ns_color_index_to_rgba
    >> should not be calling ns_lookup_indexed_color in non-gui mode. Try
    >> this instead:

    Mike> Your patch does indeed fix the bug for me! It also explains why this
    Mike> issue doesn't exist in 26.2, as the ns_color_index_to_rgba function was
    Mike> added a little over a year ago. I'm a bit shocked at how quickly my bug
    Mike> report reached someone intimately familiar with the underlying code.

I added it, so as the guilty party Iʼm obligated to fix it forever :-)

Thanks for the testing, committed to master as ea84a95bd8 , closing the bug.

Robert

bug#38564: 27.0.50; macOS "emacs -nw" git-gutter-mode segfault

[Alan Third]

On Wed, Dec 11, 2019 at 11:12:41AM +0100, Robert Pluim wrote:
> Right idea, but one layer lower than needed. ns_color_index_to_rgba
> should not be calling ns_lookup_indexed_color in non-gui mode. Try
> this instead:

Hi Robert, thanks for sorting this. One thing...

> +  if (FRAME_DISPLAY_INFO (f))

Would it not be better to use FRAME_NS_P?
-- 
Alan Third

bug#38564: 27.0.50; macOS "emacs -nw" git-gutter-mode segfault

[Robert Pluim]

>>>>> On Wed, 11 Dec 2019 19:37:34 +0000, Alan Third <alan@idiocy.org> said:

    Alan> On Wed, Dec 11, 2019 at 11:12:41AM +0100, Robert Pluim wrote:
    >> Right idea, but one layer lower than needed. ns_color_index_to_rgba
    >> should not be calling ns_lookup_indexed_color in non-gui mode. Try
    >> this instead:

    Alan> Hi Robert, thanks for sorting this. One thing...

    >> +  if (FRAME_DISPLAY_INFO (f))

    Alan> Would it not be better to use FRAME_NS_P?

Yes. In fact, how about reverting my change to ns_color_index_to_rgba
and doing this instead (you canʼt use FRAME_DISPLAY_INFO in
dispextern.h without much bigger changes):

diff --git i/src/dispextern.h w/src/dispextern.h
index 0615b16d71..4bf9f39cd0 100644
--- i/src/dispextern.h
+++ w/src/dispextern.h
@@ -123,7 +123,9 @@ #define NativeRectangle XRectangle

 #ifdef HAVE_NS
 #include "nsgui.h"
-#define FACE_COLOR_TO_PIXEL(face_color, frame) ns_color_index_to_rgba(face_color, frame)
+#define FACE_COLOR_TO_PIXEL(face_color, frame) (FRAME_NS_P (frame) \
+                                                ? ns_color_index_to_rgba (face_color, frame) \
+                                                : face_color)
 /* Following typedef needed to accommodate the MSDOS port, believe it or not.  */
 typedef struct ns_display_info Display_Info;
 typedef Emacs_Pixmap Emacs_Pix_Container;

bug#38564: 27.0.50; macOS "emacs -nw" git-gutter-mode segfault

[Mike Hamrick]

Heya folks,

I believe I'm still experiencing a version of this same bug. I'm not
always able to reproduce it unfortunately, but it often happens when
viewing git logs in Magit with 'll'. Again, terminal frame with the 'ns'
version of emacs 27.

In GNU Emacs 27.0.50 (build 6, x86_64-apple-darwin18.7.0, NS appkit-1671.60 Version 10.14.6 (Build 18G2022))
 of 2019-12-18 built on st-mikeh1
Repository revision: d55f2f74f53910c4416be1e023771dc3a8142727
Repository branch: master
System Description:  Mac OS X 10.14.6

Here's my stack trace:

frame #0: 0x0000000104038b3e Emacs`ns_lookup_indexed_color(idx=18446744073709551613, f=0x00007fb0c801ae10) at nsterm.m:2097:64
   2094 NSColor *
   2095 ns_lookup_indexed_color (unsigned long idx, struct frame *f)
   2096 {
-> 2097   struct ns_color_table *color_table = FRAME_DISPLAY_INFO (f)->color_table;
   2098   if (idx < 1 || idx >= color_table->avail)
   2099     return nil;
   2100   return color_table->colors[idx];
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xffffffff00000044)
  * frame #0: 0x0000000104038b3e Emacs`ns_lookup_indexed_color(idx=18446744073709551613, f=0x00007fb0c801ae10) at nsterm.m:2097:64
    frame #1: 0x0000000104039615 Emacs`ns_color_index_to_rgba(idx=-3, f=0x00007fb0c801ae10) at nsterm.m:2301:13
    frame #2: 0x0000000103d576f3 Emacs`extend_face_to_end_of_line(it=0x00007ffeebf29110) at xdisp.c:21896:7
    frame #3: 0x0000000103d25afa Emacs`display_line(it=0x00007ffeebf29110, cursor_vpos=4) at xdisp.c:23478:4
    frame #4: 0x0000000103d2294b Emacs`try_window(window=0x00007fb09aa19705, pos=(charpos = 1, bytepos = 1), flags=1) at xdisp.c:19005:11
    frame #5: 0x0000000103d6112f Emacs`redisplay_window(window=0x00007fb09aa19705, just_this_one_p=false) at xdisp.c:18426:8
    frame #6: 0x0000000103d5f24d Emacs`redisplay_window_0(window=0x00007fb09aa19705) at xdisp.c:16147:5
    frame #7: 0x0000000103f2e83a Emacs`internal_condition_case_1(bfun=(Emacs`redisplay_window_0 at xdisp.c:16145), arg=0x00007fb09aa19705, handlers=0x0000000105fe3e9b, hfun=(Emacs`redisplay_window_error at xdisp.c:16138)) at eval.c:1379:25
    frame #8: 0x0000000103d5db3b Emacs`redisplay_windows(window=0x00007fb09aa19705) at xdisp.c:16127:4
    frame #9: 0x0000000103d5daea Emacs`redisplay_windows(window=0x00007fb09aa194f5) at xdisp.c:16121:2
    frame #10: 0x0000000103d1d2d2 Emacs`redisplay_internal at xdisp.c:15595:5
    frame #11: 0x0000000103d21349 Emacs`redisplay at xdisp.c:14822:3
    frame #12: 0x0000000103e379d5 Emacs`read_char(commandflag=1, map=0x00007fb0999ae4e3, prev_event=0x0000000000000000, used_mouse_menu=0x00007ffeebf3054f, end_time=0x0000000000000000) at keyboard.c:2488:6
    frame #13: 0x0000000103e337e9 Emacs`read_key_sequence(keybuf=0x00007ffeebf30850, prompt=0x0000000000000000, dont_downcase_last=false, can_return_switch_frame=true, fix_current_buffer=true, prevent_redisplay=false) at keyboard.c:9536:12
    frame #14: 0x0000000103e3220a Emacs`command_loop_1 at keyboard.c:1345:15
    frame #15: 0x0000000103f2e77f Emacs`internal_condition_case(bfun=(Emacs`command_loop_1 at keyboard.c:1236), handlers=0x0000000000000090, hfun=(Emacs`cmd_error at keyboard.c:919)) at eval.c:1355:25
    frame #16: 0x0000000103e49e9c Emacs`command_loop_2(ignore=0x0000000000000000) at keyboard.c:1091:11
    frame #17: 0x0000000103f2e0ea Emacs`internal_catch(tag=0x000000000000c870, func=(Emacs`command_loop_2 at keyboard.c:1087), arg=0x0000000000000000) at eval.c:1116:25
    frame #18: 0x0000000103e311b8 Emacs`command_loop at keyboard.c:1070:2
    frame #19: 0x0000000103e30ff0 Emacs`recursive_edit_1 at keyboard.c:714:9
    frame #20: 0x0000000103e31389 Emacs`Frecursive_edit at keyboard.c:786:3
    frame #21: 0x0000000103e2e801 Emacs`main(argc=3, argv=0x00007ffeebf30e08) at emacs.c:2054:3
    frame #22: 0x00007fff631363d5 libdyld.dylib`start + 1
    frame #23: 0x00007fff631363d5 libdyld.dylib`start + 1

bug#38564: 27.0.50; macOS "emacs -nw" git-gutter-mode segfault

[Alan Third]

On Thu, Dec 19, 2019 at 10:03:53AM -0800, Mike Hamrick wrote:
> Heya folks,
> 
> I believe I'm still experiencing a version of this same bug. I'm not
> always able to reproduce it unfortunately, but it often happens when
> viewing git logs in Magit with 'll'. Again, terminal frame with the 'ns'
> version of emacs 27.

Robert hasn’t pushed his updated version of this fix, and it now
occurs to me that he may have been waiting for me to review it...
-- 
Alan Third

bug#38564: 27.0.50; macOS "emacs -nw" git-gutter-mode segfault

[Alan Third]

On Thu, Dec 12, 2019 at 08:45:11AM +0100, Robert Pluim wrote:
> >>>>> On Wed, 11 Dec 2019 19:37:34 +0000, Alan Third <alan@idiocy.org> said:
> 
>     Alan> On Wed, Dec 11, 2019 at 11:12:41AM +0100, Robert Pluim wrote:
>     >> Right idea, but one layer lower than needed. ns_color_index_to_rgba
>     >> should not be calling ns_lookup_indexed_color in non-gui mode. Try
>     >> this instead:
> 
>     Alan> Hi Robert, thanks for sorting this. One thing...
> 
>     >> +  if (FRAME_DISPLAY_INFO (f))
> 
>     Alan> Would it not be better to use FRAME_NS_P?
> 
> Yes. In fact, how about reverting my change to ns_color_index_to_rgba
> and doing this instead (you canʼt use FRAME_DISPLAY_INFO in
> dispextern.h without much bigger changes):

Hi Robert, I’m not sure if you were waiting for me, but I’m happy with
either approach.

-- 
Alan Third

bug#38564: 27.0.50; macOS "emacs -nw" git-gutter-mode segfault

[Robert Pluim]

>>>>> On Thu, 19 Dec 2019 19:14:16 +0000, Alan Third <alan@idiocy.org> said:

    Alan> On Thu, Dec 19, 2019 at 10:03:53AM -0800, Mike Hamrick wrote:
    >> Heya folks,
    >> 
    >> I believe I'm still experiencing a version of this same bug. I'm not
    >> always able to reproduce it unfortunately, but it often happens when
    >> viewing git logs in Magit with 'll'. Again, terminal frame with the 'ns'
    >> version of emacs 27.

    Alan> Robert hasn’t pushed his updated version of this fix, and it now
    Alan> occurs to me that he may have been waiting for me to review it...

I was, but I thought the previous version should work as well. Hmm, is
FRAME_DISPLAY_INFO guaranteed to be initialized?

Mike, could you try the following?

diff --git i/src/dispextern.h w/src/dispextern.h
index 0615b16d71..4bf9f39cd0 100644
--- i/src/dispextern.h
+++ w/src/dispextern.h
@@ -123,7 +123,9 @@ #define NativeRectangle XRectangle
 
 #ifdef HAVE_NS
 #include "nsgui.h"
-#define FACE_COLOR_TO_PIXEL(face_color, frame) ns_color_index_to_rgba(face_color, frame)
+#define FACE_COLOR_TO_PIXEL(face_color, frame) (FRAME_NS_P (frame) \
+                                                ? ns_color_index_to_rgba (face_color, frame) \
+                                                : face_color)
 /* Following typedef needed to accommodate the MSDOS port, believe it or not.  */
 typedef struct ns_display_info Display_Info;
 typedef Emacs_Pixmap Emacs_Pix_Container;
diff --git i/src/nsterm.m w/src/nsterm.m
index 6995577920..c415159890 100644
--- i/src/nsterm.m
+++ w/src/nsterm.m
@@ -2290,24 +2290,19 @@ so some key presses (TAB) are swallowed by the system.  */
 
 /* Convert an index into the color table into an RGBA value.  Used in
    xdisp.c:extend_face_to_end_of_line when comparing faces and frame
-   color values.  No-op on non-gui frames.  */
+   color values.  */
 
 unsigned long
 ns_color_index_to_rgba(int idx, struct frame *f)
 {
-  if (FRAME_DISPLAY_INFO (f))
-    {
-      NSColor *col;
-      col = ns_lookup_indexed_color (idx, f);
+  NSColor *col;
+  col = ns_lookup_indexed_color (idx, f);
 
-      EmacsCGFloat r, g, b, a;
-      [col getRed: &r green: &g blue: &b alpha: &a];
+  EmacsCGFloat r, g, b, a;
+  [col getRed: &r green: &g blue: &b alpha: &a];
 
-      return ARGB_TO_ULONG((int)(a*255),
-                           (int)(r*255), (int)(g*255), (int)(b*255));
-    }
-  else
-    return idx;
+  return ARGB_TO_ULONG((int)(a*255),
+                       (int)(r*255), (int)(g*255), (int)(b*255));
 }
 
 void

bug#38564: 27.0.50; macOS "emacs -nw" git-gutter-mode segfault

[Mike Hamrick]

Robert Pluim writes:
> I was, but I thought the previous version should work as well. Hmm, is
> FRAME_DISPLAY_INFO guaranteed to be initialized?

The stack trace I posted on Thursday the 19th, was built from d55f2f7,
which is a number of commits after your commit ea84a95 which updated
ns_color_index_to_rbga() to test for a non-null FRAME_DISPLAY_INFO
pointer.

I think the problem here is that FRAME_DISPLAY_INFO references
f->output_data which is a union.

#define FRAME_DISPLAY_INFO(f) ((f)->output_data.ns->display_info)

  union output_data
  {
    struct tty_output *tty;     /* From termchar.h.  */
    struct x_output *x;         /* From xterm.h.  */
    struct w32_output *w32;     /* From w32term.h.  */
    struct ns_output *ns;       /* From nsterm.h.  */
  }

In the case of a tty frame, we're going to be dereferencing a struct
tty_output *, rather than a struct ns_ouput *, and when that happens
sometimes the "display_info" offset into the tty_output struct may
or my not luckily land on patch of NULL bytes.

Robert Pluim also wrote:
> Mike, could you try the following?

Yup, I'm running with that patch now. This should stop the function
extend_face_to_end_of_line() from ever calling ns_color_index_to_rgba()
when you're in a tty frame, which I think will fix this for good.

I've taken to running my emacs 27 server process in the debugger, so if I
get any more crashes I should have more stack traces for ya.

Thanks!

Mike

bug#38564: 27.0.50; macOS "emacs -nw" git-gutter-mode segfault

[Robert Pluim]

tags 38564 fixed
close 38564 27.1
quit

>>>>> On Fri, 20 Dec 2019 12:21:40 -0800, Mike Hamrick <mikeh@muppetlabs.com> said:
    Mike> Yup, I'm running with that patch now. This should stop the function
    Mike> extend_face_to_end_of_line() from ever calling ns_color_index_to_rgba()
    Mike> when you're in a tty frame, which I think will fix this for good.

Iʼve now pushed this to the emacs-27 branch. Closing.
Committed as 81b697d106.

Robert