bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Gerd Möllmann]

This is again on my local branch based on master.  Recent fixes for ASAN
are contained in that branch.  It seems to be pretty good at producing
this...

==19549==ERROR: AddressSanitizer: heap-use-after-free on address 0x0001393095c0 at pc 0x000100144340 bp 0x00016fdc16b0 sp 0x00016fdc16a8
READ of size 4 at 0x0001393095c0 thread T0
    #0 0x10014433c in gui_produce_glyphs xdisp.c:31875
    #1 0x1000a8bc0 in move_it_in_display_line_to xdisp.c:9813
    #2 0x10009a5c0 in move_it_to xdisp.c:10373
    #3 0x1000dcbac in move_it_vertically_backward xdisp.c:10745
    #4 0x100089ca4 in move_it_by_lines xdisp.c:10940
    #5 0x10055a7a4 in Fvertical_motion indent.c:2381
    #6 0x100642f20 in eval_sub eval.c:2488
    #7 0x10064440c in Fprogn eval.c:436
    #8 0x100618360 in Fsave_excursion editfns.c:886
    #9 0x1006424a0 in eval_sub eval.c:2435
    #10 0x100647ba4 in FletX eval.c:931
    #11 0x1006424a0 in eval_sub eval.c:2435
    #12 0x10064440c in Fprogn eval.c:436
    #13 0x1006424a0 in eval_sub eval.c:2435
    #14 0x10064af5c in Funwind_protect eval.c:1298
    #15 0x1006424a0 in eval_sub eval.c:2435
    #16 0x10064440c in Fprogn eval.c:436
    #17 0x1006185a4 in Fsave_current_buffer editfns.c:899
    #18 0x1006424a0 in eval_sub eval.c:2435
    #19 0x10064440c in Fprogn eval.c:436
    #20 0x100648d50 in Flet eval.c:1023
    #21 0x1006424a0 in eval_sub eval.c:2435
    #22 0x10064440c in Fprogn eval.c:436
    #23 0x100656f78 in funcall_lambda eval.c:3218
    #24 0x1006528f4 in apply_lambda eval.c:3088
    #25 0x100643d28 in eval_sub eval.c:2572
    #26 0x10064440c in Fprogn eval.c:436
    #27 0x1006424a0 in eval_sub eval.c:2435
    #28 0x1006441ac in Fif eval.c:391
    #29 0x1006424a0 in eval_sub eval.c:2435
    #30 0x10064440c in Fprogn eval.c:436
    #31 0x100656f78 in funcall_lambda eval.c:3218
    #32 0x100655384 in funcall_general eval.c:2941
    #33 0x10064a08c in Ffuncall eval.c:2979
    #34 0x100457288 in safe_run_hooks_1 keyboard.c:1829
    #35 0x10064cc80 in internal_condition_case_n eval.c:1555
    #36 0x100424970 in safe_run_hook_funcall keyboard.c:1887
    #37 0x100654690 in run_hook_with_args eval.c:2838
    #38 0x100424edc in safe_run_hooks_maybe_narrowed keyboard.c:1920
    #39 0x10041c68c in command_loop_1 keyboard.c:1511
    #40 0x10064c3d8 in internal_condition_case eval.c:1471
    #41 0x10041aacc in command_loop_2 keyboard.c:1123
    #42 0x10064ab64 in internal_catch eval.c:1194
    #43 0x100418ab8 in command_loop keyboard.c:1093
    #44 0x1004185cc in recursive_edit_1 keyboard.c:710
    #45 0x1004fa414 in read_minibuf minibuf.c:903
    #46 0x1004f7994 in Fread_from_minibuffer minibuf.c:1371
    #47 0x100643510 in eval_sub eval.c:2506
    #48 0x10064440c in Fprogn eval.c:436
    #49 0x1006424a0 in eval_sub eval.c:2435
    #50 0x10064af5c in Funwind_protect eval.c:1298
    #51 0x1006424a0 in eval_sub eval.c:2435
    #52 0x10064440c in Fprogn eval.c:436
    #53 0x100648d50 in Flet eval.c:1023
    #54 0x1006424a0 in eval_sub eval.c:2435
    #55 0x10064af5c in Funwind_protect eval.c:1298
    #56 0x1006424a0 in eval_sub eval.c:2435
    #57 0x10064440c in Fprogn eval.c:436
    #58 0x100648d50 in Flet eval.c:1023
    #59 0x1006424a0 in eval_sub eval.c:2435
    #60 0x10064440c in Fprogn eval.c:436
    #61 0x10064465c in Fcond eval.c:416
    #62 0x1006424a0 in eval_sub eval.c:2435
    #63 0x10064440c in Fprogn eval.c:436
    #64 0x1006480b4 in FletX eval.c:955
    #65 0x1006424a0 in eval_sub eval.c:2435
    #66 0x10064440c in Fprogn eval.c:436
    #67 0x1006185a4 in Fsave_current_buffer editfns.c:899
    #68 0x1006424a0 in eval_sub eval.c:2435
    #69 0x10064440c in Fprogn eval.c:436
    #70 0x100656f78 in funcall_lambda eval.c:3218
    #71 0x1006528f4 in apply_lambda eval.c:3088
    #72 0x100643d28 in eval_sub eval.c:2572
    #73 0x10064af5c in Funwind_protect eval.c:1298
    #74 0x1006424a0 in eval_sub eval.c:2435
    #75 0x10064440c in Fprogn eval.c:436
    #76 0x100648d50 in Flet eval.c:1023
    #77 0x1006424a0 in eval_sub eval.c:2435
    #78 0x10064be50 in internal_lisp_condition_case eval.c:1425
    #79 0x10064b18c in Fcondition_case eval.c:1340
    #80 0x1006424a0 in eval_sub eval.c:2435
    #81 0x10064af5c in Funwind_protect eval.c:1298
    #82 0x1006424a0 in eval_sub eval.c:2435
    #83 0x10064440c in Fprogn eval.c:436
    #84 0x100648d50 in Flet eval.c:1023
    #85 0x1006424a0 in eval_sub eval.c:2435
    #86 0x10064440c in Fprogn eval.c:436
    #87 0x100656f78 in funcall_lambda eval.c:3218
    #88 0x100655384 in funcall_general eval.c:2941
    #89 0x10064a08c in Ffuncall eval.c:2979
    #90 0x100653d68 in Fapply eval.c:2650
    #91 0x1006429d0 in eval_sub eval.c:2454
    #92 0x10064440c in Fprogn eval.c:436
    #93 0x1006441fc in Fif eval.c:392
    #94 0x1006424a0 in eval_sub eval.c:2435
    #95 0x10064440c in Fprogn eval.c:436
    #96 0x1006441fc in Fif eval.c:392
    #97 0x1006424a0 in eval_sub eval.c:2435
    #98 0x10064440c in Fprogn eval.c:436
    #99 0x100648d50 in Flet eval.c:1023
    #100 0x1006424a0 in eval_sub eval.c:2435
    #101 0x10064440c in Fprogn eval.c:436
    #102 0x100656f78 in funcall_lambda eval.c:3218
    #103 0x100655384 in funcall_general eval.c:2941
    #104 0x10064a08c in Ffuncall eval.c:2979
    #105 0x100653d68 in Fapply eval.c:2650
    #106 0x1006429d0 in eval_sub eval.c:2454
    #107 0x10064440c in Fprogn eval.c:436
    #108 0x1006424a0 in eval_sub eval.c:2435
    #109 0x1006441ac in Fif eval.c:391
    #110 0x1006424a0 in eval_sub eval.c:2435
    #111 0x10064440c in Fprogn eval.c:436
    #112 0x1006441fc in Fif eval.c:392
    #113 0x1006424a0 in eval_sub eval.c:2435
    #114 0x10064440c in Fprogn eval.c:436
    #115 0x100648d50 in Flet eval.c:1023
    #116 0x1006424a0 in eval_sub eval.c:2435
    #117 0x10064440c in Fprogn eval.c:436
    #118 0x100656f78 in funcall_lambda eval.c:3218
    #119 0x1006528f4 in apply_lambda eval.c:3088
    #120 0x100643d28 in eval_sub eval.c:2572
    #121 0x10064440c in Fprogn eval.c:436
    #122 0x100656f78 in funcall_lambda eval.c:3218
    #123 0x100655384 in funcall_general eval.c:2941
    #124 0x10064a08c in Ffuncall eval.c:2979
    #125 0x100635fbc in Ffuncall_interactively callint.c:248
    #126 0x1006564d4 in funcall_subr eval.c:3044
    #127 0x1006551dc in funcall_general eval.c:2925
    #128 0x10064a08c in Ffuncall eval.c:2979
    #129 0x100652d64 in Fapply eval.c:2603
    #130 0x100636ce8 in Fcall_interactively callint.c:340
    #131 0x100655b14 in funcall_subr eval.c:3021
    #132 0x100730088 in exec_byte_code bytecode.c:809
    #133 0x10065e22c in fetch_and_exec_byte_code eval.c:3066
    #134 0x100656a54 in funcall_lambda eval.c:3138
    #135 0x10065522c in funcall_general eval.c:2929
    #136 0x10064a08c in Ffuncall eval.c:2979
    #137 0x10042645c in call1 lisp.h:3313
    #138 0x10041c518 in command_loop_1 keyboard.c:1496
    #139 0x10064c3d8 in internal_condition_case eval.c:1471
    #140 0x10041aacc in command_loop_2 keyboard.c:1123
    #141 0x10064ab64 in internal_catch eval.c:1194
    #142 0x100418b64 in command_loop keyboard.c:1101
    #143 0x1004185cc in recursive_edit_1 keyboard.c:710
    #144 0x100419588 in Frecursive_edit keyboard.c:793
    #145 0x1004116c8 in main emacs.c:2521
    #146 0x101555088 in start+0x204 (dyld:arm64e+0x5088)

0x0001393095c0 is located 256 bytes inside of 296-byte region [0x0001393094c0,0x0001393095e8)
freed by thread T0 here:
    #0 0x1033f2de4 in wrap_free+0x98 (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x3ede4)
    #1 0x10098d5e0 in rpl_free free.c:48
    #2 0x1005af84c in xfree alloc.c:810
    #3 0x1003f32ac in free_realized_face xfaces.c:4511
    #4 0x1003e5e40 in free_realized_faces xfaces.c:4702
    #5 0x1003d4a6c in free_all_realized_faces xfaces.c:4742
    #6 0x1000cee18 in init_iterator xdisp.c:3193
    #7 0x1001001ac in gui_consider_frame_title xdisp.c:13497
    #8 0x1001d72cc in prepare_menu_bars xdisp.c:13612
    #9 0x1000f2c64 in redisplay_internal xdisp.c:16529
    #10 0x100109858 in redisplay xdisp.c:16111
    #11 0x100896f90 in -[EmacsView layoutSublayersOfLayer:] nsterm.m:8675
    #12 0x1900a9624 in CA::Layer::layout_if_needed(CA::Transaction*)+0x224 (QuartzCore:arm64e+0x20624)
    #13 0x1901f661c in CA::Context::commit_transaction(CA::Transaction*, double, double*)+0x1c0 (QuartzCore:arm64e+0x16d61c)
    #14 0x19008b4c8 in CA::Transaction::commit()+0x2bc (QuartzCore:arm64e+0x24c8)
    #15 0x18bee1698 in __62+[CATransaction(NSCATransaction) NS_setFlushesWithDisplayLink]_block_invoke+0x12c (AppKit:arm64e+0x1ac698)
    #16 0x18c646754 in ___NSRunLoopObserverCreateWithHandler_block_invoke+0x3c (AppKit:arm64e+0x911754)
    #17 0x1892101a0 in __CFRUNLOOP_IS_CALLING_OUT_TO_AN_OBSERVER_CALLBACK_FUNCTION__+0x20 (CoreFoundation:arm64e+0x841a0)
    #18 0x18920fff0 in __CFRunLoopDoObservers+0x24c (CoreFoundation:arm64e+0x83ff0)
    #19 0x18920f524 in __CFRunLoopRun+0x300 (CoreFoundation:arm64e+0x83524)
    #20 0x18920ea80 in CFRunLoopRunSpecific+0x254 (CoreFoundation:arm64e+0x82a80)
    #21 0x191e4e334 in RunCurrentEventLoopInMode+0x120 (HIToolbox:arm64e+0x32334)
    #22 0x191e4dfc0 in ReceiveNextEventCommon+0x140 (HIToolbox:arm64e+0x31fc0)
    #23 0x191e4de64 in _BlockUntilNextEventMatchingListInModeWithFilter+0x44 (HIToolbox:arm64e+0x31e64)
    #24 0x18bd76518 in _DPSNextEvent+0x358 (AppKit:arm64e+0x41518)
    #25 0x18bd74e10 in -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:]+0x52c (AppKit:arm64e+0x3fe10)
    #26 0x18bd66fdc in -[NSApplication run]+0x250 (AppKit:arm64e+0x31fdc)
    #27 0x1008744f4 in -[EmacsApp run] nsterm.m:5813
    #28 0x1008cb450 in ns_read_socket_1 nsterm.m:4693
    #29 0x1008b1e74 in ns_read_socket nsterm.m:4711

previously allocated by thread T0 here:
    #0 0x1033f2ca8 in wrap_malloc+0x94 (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x3eca8)
    #1 0x1005af4f4 in lmalloc alloc.c:1361
    #2 0x1005af40c in xmalloc alloc.c:751
    #3 0x1003f92b4 in make_realized_face xfaces.c:4471
    #4 0x1003f5c00 in realize_gui_face xfaces.c:6023
    #5 0x1003e4000 in realize_face xfaces.c:5954
    #6 0x1003e70fc in lookup_face xfaces.c:4890
    #7 0x1003eef98 in face_at_buffer_position xfaces.c:6641
    #8 0x1001a2d9c in face_at_pos xdisp.c:4499
    #9 0x10019ee18 in handle_face_prop xdisp.c:4600
    #10 0x100198810 in handle_stop xdisp.c:3947
    #11 0x1000d72e4 in reseat xdisp.c:7582
    #12 0x1000d7ab8 in reseat_at_previous_visible_line_start xdisp.c:7445
    #13 0x10008c204 in start_display xdisp.c:3581
    #14 0x1005592d8 in Fvertical_motion indent.c:2241
    #15 0x100642f20 in eval_sub eval.c:2488
    #16 0x10064440c in Fprogn eval.c:436
    #17 0x100618360 in Fsave_excursion editfns.c:886
    #18 0x1006424a0 in eval_sub eval.c:2435
    #19 0x100647ba4 in FletX eval.c:931
    #20 0x1006424a0 in eval_sub eval.c:2435
    #21 0x10064440c in Fprogn eval.c:436
    #22 0x1006424a0 in eval_sub eval.c:2435
    #23 0x10064af5c in Funwind_protect eval.c:1298
    #24 0x1006424a0 in eval_sub eval.c:2435
    #25 0x10064440c in Fprogn eval.c:436
    #26 0x1006185a4 in Fsave_current_buffer editfns.c:899
    #27 0x1006424a0 in eval_sub eval.c:2435
    #28 0x10064440c in Fprogn eval.c:436
    #29 0x100648d50 in Flet eval.c:1023

SUMMARY: AddressSanitizer: heap-use-after-free xdisp.c:31875 in gui_produce_glyphs
Shadow bytes around the buggy address:
  0x007027281260: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x007027281270: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x007027281280: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa
  0x007027281290: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x0070272812a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0070272812b0: fd fd fd fd fd fd fd fd[fd]fd fd fd fd fa fa fa
  0x0070272812c0: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x0070272812d0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0070272812e0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa
  0x0070272812f0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x007027281300: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==19549==ABORTING
(lldb) AddressSanitizer report breakpoint hit. Use 'thread info -s' to get extended information about the report.


The problem here, it seems to me, is that the redisplay done in
-[EmacsView layoutSublayersOfLayer:] nsterm.m:8675, frees realized faces
at a moment that the code doesn't cannot expect.

I'm too lazy too look further.  I'm pretty sure the story goes pretty
much like what we had before with relocating strings.

Is there a way to prevent freeing realized faces?

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Date: Thu, 06 Oct 2022 17:03:17 +0200
> 
> This is again on my local branch based on master.  Recent fixes for ASAN
> are contained in that branch.  It seems to be pretty good at producing
> this...
> 
> ==19549==ERROR: AddressSanitizer: heap-use-after-free on address 0x0001393095c0 at pc 0x000100144340 bp 0x00016fdc16b0 sp 0x00016fdc16a8
> READ of size 4 at 0x0001393095c0 thread T0
>     #0 0x10014433c in gui_produce_glyphs xdisp.c:31875
>     #1 0x1000a8bc0 in move_it_in_display_line_to xdisp.c:9813
>     #2 0x10009a5c0 in move_it_to xdisp.c:10373
>     #3 0x1000dcbac in move_it_vertically_backward xdisp.c:10745
>     #4 0x100089ca4 in move_it_by_lines xdisp.c:10940
>     #5 0x10055a7a4 in Fvertical_motion indent.c:2381

Sigh...

> The problem here, it seems to me, is that the redisplay done in
> -[EmacsView layoutSublayersOfLayer:] nsterm.m:8675, frees realized faces
> at a moment that the code doesn't cannot expect.

Right.

> I'm too lazy too look further.  I'm pretty sure the story goes pretty
> much like what we had before with relocating strings.
> 
> Is there a way to prevent freeing realized faces?

Yes: set inhibit_free_realized_faces non-zero (and record
unwind_protect to restore it).

It sounds like we need to do that in probably_quit, at least for NS
builds, because it could trigger redisplay, sigh...

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> Is there a way to prevent freeing realized faces?
>
> Yes: set inhibit_free_realized_faces non-zero (and record
> unwind_protect to restore it).

Thanks.

>
> It sounds like we need to do that in probably_quit, at least for NS
> builds, because it could trigger redisplay, sigh...

Right, sigh...  But it's getting easier to make sense of this weird
stuff.

Po Lu, is this also something for Haiku?

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: 58334@debbugs.gnu.org, Po Lu <luangruo@yahoo.com>
> Date: Thu, 06 Oct 2022 20:01:21 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Is there a way to prevent freeing realized faces?
> >
> > Yes: set inhibit_free_realized_faces non-zero (and record
> > unwind_protect to restore it).
> 
> Thanks.

Actually, I no longer think this will help, because redisplay_internal
sets inhibit_free_realized_faces to zero at the beginning...

Any way of figuring out which face is it that triggers the ASAN?  Is
it one of the basic faces, or some non-basic face?

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Gerd Möllmann]

On 22-10-06 20:30 , Eli Zaretskii wrote:
> Actually, I no longer think this will help, because redisplay_internal
> sets inhibit_free_realized_faces to zero at the beginning...

Yeah, I've seen the specbind right now.

> Any way of figuring out which face is it that triggers the ASAN?  Is
> it one of the basic faces, or some non-basic face?

I'm afraid no.  What about the idea to additionally check for inhibited 
GC?  That is, free faces only if not imhibit_free and not imhibit_gc?

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

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

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

> Po Lu, is this also something for Haiku?

Haiku calls Lisp in internal hooks (via safe_call) in
gui_produce_glyphs.  It does not call redisplay itself in input
callbacks during normal exxecution.

An exception is made when control over user input is transferred to
another GUI thread as part of a popup or dialog.  xselect.c and xmenu.c
do pretty much the same thing.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

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

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

>     #0 0x1033f2ca8 in wrap_malloc+0x94 (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x3eca8)
>     #1 0x1005af4f4 in lmalloc alloc.c:1361
>     #2 0x1005af40c in xmalloc alloc.c:751
>     #3 0x1003f92b4 in make_realized_face xfaces.c:4471
>     #4 0x1003f5c00 in realize_gui_face xfaces.c:6023
>     #5 0x1003e4000 in realize_face xfaces.c:5954

[...]

>     #14 0x1005592d8 in Fvertical_motion indent.c:2241

I'm pretty sure the right fix is to block input around realize_face and
Fvertical_motion, since that code is clearly not reentrant.

> The problem here, it seems to me, is that the redisplay done in
> -[EmacsView layoutSublayersOfLayer:] nsterm.m:8675, frees realized faces
> at a moment that the code doesn't cannot expect.

Also, how come layoutSublayersOfLayer is called so often?  AFAIU it's
only there to coax the system into actually resizing Emacs while the
system blocks the input loop from returning control to Emacs, which
should only happen during drag-to-resize.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Gerd Möllmann]

Po Lu <luangruo@yahoo.com> writes:

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>> Po Lu, is this also something for Haiku?
>
> Haiku calls Lisp in internal hooks (via safe_call) in
> gui_produce_glyphs.  It does not call redisplay itself in input
> callbacks during normal exxecution.
>
> An exception is made when control over user input is transferred to
> another GUI thread as part of a popup or dialog.  xselect.c and xmenu.c
> do pretty much the same thing.

So it can call redisplay in some cases, and we have to protect against
it?

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Gerd Möllmann]

Po Lu <luangruo@yahoo.com> writes:

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>>     #0 0x1033f2ca8 in wrap_malloc+0x94 (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x3eca8)
>>     #1 0x1005af4f4 in lmalloc alloc.c:1361
>>     #2 0x1005af40c in xmalloc alloc.c:751
>>     #3 0x1003f92b4 in make_realized_face xfaces.c:4471
>>     #4 0x1003f5c00 in realize_gui_face xfaces.c:6023
>>     #5 0x1003e4000 in realize_face xfaces.c:5954
>
> [...]
>
>>     #14 0x1005592d8 in Fvertical_motion indent.c:2241
>
> I'm pretty sure the right fix is to block input around realize_face and
> Fvertical_motion, since that code is clearly not reentrant.

If we can find one, I would prefer a broader solution, even if it is a
bit heavy-handed.  I'm a bit afraid of finding these problems piecemeal,
and it's getting a bit tiresome, but that's just me - why do I run with
ASAN...

>
>> The problem here, it seems to me, is that the redisplay done in
>> -[EmacsView layoutSublayersOfLayer:] nsterm.m:8675, frees realized faces
>> at a moment that the code doesn't cannot expect.
>
> Also, how come layoutSublayersOfLayer is called so often?  AFAIU it's
> only there to coax the system into actually resizing Emacs while the
> system blocks the input loop from returning control to Emacs, which
> should only happen during drag-to-resize.

I don't know.  Does it help if I describe what I did?

The backtrace I showed was from starting Emacs with my init file.  It
was busy with restoring desktop, I think, and at the point where frame
size and poisitino was restored (just a guess), it crashed.

Today, I got basically the same crash modulo Lisp frames in the
backtrace when finding a file in another frame.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Eli Zaretskii]

> Cc: 58334@debbugs.gnu.org
> Date: Fri, 07 Oct 2022 08:46:15 +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:
> 
> >     #0 0x1033f2ca8 in wrap_malloc+0x94 (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x3eca8)
> >     #1 0x1005af4f4 in lmalloc alloc.c:1361
> >     #2 0x1005af40c in xmalloc alloc.c:751
> >     #3 0x1003f92b4 in make_realized_face xfaces.c:4471
> >     #4 0x1003f5c00 in realize_gui_face xfaces.c:6023
> >     #5 0x1003e4000 in realize_face xfaces.c:5954
> 
> [...]
> 
> >     #14 0x1005592d8 in Fvertical_motion indent.c:2241
> 
> I'm pretty sure the right fix is to block input around realize_face and
> Fvertical_motion, since that code is clearly not reentrant.

Why isn't Fvertical_motion reentrant?

Anyway, the problem is not that realize_face was interrupted, the
problem is that the face realized above was later freed as a side
effect of calling redisplay.  And the display code (which is invoked
by Fvertical_motion) almost everywhere assumes that FACE_FROM_ID will
never yield a freed face, it just returns

   FRAME_FACE_CACHE (f)->faces_by_id[id]

without checking whether ID is beyond the limit of the frame's current
face cache.  The assertion there is not compiled in a production
build.  (Gerd, was your build with --enable-checking?)

So if the frame's face cache can be freed like that as a side effect
of maybe_quit, we'll have to introduce cache checking into
FACE_FROM_ID, and if the ID is not in the cache do whatever it takes
to correct the situation.

IOW, I don't see how block_input anywhere can solve this particular
problem.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

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

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

> So it can call redisplay in some cases, and we have to protect against
> it?

Yes.  But only inside a popup menu or drag-and-drop.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>  (Gerd, was your build with --enable-checking?)

No, it's with -g -O0, ASAN, and without native comp.

With enable-checking on top, it's so slow I can't stand it anymore.

> IOW, I don't see how block_input anywhere can solve this particular
> problem.

I wonder too.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Gerd Möllmann]

Po Lu <luangruo@yahoo.com> writes:

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>> So it can call redisplay in some cases, and we have to protect against
>> it?
>
> Yes.  But only inside a popup menu or drag-and-drop.

Thanks

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Gerd Möllmann]

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

> Eli Zaretskii <eliz@gnu.org> writes:
>> IOW, I don't see how block_input anywhere can solve this particular
>> problem.
>
> I wonder too.

And, while vaccuming, I also wondered what happens with the glyph
matrices, and maybe other global state?

I don't wana know.  Little kittens, little kittens, little
kittens... :-).

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

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

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

> And, while vaccuming, I also wondered what happens with the glyph
> matrices, and maybe other global state?

Isn't input blocked wherever the glyph matrices are modified?  If not,
how come expose_frame always works correctly?

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Gerd Möllmann]

Po Lu <luangruo@yahoo.com> writes:

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>> And, while vaccuming, I also wondered what happens with the glyph
>> matrices, and maybe other global state?
>
> Isn't input blocked wherever the glyph matrices are modified?
> If not,

Partly.  Eli please correct me if this has changed.

Redisplay has two phases:

1. Building desired matrices, that is, what should be on the screen
after redisplay.

2. Updating current matrices from desired matrices, and bringing that on
the screen.

Only phase 2 has input blocked because the current matrices are
modified, which are shared state with the GUI code.

> how come expose_frame always works correctly?

Expose uses only current matrices.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

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

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

> Redisplay has two phases:
>
> 1. Building desired matrices, that is, what should be on the screen
> after redisplay.

Well, at least there's this in redisplay_internal (I think you wrote
it):

  /* I don't think this happens but let's be paranoid.  */
  if (redisplaying_p) <=================================
    return;

So perhaps the right thing to do would be to replace the comment with
one saying that process_pending_signals can potentially cause redisplay
to be called within itself.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Po Lu <luangruo@yahoo.com>,  58334@debbugs.gnu.org
> Date: Fri, 07 Oct 2022 10:07:01 +0200
> 
> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
> 
> > Eli Zaretskii <eliz@gnu.org> writes:
> >> IOW, I don't see how block_input anywhere can solve this particular
> >> problem.
> >
> > I wonder too.
> 
> And, while vaccuming, I also wondered what happens with the glyph
> matrices, and maybe other global state?

Fvertical_motion (and other functions that call the move_it_*
functions) in general don't rely on glyph matrices.  So I'm not sure
what exactly worries you.

In any case, glyph matrices are kept as long as their windows are
kept.  They aren't "freed" like faces or images.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Gerd Möllmann]

Po Lu <luangruo@yahoo.com> writes:

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>> Redisplay has two phases:
>>
>> 1. Building desired matrices, that is, what should be on the screen
>> after redisplay.
>
> Well, at least there's this in redisplay_internal (I think you wrote
> it):
>
>   /* I don't think this happens but let's be paranoid.  */
>   if (redisplaying_p) <=================================
>     return;
>
> So perhaps the right thing to do would be to replace the comment with
> one saying that process_pending_signals can potentially cause redisplay
> to be called within itself.

I'd rather first understand what happens and why, which I don't.
Otherwise the comment would be no good.

So, do you agree that block_input wouldn't solve the problem?  Or does
it?

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  58334@debbugs.gnu.org
> Date: Fri, 07 Oct 2022 16:36:05 +0800
> 
> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
> 
> > And, while vaccuming, I also wondered what happens with the glyph
> > matrices, and maybe other global state?
> 
> Isn't input blocked wherever the glyph matrices are modified?

No, not in general.  Why would it?  We don't allow re-entering
redisplay anyway.

> If not, how come expose_frame always works correctly?

expose_frame doesn't modify glyph matrices, it only uses them,
i.e. accesses them in read-only fashion.  At least AFAIK, that is.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

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

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

> So, do you agree that block_input wouldn't solve the problem?  Or does
> it?

It should, because it prevents the read_socket_hook from being called.
However, you must keep in mind that anything that can call unblock_input
can also run redisplay, as unblock_input reads pending async input if
the input is completely unblocked.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  58334@debbugs.gnu.org
> Date: Fri, 07 Oct 2022 10:54:34 +0200
> 
> Po Lu <luangruo@yahoo.com> writes:
> 
> > Gerd Möllmann <gerd.moellmann@gmail.com> writes:
> >
> >> And, while vaccuming, I also wondered what happens with the glyph
> >> matrices, and maybe other global state?
> >
> > Isn't input blocked wherever the glyph matrices are modified?
> > If not,
> 
> Partly.  Eli please correct me if this has changed.
> 
> Redisplay has two phases:
> 
> 1. Building desired matrices, that is, what should be on the screen
> after redisplay.
> 
> 2. Updating current matrices from desired matrices, and bringing that on
> the screen.
> 
> Only phase 2 has input blocked because the current matrices are
> modified, which are shared state with the GUI code.

I don't see input blocked in phase 2, either.  We interrupt phase 2 if
input is pending, but even that only in some cases.

The GUI code runs in the same thread as phase 2, so there's no reason
to synchronize anything here, AFAIU.  If you are thinking about
expose_frame and friends, then that was changed years ago not to run
from the signal handler, so it cannot cause any problems to code that
modifies the glyph matrices.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  58334@debbugs.gnu.org
> Date: Fri, 07 Oct 2022 18:28:33 +0800
> 
> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
> 
> > Redisplay has two phases:
> >
> > 1. Building desired matrices, that is, what should be on the screen
> > after redisplay.
> 
> Well, at least there's this in redisplay_internal (I think you wrote
> it):
> 
>   /* I don't think this happens but let's be paranoid.  */
>   if (redisplaying_p) <=================================
>     return;
> 
> So perhaps the right thing to do would be to replace the comment with
> one saying that process_pending_signals can potentially cause redisplay
> to be called within itself.

You don't need process_pending_signals, it's enough that some hook
calls 'redisplay' from Lisp.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: Po Lu <luangruo@yahoo.com>,  58334@debbugs.gnu.org
>> Date: Fri, 07 Oct 2022 10:07:01 +0200
>> 
>> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>> 
>> > Eli Zaretskii <eliz@gnu.org> writes:
>> >> IOW, I don't see how block_input anywhere can solve this particular
>> >> problem.
>> >
>> > I wonder too.
>> 
>> And, while vaccuming, I also wondered what happens with the glyph
>> matrices, and maybe other global state?
>
> Fvertical_motion (and other functions that call the move_it_*
> functions) in general don't rely on glyph matrices.  So I'm not sure
> what exactly worries you.

I not yet worried, just wondering :-).

If we don't change some other shared state, then we're safe if we
prevent freeing faces?  That's would be good.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

> If you are thinking about expose_frame and friends, then that was
> changed years ago not to run from the signal handler, so it cannot
> cause any problems to code that modifies the glyph matrices.

Again what learned.  I was indeed thinking of that.  Thanks.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  58334@debbugs.gnu.org
> Date: Fri, 07 Oct 2022 19:19:53 +0800
> 
> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
> 
> > So, do you agree that block_input wouldn't solve the problem?  Or does
> > it?
> 
> It should, because it prevents the read_socket_hook from being called.
> However, you must keep in mind that anything that can call unblock_input
> can also run redisplay, as unblock_input reads pending async input if
> the input is completely unblocked.

IMNSHO, we cannot start blocking input left and right, because it will
make Emacs unresponsive.

I think a better alternative is to audit the uses of FACE_FROM_ID and
see what we can do to protect their callers from a situation where the
frame's face cache was freed since the face ID was obtained.  We could
even make the remedy be part of FACE_FROM_ID itself, so it will
"self-heal", so to speak.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Gerd Möllmann]

On 22-10-07 13:19 , Po Lu wrote:
> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
> 
>> So, do you agree that block_input wouldn't solve the problem?  Or does
>> it?
> 
> It should, because it prevents the read_socket_hook from being called.
> However, you must keep in mind that anything that can call unblock_input
> can also run redisplay, as unblock_input reads pending async input if
> the input is completely unblocked.

Thanks.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: luangruo@yahoo.com,  58334@debbugs.gnu.org
> Date: Fri, 07 Oct 2022 13:29:38 +0200
> 
> If we don't change some other shared state, then we're safe if we
> prevent freeing faces?  That's would be good.

Yes, I think so.  But preventing freeing the faces is a losing game,
in the long run, because we cannot prevent that forever without
adversely affecting the Emacs memory footprint.

I think a better way is to re-generate the faces when we discover they
were freed.  This is easy for the basic faces, but fundamentally
impossible for non-basic ones.  That's why I asked you earlier whether
the offending face was a basic one.  However, I think we can rely on
inhibit_free_realized_faces to avoid freeing non-basic faces, if we
use that flag in strategic places.  Basically, non-basic faces are
realized and cached by redisplay itself, so theoretically we should be
able to prevent their freeing (and perhaps we already have that in
place, see redisplay_internal).

So I would recommend to fix FACE_FROM_ID to re-generate the basic
faces if needed, on the assumption that the cases where we have
problems with using face ID are limited to basic faces.  If, after
that, we will find cases with non-basic faces, I'd first look for more
opportunities to use inhibit_free_realized_faces.

One other thing is that inhibit_free_realized_faces is a boolean, so
if nesting is possible, it cannot support such nesting; we'd need a
reference count instead.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Eli Zaretskii]

> Date: Thu, 6 Oct 2022 20:36:22 +0200
> Cc: 58334@debbugs.gnu.org, luangruo@yahoo.com
> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> 
> On 22-10-06 20:30 , Eli Zaretskii wrote:
> > Actually, I no longer think this will help, because redisplay_internal
> > sets inhibit_free_realized_faces to zero at the beginning...
> 
> Yeah, I've seen the specbind right now.
> 
> > Any way of figuring out which face is it that triggers the ASAN?  Is
> > it one of the basic faces, or some non-basic face?
> 
> I'm afraid no.  What about the idea to additionally check for inhibited 
> GC?  That is, free faces only if not imhibit_free and not imhibit_gc?

I don't see how "GC inhibited" is related, except by chance.  It
sounds wrong to conflate the two.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

> So I would recommend to fix FACE_FROM_ID to re-generate the basic
> faces if needed, on the assumption that the cases where we have
> problems with using face ID are limited to basic faces.  If, after
> that, we will find cases with non-basic faces, I'd first look for more
> opportunities to use inhibit_free_realized_faces.

Sigh, I'd rather do something easy, and continue with what I wanted to
try out in the branch here.  It has only gpt one commit so far, in 3 or
4 days.

> One other thing is that inhibit_free_realized_faces is a boolean, so
> if nesting is possible, it cannot support such nesting; we'd need a
> reference count instead.

Yes, that's why I asked if we get by with something like this:

diff --git a/src/xdisp.c b/src/xdisp.c
index 9534e27843..fd94509fe4 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -3179,7 +3179,8 @@ init_iterator (struct it *it, struct window *w,
      free realized faces now because they depend on face definitions
      that might have changed.  Don't free faces while there might be
      desired matrices pending which reference these faces.  */
-  if (!inhibit_free_realized_faces)
+  if (!inhibit_free_realized_faces
+      && !garbage_collection_inhibited)
     {
       if (face_change)
 	{

BTW, I've commented out the call to redisplay in nsterm.m
layoutSomething now in my branch.  Let's see what the effect is.  So far
I don't notice anything.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Gerd Möllmann]

On 22-10-07 14:01 , Eli Zaretskii wrote:
> I don't see how "GC inhibited" is related, except by chance.  It
> sounds wrong to conflate the two.

Couldn't we bind it in nsterm.m?

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: luangruo@yahoo.com,  58334@debbugs.gnu.org
> Date: Fri, 07 Oct 2022 14:01:49 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > So I would recommend to fix FACE_FROM_ID to re-generate the basic
> > faces if needed, on the assumption that the cases where we have
> > problems with using face ID are limited to basic faces.  If, after
> > that, we will find cases with non-basic faces, I'd first look for more
> > opportunities to use inhibit_free_realized_faces.
> 
> Sigh, I'd rather do something easy

I'm not sure there are any "easy" solutions to this.  We could keep
sprinkling inhibit_free_realized_faces some more (over the last years,
I added quite a few of them, and in other cases added calls to
realize_basic_faces).

> BTW, I've commented out the call to redisplay in nsterm.m
> layoutSomething now in my branch.  Let's see what the effect is.  So far
> I don't notice anything.

If Git history for the code which calls redisplay shows commits that
have references to bug reports in their log messages, perhaps read
those bug reports to see if they show recipes for problems that were
supposed to be fixed by adding that call.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Eli Zaretskii]

> Date: Fri, 7 Oct 2022 14:03:58 +0200
> Cc: 58334@debbugs.gnu.org, luangruo@yahoo.com
> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> 
> On 22-10-07 14:01 , Eli Zaretskii wrote:
> > I don't see how "GC inhibited" is related, except by chance.  It
> > sounds wrong to conflate the two.
> 
> Couldn't we bind it in nsterm.m?

Bind what?

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Gerd Möllmann]

On 22-10-07 14:06 , Eli Zaretskii wrote:
>> Date: Fri, 7 Oct 2022 14:03:58 +0200
>> Cc: 58334@debbugs.gnu.org, luangruo@yahoo.com
>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>>
>> On 22-10-07 14:01 , Eli Zaretskii wrote:
>>> I don't see how "GC inhibited" is related, except by chance.  It
>>> sounds wrong to conflate the two.
>>
>> Couldn't we bind it in nsterm.m?
> 
> Bind what?

int count = inhibit_garbahe_collection ();
redisplay ();
unbind_to...

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

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

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

> int count = inhibit_garbahe_collection ();
> redisplay ();
> unbind_to...

Why would you only inhibit garbage collection there?  What if some
finalizer function calls preedit text inside process_pending_signals?

Also, what about where we decode X preconversion text?

In the recent past, Emacs also used to run Lisp as part of the character
conversion of keyboard input, straight from handle_one_xevent:

	    if (nchars < nbytes)
	      {
		/* Decode the input data.  */

		/* The input should be decoded with `coding_system'
		   which depends on which X*LookupString function
		   we used just above and the locale.  */
		setup_coding_system (coding_system, &coding);
		coding.src_multibyte = false;
		coding.dst_multibyte = true;
		/* The input is converted to events, thus we can't
		   handle composition.  Anyway, there's no XIM that
		   gives us composition information.  */
		coding.common_flags &= ~CODING_ANNOTATION_MASK;

		SAFE_NALLOCA (coding.destination, MAX_MULTIBYTE_LENGTH,
			      nbytes);
		coding.dst_bytes = MAX_MULTIBYTE_LENGTH * nbytes;
		coding.mode |= CODING_MODE_LAST_BLOCK;
		decode_coding_c_string (&coding, copy_bufptr, nbytes, Qnil);
		nbytes = coding.produced;
		nchars = coding.produced_char;
		copy_bufptr = coding.destination;
	      }

How come that never caused problems?

Thanks.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

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

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

> BTW, I've commented out the call to redisplay in nsterm.m
> layoutSomething now in my branch.  Let's see what the effect is.  So far
> I don't notice anything.

It prevents drag-to-resize from working at all on the Mac OS system
where I tried.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Eli Zaretskii]

> Date: Fri, 7 Oct 2022 14:08:02 +0200
> Cc: 58334@debbugs.gnu.org, luangruo@yahoo.com
> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> 
> On 22-10-07 14:06 , Eli Zaretskii wrote:
> >> Date: Fri, 7 Oct 2022 14:03:58 +0200
> >> Cc: 58334@debbugs.gnu.org, luangruo@yahoo.com
> >> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> >>
> >> On 22-10-07 14:01 , Eli Zaretskii wrote:
> >>> I don't see how "GC inhibited" is related, except by chance.  It
> >>> sounds wrong to conflate the two.
> >>
> >> Couldn't we bind it in nsterm.m?
> > 
> > Bind what?
> 
> int count = inhibit_garbahe_collection ();
> redisplay ();
> unbind_to...

Could be dangerous, unless we also inhibit all the hooks that
redisplay can call, because who knows what arbitrary Lisp can do to
memory?  And some of the Lisp called by redisplay can't be easily
disabled.  Example: the :eval forms in the mode line.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

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

Eli Zaretskii <eliz@gnu.org> writes:

> You don't need process_pending_signals, it's enough that some hook
> calls 'redisplay' from Lisp.

process_pending_signals called from probably_quit and unblock_input
calling handle_async_input, and thus gobble_input, is what will run that
hook calling redisplay, right?  Since the unwanted execution of Lisp
happens inside read_socket_hook.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  58334@debbugs.gnu.org
> Date: Fri, 07 Oct 2022 20:12:58 +0800
> 
> In the recent past, Emacs also used to run Lisp as part of the character
> conversion of keyboard input, straight from handle_one_xevent:
> 
> 	    if (nchars < nbytes)
> 	      {
> 		/* Decode the input data.  */
> 
> 		/* The input should be decoded with `coding_system'
> 		   which depends on which X*LookupString function
> 		   we used just above and the locale.  */
> 		setup_coding_system (coding_system, &coding);
> 		coding.src_multibyte = false;
> 		coding.dst_multibyte = true;
> 		/* The input is converted to events, thus we can't
> 		   handle composition.  Anyway, there's no XIM that
> 		   gives us composition information.  */
> 		coding.common_flags &= ~CODING_ANNOTATION_MASK;
> 
> 		SAFE_NALLOCA (coding.destination, MAX_MULTIBYTE_LENGTH,
> 			      nbytes);
> 		coding.dst_bytes = MAX_MULTIBYTE_LENGTH * nbytes;
> 		coding.mode |= CODING_MODE_LAST_BLOCK;
> 		decode_coding_c_string (&coding, copy_bufptr, nbytes, Qnil);
> 		nbytes = coding.produced;
> 		nchars = coding.produced_char;
> 		copy_bufptr = coding.destination;
> 	      }
> 
> How come that never caused problems?

Why should it cause problems? what kind of problems?

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Gerd Möllmann]

On 22-10-07 14:14 , Po Lu wrote:
> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
> 
>> BTW, I've commented out the call to redisplay in nsterm.m
>> layoutSomething now in my branch.  Let's see what the effect is.  So far
>> I don't notice anything.
> 
> It prevents drag-to-resize from working at all on the Mac OS system
> where I tried.

Is drag-to-resize the "normal" resizing with the mouse?  That seems to 
work fine here (macOS 12.6).

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

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

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

> Is drag-to-resize the "normal" resizing with the mouse?

Yes.

> That seems to work fine here (macOS 12.6).

I tried on Mac OS 10.12.1.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

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

Eli Zaretskii <eliz@gnu.org> writes:

> Why should it cause problems? what kind of problems?

Okay, I guess I got this bug report messed up with the other bug
report(s) regarding the execution of Lisp inside read_socket_hook.

I thought I was replying to something in bug#58042.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: gerd.moellmann@gmail.com,  58334@debbugs.gnu.org
> Date: Fri, 07 Oct 2022 20:16:43 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > You don't need process_pending_signals, it's enough that some hook
> > calls 'redisplay' from Lisp.
> 
> process_pending_signals called from probably_quit and unblock_input
> calling handle_async_input, and thus gobble_input, is what will run that
> hook calling redisplay, right?

I meant that the problem is much wider and more general: any Lisp can
call redisplay at will.  And we call Lisp from C all over the place.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Gerd Möllmann]

On 22-10-07 14:14 , Eli Zaretskii wrote:
>> int count = inhibit_garbahe_collection ();
>> redisplay ();
>> unbind_to...
> 
> Could be dangerous, unless we also inhibit all the hooks that
> redisplay can call, because who knows what arbitrary Lisp can do to
> memory?  And some of the Lisp called by redisplay can't be easily
> disabled.  Example: the :eval forms in the mode line.

I don't care.  Then let them not write shitty Lisp :-).

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Gerd Möllmann]

On 22-10-07 14:22 , Po Lu wrote:
> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
> 
>> Is drag-to-resize the "normal" resizing with the mouse?
> 
> Yes.
> 
>> That seems to work fine here (macOS 12.6).
> 
> I tried on Mac OS 10.12.1.

Then I propose to make the call to redisplay in layout... conditional on 
OS version, and let's forget about it until a bug report comes in.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Gerd Möllmann]

I'd like to finish this one way or another, so here it goes...

The call to redisplay in nsterm.m was identified as the problem because
it can free realized faces, that other code relies on not to happen.

The call to redisplay was introduced in the commit below, and I'm
looking at the bugs mentioned in it.

68bd6f3ea9c05637501139c46f1f4304482db95f
Author:     Alan Third <alan@idiocy.org>
CommitDate: Sat Feb 13 22:41:25 2021 +0000

Fix flicker when resizing NS frame programmatically (bug#46155)
; Incidentally fixes bug#21326.

IÄm using Emacs master on macOS 12.6 with

modified   src/nsterm.m
@@ -8672,7 +8672,7 @@ - (void)layoutSublayersOfLayer:(CALayer *)layer
       waiting_for_input = 0;
       block_input ();
 
-      redisplay ();
+      //redisplay ();
 
       unblock_input ();
       waiting_for_input = owfi;


------------------------------------------------------------------------
bug#46155: 28.0.50; Regression: buffer contents flicker on macOS
------------------------------------------------------------------------

The bug complains that

(dotimes (n 10)
  (set-frame-parameter nil 'width (+ 80 n))
  (sit-for 0.1))

leads to flicker.  Not reproducible.

This funny comment by Alan made my day, so I'll quote it here :-)

    Well... I tried something ridiculous and it appears to work...

    I suspect forcing redisplay this way within the NS run loop is bad
    form, but it appears to work.

In the mails under bug#46155, someone mentions that Emacs displays an
empty buffer while busy on startup.  Reproducible.

That's all I checked.

------------------------------------------------------------------------
bug#21326: 24.5; OS X, frame blank while resizing
------------------------------------------------------------------------

Not reproducible.

------------------------------------------------------------------------
Possible solutions:

1. Don't call redisplay, depending on OS version.

2. What Eli said - make uses of realized faces resilient against face
cache clearing.

3. Block input in vertical-motion etc. as Po Lu suggested.

4. Do nothing because that's all esoteric.

I favour (1) or (4) because (2) I'm not motivated to do, and TBH I agree
with Alan's comment above :-).  Can't say much about (3), except that
Eli doesn't seem to like it.

bug#58334: 29.0.50; ASAN heap use after free in gui_produce_glyphs

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Date: Sat, 08 Oct 2022 08:58:02 +0200
> 
> Possible solutions:
> 
> 1. Don't call redisplay, depending on OS version.
> 
> 2. What Eli said - make uses of realized faces resilient against face
> cache clearing.
> 
> 3. Block input in vertical-motion etc. as Po Lu suggested.
> 
> 4. Do nothing because that's all esoteric.
> 
> I favour (1) or (4) because (2) I'm not motivated to do, and TBH I agree
> with Alan's comment above :-).  Can't say much about (3), except that
> Eli doesn't seem to like it.

If the choice should be 1 or 4, then I don't care much what you do,
because I don't use macOS.  FTR, both those alternatives sound bad to
me.  Though 1 seems slightly better, because it at least avoids
reading from unintialized memory, which could potentially lead to
crashes, and the OS versions where the call to redisplay matters are
relatively old (or so it sounds).