From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.ciao.gmane.io!not-for-mail From: Daniel Colascione Newsgroups: gmane.emacs.devel Subject: [PATCH] Really prevent quitting in sensitive contexts Date: Wed, 11 Mar 2020 19:05:54 -0700 Message-ID: <20200312020554.194607-1-dancol@dancol.org> Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Injection-Info: ciao.gmane.io; posting-host="ciao.gmane.io:159.69.161.202"; logging-data="94920"; mail-complaints-to="usenet@ciao.gmane.io" To: emacs-devel@gnu.org, dancol@dancol.org Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Thu Mar 12 03:08:37 2020 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jCDH2-000OZh-AQ for ged-emacs-devel@m.gmane-mx.org; Thu, 12 Mar 2020 03:08:36 +0100 Original-Received: from localhost ([::1]:60554 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jCDH1-0001TI-6P for ged-emacs-devel@m.gmane-mx.org; Wed, 11 Mar 2020 22:08:35 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:39270) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jCDEi-00010i-RC for emacs-devel@gnu.org; Wed, 11 Mar 2020 22:06:15 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1jCDEg-000189-Ri for emacs-devel@gnu.org; Wed, 11 Mar 2020 22:06:12 -0400 Original-Received: from dancol.org ([2600:3c01::f03c:91ff:fedf:adf3]:49418) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1jCDEg-00017L-9e for emacs-devel@gnu.org; Wed, 11 Mar 2020 22:06:10 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=dancol.org; s=x; h=Content-Transfer-Encoding:MIME-Version:Message-Id:Date:Subject:To: From:Sender:Reply-To:Cc:Content-Type:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: In-Reply-To:References:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=tPERbpM0xUasq7v9cFvczQWznF2dUC3F5N6c+heAckA=; b=C7dk3p50HzREnGsVJB9bBdrYgI mYMSW3n/99zb226EVQe3kaIdyd+Nn/iknMG1TnOsc9RjuHngm+hSSPnmjcXkk3nwD4VF5g/Lu01nx U8Jx0v9x+s12sd7JNWiskvWf6uIkD8QAmyVjem+lRlTRq/rZ2nliCaDYSQ1P3M30afiltYd45s0tV djTSkMbuISuIMEqb8M9mlUm4Qc9g1g0AW+OE2tTgyMyK8XOKobGj32udXiGEu4SegYC/ffUZ30qel bsnAiUt8LBECbgf+92Eh4cV3+vUyPV7hrcv4sCry38m6d+4AMgLeqyfWIIU7Fi6rjF2xpFdxPPuHa iQ+eCeDQ==; Original-Received: from dancol by dancol.org with local (Exim 4.89) (envelope-from ) id 1jCDEc-0007OE-3E; Wed, 11 Mar 2020 19:06:06 -0700 X-Mailer: git-send-email 2.25.1.481.gfbce0eb801-goog X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2600:3c01::f03c:91ff:fedf:adf3 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:245479 Archived-At: 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