all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* [PATCH] Really prevent quitting in sensitive contexts
@ 2020-03-12  2:05 Daniel Colascione
  2020-03-12  2:20 ` Stefan Monnier
  0 siblings, 1 reply; 20+ messages in thread
From: Daniel Colascione @ 2020-03-12  2:05 UTC (permalink / raw)
  To: emacs-devel, dancol

Emacs has traditionally inhibited quitting in process filters and a
few other contexts to prevent stray user-typed C-g keystrokes from
interfering with "background" tasks like recording process output.
This quit inhibition is accomplished by binding inhibit-quit to t
around these sensitive regions.  The trouble is that code like
with-local-quit can temporarily bind inhibit-quit to nil inside code
running inside a process filter and thereby make "background" against
work sensitive to stray C-g keystrokes.

with-local-quit is actually documented to allow this sort of
foreground quitting, but since the interactive C-g race is
unavoidable, we should deprecate use of with-local-quit for this
purpose.  For compatibility, we can't remove it entirely.

This change adds a new inhibit-quit-override flag.  When non-nil,
inhibit-quit-override makes Emacs behave as if inhibit-quit were t
regardless of inhibit-quit's actual value.  Emacs sets
inhibit-quit-override inside process filters and other critical
regions and ensures that with-local-quit and other binders of
inhibit-quit don't accidentally allow C-g to influence these contexts.

* src/process.c (wait_reading_process_output): Check
Vinhibit_quit_override as well as Vinhibit_quit.
(read_and_dispose_of_process_output, exec_sentinel): Bind
Vinhibit_quit_override instead of Vinhibit_quit.
* src/lisp.h (QUITP): check Vinhibit_quit_override as well as
Vinhibit_quit.
* src/eval.c (maybe_quit): Use QUITP instead of open-coded check.
(Qinhibit_quit_override, Vinhibit_quit_override): Define.
* src/ccl.c (ccl_driver): Use QUITP instead of open-coded check.
* src/keyboard.c
(menu_bar_items,tab_bar_items,tool_bar_items,safe_run_hooks)
(timer_check_2): Set Vinhibit_quit_override instead of
Vinhibit_quit.
(handle_interrupt): Set Vinhibit_quit_override to nil
after ten quits.
---
 etc/NEWS       |  5 +++++
 src/ccl.c      |  2 +-
 src/eval.c     | 11 ++++++++++-
 src/keyboard.c | 32 +++++++++++++++++---------------
 src/lisp.h     |  4 +++-
 src/process.c  |  8 +++++---
 src/xdisp.c    | 10 +++++-----
 7 files changed, 46 insertions(+), 26 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index fcdf6dbe24..6181ef2c18 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -193,6 +193,11 @@ are 'eq'.  To compare contents, use 'compare-window-configurations'
 instead.  This change helps fix a bug in 'sxhash-equal', which returned
 incorrect hashes for window configurations and some other objects.
 
+** When the new 'inhibit-quit-override' variable is non-nil, Emacs
+behaves as if 'inhibit-quit' were t.  This change prevents
+with-local-quit from accidentally allowing quits inside process
+filters, redisplay callbacks, and other special contexts.
+
 ---
 ** The obsolete function 'thread-alive-p' has been removed.
 
diff --git a/src/ccl.c b/src/ccl.c
index ac44dc1f60..a495d5a4ca 100644
--- a/src/ccl.c
+++ b/src/ccl.c
@@ -893,7 +893,7 @@ ccl_driver (struct ccl_program *ccl, int *source, int *destination, int src_size
       ccl_backtrace_table[ccl_backtrace_idx] = 0;
 #endif
 
-      if (!NILP (Vquit_flag) && NILP (Vinhibit_quit))
+      if (QUITP)
 	{
 	  /* We can't just signal Qquit, instead break the loop as if
              the whole data is processed.  Don't reset Vquit_flag, it
diff --git a/src/eval.c b/src/eval.c
index 4559a0e1f6..bbfb3579d2 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -1539,7 +1539,7 @@ process_quit_flag (void)
 void
 maybe_quit (void)
 {
-  if (!NILP (Vquit_flag) && NILP (Vinhibit_quit))
+  if (QUITP)
     process_quit_flag ();
   else if (pending_signals)
     process_pending_signals ();
@@ -4062,8 +4062,17 @@ syms_of_eval (void)
 before making `inhibit-quit' nil.  */);
   Vinhibit_quit = Qnil;
 
+  DEFVAR_LISP ("inhibit-quit-override", Vinhibit_quit_override,
+	       doc: /* If t, consider inhibit-quit t even if it's set
+to nil.  Emacs uses this flag to prevent with-local-quit from
+temporarily allowing quits in places where we really don't want to
+allow quits at all, e.g., while running filters.  Never locally bind
+this symbol to nil!  */);
+  Vinhibit_quit_override = Qnil;
+
   DEFSYM (Qsetq, "setq");
   DEFSYM (Qinhibit_quit, "inhibit-quit");
+  DEFSYM (Qinhibit_quit_override, "inhibit-quit-override");
   DEFSYM (Qautoload, "autoload");
   DEFSYM (Qinhibit_debugger, "inhibit-debugger");
   DEFSYM (Qmacro, "macro");
diff --git a/src/keyboard.c b/src/keyboard.c
index 9ce168c6dd..ab2890b117 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -1834,7 +1834,7 @@ safe_run_hooks (Lisp_Object hook)
 {
   ptrdiff_t count = SPECPDL_INDEX ();
 
-  specbind (Qinhibit_quit, Qt);
+  specbind (Qinhibit_quit_override, Qt);
   run_hook_with_args (2, ((Lisp_Object []) {hook, hook}), safe_run_hook_funcall);
   unbind_to (count, Qnil);
 }
@@ -4337,7 +4337,7 @@ timer_check_2 (Lisp_Object timers, Lisp_Object idle_timers)
 		 code fails to reschedule it right.  */
 	      ASET (chosen_timer, 0, Qt);
 
-	      specbind (Qinhibit_quit, Qt);
+	      specbind (Qinhibit_quit_override, Qt);
 
 	      call1 (Qtimer_event_handler, chosen_timer);
 	      Vdeactivate_mark = old_deactivate_mark;
@@ -4382,8 +4382,8 @@ timer_check (void)
   struct timespec nexttime;
   Lisp_Object timers, idle_timers;
 
-  Lisp_Object tem = Vinhibit_quit;
-  Vinhibit_quit = Qt;
+  Lisp_Object tem = Vinhibit_quit_override;
+  Vinhibit_quit_override = Qt;
 
   /* We use copies of the timers' lists to allow a timer to add itself
      again, without locking up Emacs if the newly added timer is
@@ -4397,7 +4397,7 @@ timer_check (void)
   else
     idle_timers = Qnil;
 
-  Vinhibit_quit = tem;
+  Vinhibit_quit_override = tem;
 
   do
     {
@@ -7417,8 +7417,8 @@ menu_bar_items (Lisp_Object old)
      quitting while building the menus.
      We do this instead of specbind because (1) errors will clear it anyway
      and (2) this avoids risk of specpdl overflow.  */
-  oquit = Vinhibit_quit;
-  Vinhibit_quit = Qt;
+  oquit = Vinhibit_quit_override;
+  Vinhibit_quit_override = Qt;
 
   if (!NILP (old))
     menu_bar_items_vector = old;
@@ -7528,7 +7528,7 @@ menu_bar_items (Lisp_Object old)
     menu_bar_items_index = i;
   }
 
-  Vinhibit_quit = oquit;
+  Vinhibit_quit_override = oquit;
   SAFE_FREE ();
   return menu_bar_items_vector;
 }
@@ -7978,8 +7978,8 @@ tab_bar_items (Lisp_Object reuse, int *nitems)
      quitting while building the menus.  We do this instead of
      specbind because (1) errors will clear it anyway and (2) this
      avoids risk of specpdl overflow.  */
-  oquit = Vinhibit_quit;
-  Vinhibit_quit = Qt;
+  oquit = Vinhibit_quit_override;
+  Vinhibit_quit_override = Qt;
 
   /* Initialize tab_bar_items_vector and protect it from GC.  */
   init_tab_bar_items (reuse);
@@ -8037,7 +8037,7 @@ tab_bar_items (Lisp_Object reuse, int *nitems)
 	  map_keymap (keymap, process_tab_bar_item, Qnil, NULL, 1);
       }
 
-  Vinhibit_quit = oquit;
+  Vinhibit_quit_override = oquit;
   *nitems = ntab_bar_items / TAB_BAR_ITEM_NSLOTS;
   SAFE_FREE ();
   return tab_bar_items_vector;
@@ -8362,8 +8362,8 @@ tool_bar_items (Lisp_Object reuse, int *nitems)
      quitting while building the menus.  We do this instead of
      specbind because (1) errors will clear it anyway and (2) this
      avoids risk of specpdl overflow.  */
-  oquit = Vinhibit_quit;
-  Vinhibit_quit = Qt;
+  oquit = Vinhibit_quit_override;
+  Vinhibit_quit_override = Qt;
 
   /* Initialize tool_bar_items_vector and protect it from GC.  */
   init_tool_bar_items (reuse);
@@ -8421,7 +8421,7 @@ tool_bar_items (Lisp_Object reuse, int *nitems)
 	  map_keymap (keymap, process_tool_bar_item, Qnil, NULL, 1);
       }
 
-  Vinhibit_quit = oquit;
+  Vinhibit_quit_override = oquit;
   *nitems = ntool_bar_items / TOOL_BAR_ITEM_NSLOTS;
   SAFE_FREE ();
   return tool_bar_items_vector;
@@ -10916,7 +10916,9 @@ handle_interrupt (bool in_signal_handler)
       int count = NILP (Vquit_flag) ? 1 : force_quit_count + 1;
       force_quit_count = count;
       if (count == 3)
-	Vinhibit_quit = Qnil;
+        Vinhibit_quit = Qnil;
+      if (count == 10)
+        Vinhibit_quit_override = Qnil;
       Vquit_flag = Qt;
     }
 
diff --git a/src/lisp.h b/src/lisp.h
index a379977d35..2d7cb85fab 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3345,7 +3345,9 @@ SPECPDL_INDEX (void)
 
 /* True if ought to quit now.  */
 
-#define QUITP (!NILP (Vquit_flag) && NILP (Vinhibit_quit))
+#define QUITP (!NILP (Vquit_flag)                               \
+               && NILP (Vinhibit_quit)                          \
+               && NILP (Vinhibit_quit_override))
 
 /* Process a quit rarely, based on a counter COUNT, for efficiency.
    "Rarely" means once per USHRT_MAX + 1 times; this is somewhat
diff --git a/src/process.c b/src/process.c
index 91d426103d..f280588f51 100644
--- a/src/process.c
+++ b/src/process.c
@@ -5214,7 +5214,9 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
   FD_ZERO (&Available);
   FD_ZERO (&Writeok);
 
-  if (time_limit == 0 && nsecs == 0 && wait_proc && !NILP (Vinhibit_quit)
+  if (time_limit == 0 && nsecs == 0 && wait_proc
+      && !NILP (Vinhibit_quit)
+      && !NILP (Vinhibit_quit_override)
       && !(CONSP (wait_proc->status)
 	   && EQ (XCAR (wait_proc->status), Qexit)))
     message1 ("Blocking call to accept-process-output with quit inhibited!!");
@@ -6145,7 +6147,7 @@ read_and_dispose_of_process_output (struct Lisp_Process *p, char *chars,
   /* We inhibit quit here instead of just catching it so that
      hitting ^G when a filter happens to be running won't screw
      it up.  */
-  specbind (Qinhibit_quit, Qt);
+  specbind (Qinhibit_quit_override, Qt);
   specbind (Qlast_nonmenu_event, Qt);
 
   /* In case we get recursively called,
@@ -7364,7 +7366,7 @@ exec_sentinel (Lisp_Object proc, Lisp_Object reason)
   sentinel = p->sentinel;
 
   /* Inhibit quit so that random quits don't screw up a running filter.  */
-  specbind (Qinhibit_quit, Qt);
+  specbind (Qinhibit_quit_override, Qt);
   specbind (Qlast_nonmenu_event, Qt); /* Why? --Stef  */
 
   /* In case we get recursively called,
diff --git a/src/xdisp.c b/src/xdisp.c
index c2aa314c1a..b3a7512dcc 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -2801,7 +2801,7 @@ safe__call (bool inhibit_quit, ptrdiff_t nargs, Lisp_Object func, va_list ap)
 
       specbind (Qinhibit_redisplay, Qt);
       if (inhibit_quit)
-	specbind (Qinhibit_quit, Qt);
+	specbind (Qinhibit_quit_override, Qt);
       /* Use Qt to ensure debugger does not run,
 	 so there is no possibility of wanting to redisplay.  */
       val = internal_condition_case_n (Ffuncall, nargs, args, Qt,
@@ -27259,13 +27259,13 @@ dump_glyph_string (struct glyph_string *s)
    section.  If any of the code run by callers of ALLOCATE_HDC happens
    to call Lisp (might be possible due to all the hooks lying around),
    we must prevent it from quitting.  */
-# define ALLOCATE_HDC(hdc, f)			\
-  Lisp_Object prev_quit = Vinhibit_quit;	\
-  Vinhibit_quit = Qt;				\
+# define ALLOCATE_HDC(hdc, f)                           \
+  Lisp_Object prev_quit = Vinhibit_quit_override;	\
+  Vinhibit_quit_override = Qt;				\
   HDC hdc = get_frame_dc ((f))
 # define RELEASE_HDC(hdc, f)			\
   release_frame_dc ((f), (hdc));		\
-  Vinhibit_quit = prev_quit
+  Vinhibit_quit_override = prev_quit
 #else
 # define ALLOCATE_HDC(hdc, f)
 # define RELEASE_HDC(hdc, f)
-- 
2.25.1.481.gfbce0eb801-goog




^ permalink raw reply related	[flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-04-14  8:40 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-03-12  2:05 [PATCH] Really prevent quitting in sensitive contexts Daniel Colascione
2020-03-12  2:20 ` Stefan Monnier
2020-03-12  2:32   ` Daniel Colascione
2020-03-12 10:07     ` Michael Albinus
2020-03-12 10:21       ` Robert Pluim
2020-03-12 10:33         ` Michael Albinus
2020-03-13 10:00           ` Michael Albinus
2020-04-14  6:56       ` Daniel Colascione
2020-04-14  8:40         ` Michael Albinus
2020-03-12  3:29   ` Óscar Fuentes
2020-03-12  7:21     ` Eli Zaretskii
2020-03-12 13:31   ` Richard Copley
2020-03-12 17:36     ` Drew Adams
2020-03-12 22:06     ` Stefan Monnier
2020-03-12 23:14       ` Richard Copley
2020-03-12 23:35         ` Stefan Monnier
2020-03-12 23:56           ` Richard Copley
2020-03-14 15:31       ` Drew Adams
2020-03-14 18:21         ` Stefan Monnier
2020-03-14 18:36           ` Drew Adams

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.