bug#65193: 29.1.50; SET_FRAME_ICONIFIED can cause Lisp evaluation inside read_socket_hook.

bug#65193: 29.1.50; SET_FRAME_ICONIFIED can cause Lisp evaluation inside read_socket_hook.

[YAMAMOTO Mitsuharu]

SET_FRAME_ICONIFIED has a call to gui_consider_frame_title and it can
cause Lisp evaluation if icon-title-format or frame-title-format
contains (:eval FORM).  This is problematic because
SET_FRAME_ICONIFIED can be called inside read_socket_hook.

The call to gui_consider_frame_title was introduced by the following
change:

commit e99f41f03a97641ee05ba4a27f8b91c190f55df1
Author: Po Lu <luangruo@yahoo.com>
Date:   Thu Jun 9 21:48:19 2022 +0800

   Fix recalculation of `icon-title-format' after a frame is iconified

   Previously it would only happen upon the next mode line
   redisplay, meaning that just pressing C-z would not update the
   implicit title, since C-z doesn't cause a redisplay.

   * src/dispextern.h: Update prototypes.
   * src/frame.h (SET_FRAME_ICONIFIED): De-slugify.  Call
   `gui_consider_frame_title', since `icon-title-format' might be
   different from the current frame title.
   * src/xdisp.c (gui_consider_frame_title): Export (also in
   dispextern.h).  (bug#55850)

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

bug#65193: 29.1.50; SET_FRAME_ICONIFIED can cause Lisp evaluation inside read_socket_hook.

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

YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:

> SET_FRAME_ICONIFIED has a call to gui_consider_frame_title and it can
> cause Lisp evaluation if icon-title-format or frame-title-format
> contains (:eval FORM).  This is problematic because
> SET_FRAME_ICONIFIED can be called inside read_socket_hook.
>
> The call to gui_consider_frame_title was introduced by the following
> change:

What practical problems have resulted from this change?

There are already several situations under which Lisp can be called
inside read_socket_hook.  And in principle, anything that performs GC
can in turn call finalizers that subsequently run Lisp.

bug#65193: 29.1.50; SET_FRAME_ICONIFIED can cause Lisp evaluation inside read_socket_hook.

[YAMAMOTO Mitsuharu]

On Thu, 10 Aug 2023 17:26:00 +0900,
Po Lu wrote:
> 
> YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:
> 
> > SET_FRAME_ICONIFIED has a call to gui_consider_frame_title and it can
> > cause Lisp evaluation if icon-title-format or frame-title-format
> > contains (:eval FORM).  This is problematic because
> > SET_FRAME_ICONIFIED can be called inside read_socket_hook.
> >
> > The call to gui_consider_frame_title was introduced by the following
> > change:
> 
> What practical problems have resulted from this change?

To my understanding, the Lisp evaluator is not reentrant, and thus
Lisp evaluation inside read_socket_hook, which can be called from
fairly arbitrary places via unblock_input/maybe_quit, causes
hard-to-reproduce, spontaneous crashes.

> There are already several situations under which Lisp can be called
> inside read_socket_hook.

That's really surprising.  I've once heard we shouldn't do that.  Is
that changed?

> And in principle, anything that performs GC can in turn call
> finalizers that subsequently run Lisp.

My understanding is that GC can only be triggered by eval_sub or
Ffuncall calls (except explicit garbage_collect calls), but not by
Lisp object allocations, for example.  Avoiding Lisp evaluation inside
read_socket_hook also means avoiding GC inside read_socket_hook.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

bug#65193: 29.1.50; SET_FRAME_ICONIFIED can cause Lisp evaluation inside read_socket_hook.

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

YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:

> To my understanding, the Lisp evaluator is not reentrant, and thus
> Lisp evaluation inside read_socket_hook, which can be called from
> fairly arbitrary places via unblock_input/maybe_quit, causes
> hard-to-reproduce, spontaneous crashes.

Only if that Lisp is permitted to signal out of those arbitrary places,
since we no longer call read_socket_hook directly from signal handlers.
Mode line format evaluation catches all non-local exits, AFAIK.

> That's really surprising.  I've once heard we shouldn't do that.  Is
> that changed?

I think so, see above.

> My understanding is that GC can only be triggered by eval_sub or
> Ffuncall calls (except explicit garbage_collect calls), but not by
> Lisp object allocations, for example.  Avoiding Lisp evaluation inside
> read_socket_hook also means avoiding GC inside read_socket_hook.

Hmm.  But given that we haven't avoided Lisp evaluation within
read_socket_hook for some time now, that's still a moot point.

bug#65193: 29.1.50; SET_FRAME_ICONIFIED can cause Lisp evaluation inside read_socket_hook.

[YAMAMOTO Mitsuharu]

On Thu, 10 Aug 2023 22:03:31 +0900,
Po Lu wrote:
> 
> YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:
> 
> > To my understanding, the Lisp evaluator is not reentrant, and thus
> > Lisp evaluation inside read_socket_hook, which can be called from
> > fairly arbitrary places via unblock_input/maybe_quit, causes
> > hard-to-reproduce, spontaneous crashes.
> 
> Only if that Lisp is permitted to signal out of those arbitrary places,
> since we no longer call read_socket_hook directly from signal handlers.
> Mode line format evaluation catches all non-local exits, AFAIK.
>
> > That's really surprising.  I've once heard we shouldn't do that.  Is
> > that changed?
> 
> I think so, see above.

I quote an old post by Stefan Monnier (author of SYNC_INPUT) below:

> SYNC_INPUT doesn't make much difference here: indeed the code is not
> executed from the signal handler any more, but it's still handled at
> fairly arbitrary points in the code, many of whom do not allow execution
> of elisp code.  So w.r.t execution of elisp code from read_socket_hook,
> it's still a big no-no.
(https://lists.gnu.org/r/emacs-devel/2008-03/msg01090.html)

What made Lisp evaluation inside read_socket_hook possible since then?

At least it can cause some corruption/inconsistency unless every
unblock_input/maybe_quit takes account of potential GC (including
string compaction).  Long-standing code would originally be written
without such a consideration.  Is it already audited thoroughly?

> > My understanding is that GC can only be triggered by eval_sub or
> > Ffuncall calls (except explicit garbage_collect calls), but not by
> > Lisp object allocations, for example.  Avoiding Lisp evaluation inside
> > read_socket_hook also means avoiding GC inside read_socket_hook.
> 
> Hmm.  But given that we haven't avoided Lisp evaluation within
> read_socket_hook for some time now, that's still a moot point.

Since which version?

Probably the possibility of encountering the problem would be quite
low and even if it happens, it is really difficult to reproduce, let
alone to identify the root cause.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

bug#65193: 29.1.50; SET_FRAME_ICONIFIED can cause Lisp evaluation inside read_socket_hook.

[YAMAMOTO Mitsuharu]

Another problem specific to the NS port w.r.t. the call to
gui_consider_frame_title from SET_FRAME_ICONIFIED is that it can
also be called inside the select emulation via -[EmacsView
windowDidDeminiaturize:] for example.

With multiple Lisp threads, select causes thread switching.  So, Lisp
evaluations can simultaneously happen both on the main thread (via
-[EmacsView windowDidDeminiaturize:]) and on another Lisp thread.
This breaks the cooperative nature of the Emacs threads.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

bug#65193: 29.1.50; SET_FRAME_ICONIFIED can cause Lisp evaluation inside read_socket_hook.

[Eli Zaretskii]

> Cc: 65193@debbugs.gnu.org
> Date: Fri, 11 Aug 2023 01:33:41 +0900
> From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> 
> Another problem specific to the NS port w.r.t. the call to
> gui_consider_frame_title from SET_FRAME_ICONIFIED is that it can
> also be called inside the select emulation via -[EmacsView
> windowDidDeminiaturize:] for example.

Please describe in more detail how such a call could happen, including
the sequence of calls which could call SET_FRAME_ICONIFIED from the
select emulation.  This situation, if it can happen, should be
investigated.

Thanks.

bug#65193: 29.1.50; SET_FRAME_ICONIFIED can cause Lisp evaluation inside read_socket_hook.

[YAMAMOTO Mitsuharu]

On Fri, 11 Aug 2023 02:58:50 +0900,
Eli Zaretskii wrote:
> 
> > Cc: 65193@debbugs.gnu.org
> > Date: Fri, 11 Aug 2023 01:33:41 +0900
> > From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> > 
> > Another problem specific to the NS port w.r.t. the call to
> > gui_consider_frame_title from SET_FRAME_ICONIFIED is that it can
> > also be called inside the select emulation via -[EmacsView
> > windowDidDeminiaturize:] for example.
> 
> Please describe in more detail how such a call could happen, including
> the sequence of calls which could call SET_FRAME_ICONIFIED from the
> select emulation.  This situation, if it can happen, should be
> investigated.

Sorry, I thought the NS port can run another Lisp thread while one
thread is waiting for inputs with (emulated) select as in other
platforms (including the Mac port).  But actually that's not the case,
so the situation I said does not happen with the current NS select
emulation.  Of course, once the NS port obtains such ability, the
problem I described will happen.

Below is the stacktrace that leads to SET_FRAME_ICONIFIED from the NS
select emulation (ns_select):

* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 2.1
  * frame #0: 0x0000000100f06f40 Emacs`SET_FRAME_ICONIFIED(f=0x0000000123904aa0, i=0) at frame.h:1300:3
    frame #1: 0x0000000100f3eae0 Emacs`-[EmacsView windowDidDeminiaturize:](self=0x0000000107891800, _cmd="windowDidDeminiaturize:", sender=@"NSWindowDidDeminiaturizeNotification") at nsterm.m:8140:3
    frame #2: 0x000000018d8b25c4 CoreFoundation`__CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ + 28
    frame #3: 0x000000018d9545a0 CoreFoundation`___CFXRegistrationPost_block_invoke + 52
    frame #4: 0x000000018d95450c CoreFoundation`_CFXRegistrationPost + 456
    frame #5: 0x000000018d88107c CoreFoundation`_CFXNotificationPost + 720
    frame #6: 0x000000018e60e370 Foundation`-[NSNotificationCenter postNotificationName:object:userInfo:] + 64
    frame #7: 0x000000019045724c AppKit`-[NSWindow(NSWindow_Theme) showDeminiaturizedWindow] + 164
    frame #8: 0x000000019096aecc AppKit`___NSWindowAddDockMessageHandlers_block_invoke_3 + 256
    frame #9: 0x000000019023879c AppKit`_NSCGSDockMessageReceive + 1616
    frame #10: 0x00000001958104f0 HIToolbox`DockCallback(unsigned int, unsigned int, void*, void*) + 1532
    frame #11: 0x0000000192af7484 HIServices`_DCXMaximizedWindows + 56
    frame #12: 0x0000000192af7444 HIServices`_XMaximizedWindows + 128
    frame #13: 0x0000000192ae8354 HIServices`mshMIGPerform + 200
    frame #14: 0x000000018d8be5e8 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE1_PERFORM_FUNCTION__ + 60
    frame #15: 0x000000018d8be4a4 CoreFoundation`__CFRunLoopDoSource1 + 596
    frame #16: 0x000000018d8bc924 CoreFoundation`__CFRunLoopRun + 2372
    frame #17: 0x000000018d8bb868 CoreFoundation`CFRunLoopRunSpecific + 600
    frame #18: 0x00000001957e8b40 HIToolbox`RunCurrentEventLoopInMode + 292
    frame #19: 0x00000001957e88b4 HIToolbox`ReceiveNextEventCommon + 552
    frame #20: 0x00000001957e8674 HIToolbox`_BlockUntilNextEventMatchingListInModeWithFilter + 72
    frame #21: 0x00000001900b2028 AppKit`_DPSNextEvent + 836
    frame #22: 0x00000001900b09c8 AppKit`-[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 1292
    frame #23: 0x00000001900a2814 AppKit`-[NSApplication run] + 596
    frame #24: 0x0000000100f17fd8 Emacs`-[EmacsApp run](self=0x0000000107700040, _cmd="run") at nsterm.m:5823:7
    frame #25: 0x0000000100f115ac Emacs`ns_select_1(nfds=0, readfds=0x000000016fdf24e0, writefds=0x000000016fdf2580, exceptfds=0x0000000000000000, timeout=0x000000016fdf2650, sigmask=0x0000000000000000, run_loop_only=NO) at nsterm.m:4838:3
    frame #26: 0x0000000100f10e74 Emacs`ns_select(nfds=0, readfds=0x000000016fdf24e0, writefds=0x000000016fdf2580, exceptfds=0x0000000000000000, timeout=0x000000016fdf2650, sigmask=0x0000000000000000) at nsterm.m:4890:10
    frame #27: 0x0000000100d029ac Emacs`wait_reading_process_output(time_limit=30, nsecs=0, read_kbd=-1, do_display=true, wait_for_cell=(i = 0x0000000000000000), wait_proc=0x0000000000000000, just_wait_proc=0) at process.c:5689:18
    frame #28: 0x0000000100027654 Emacs`sit_for(timeout=(i = 0x000000000000007a), reading=true, display_option=1) at dispnew.c:6264:7
    frame #29: 0x00000001006327b4 Emacs`read_char(commandflag=1, map=(i = 0x0000000106fdd243), prev_event=(i = 0x0000000000000000), used_mouse_menu=0x000000016fdf8ae0, end_time=0x0000000000000000) at keyboard.c:2881:11
    frame #30: 0x00000001006214ac Emacs`read_key_sequence(keybuf=0x000000016fdfb580, prompt=(i = 0x0000000000000000), dont_downcase_last=false, can_return_switch_frame=true, fix_current_buffer=true, prevent_redisplay=false) at keyboard.c:10083:12
    frame #31: 0x00000001006195fc Emacs`command_loop_1 at keyboard.c:1384:15
    frame #32: 0x0000000100aa1a50 Emacs`internal_condition_case(bfun=(Emacs`command_loop_1 at keyboard.c:1278), handlers=(i = 0x0000000000000090), hfun=(Emacs`cmd_error at keyboard.c:936)) at eval.c:1474:25
    frame #33: 0x000000010061789c Emacs`command_loop_2(handlers=(i = 0x0000000000000090)) at keyboard.c:1133:11
    frame #34: 0x0000000100a9e27c Emacs`internal_catch(tag=(i = 0x000000000000eac0), func=(Emacs`command_loop_2 at keyboard.c:1129), arg=(i = 0x0000000000000090)) at eval.c:1197:25
    frame #35: 0x0000000100612cdc Emacs`command_loop at keyboard.c:1111:2
    frame #36: 0x0000000100612060 Emacs`recursive_edit_1 at keyboard.c:720:9
    frame #37: 0x0000000100613f74 Emacs`Frecursive_edit at keyboard.c:803:3
    frame #38: 0x0000000100605848 Emacs`main(argc=2, argv=0x000000016fdff6a8) at emacs.c:2521:3
    frame #39: 0x000000018d7dd430 libdyld.dylib`start + 4

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

bug#65193: 29.1.50; SET_FRAME_ICONIFIED can cause Lisp evaluation inside read_socket_hook.

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

YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:

> What made Lisp evaluation inside read_socket_hook possible since then?

I don't know, but we've done it since then without problems.

> At least it can cause some corruption/inconsistency unless every
> unblock_input/maybe_quit takes account of potential GC (including
> string compaction).  Long-standing code would originally be written
> without such a consideration.  Is it already audited thoroughly?

Can you find any code which saves pointers to string data or buffer text
around calls to functions that might read input?

> Since which version?

I don't know.

> Probably the possibility of encountering the problem would be quite
> low and even if it happens, it is really difficult to reproduce, let
> alone to identify the root cause.

To the best of my knowledge, this is only a problem for the NS port.
Since other ports don't read input from their select emulations.

That is no great loss, given that the toolkit aborts after the first
call to redisplay from outside the main thread, so threads don't work
under NS anyway.

bug#65193: 29.1.50; SET_FRAME_ICONIFIED can cause Lisp evaluation inside read_socket_hook.

[Eli Zaretskii]

> Date: Fri, 11 Aug 2023 08:29:30 +0900
> From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> Cc: luangruo@yahoo.com,
> 	65193@debbugs.gnu.org
> 
> On Fri, 11 Aug 2023 02:58:50 +0900,
> Eli Zaretskii wrote:
> > 
> > > Cc: 65193@debbugs.gnu.org
> > > Date: Fri, 11 Aug 2023 01:33:41 +0900
> > > From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> > > 
> > > Another problem specific to the NS port w.r.t. the call to
> > > gui_consider_frame_title from SET_FRAME_ICONIFIED is that it can
> > > also be called inside the select emulation via -[EmacsView
> > > windowDidDeminiaturize:] for example.
> > 
> > Please describe in more detail how such a call could happen, including
> > the sequence of calls which could call SET_FRAME_ICONIFIED from the
> > select emulation.  This situation, if it can happen, should be
> > investigated.
> 
> Sorry, I thought the NS port can run another Lisp thread while one
> thread is waiting for inputs with (emulated) select as in other
> platforms (including the Mac port).  But actually that's not the case,
> so the situation I said does not happen with the current NS select
> emulation.  Of course, once the NS port obtains such ability, the
> problem I described will happen.
> 
> Below is the stacktrace that leads to SET_FRAME_ICONIFIED from the NS
> select emulation (ns_select):

OK, thanks.