[PATCH] Really prevent quitting in sensitive contexts

[PATCH] Really prevent quitting in sensitive contexts

[Daniel Colascione]

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

Re: [PATCH] Really prevent quitting in sensitive contexts

[Stefan Monnier]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/x-markdown; coding=UTF-8, Size: 837 bytes --]

> with-local-quit is actually documented to allow this sort of
> foreground quitting,

AFAIK it's not just documented to allow that, but it's designed
specifically for that.

> but since the interactive C-g race is unavoidable, we should deprecate
> use of with-local-quit for this purpose.

I tend to agree.

> This change adds a new inhibit-quit-override flag.

Sounds like an arm's race: then we'll have to add
a with-local-quit-override-override, etc...

> +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.

I'm not really familiar with those accidental cases.
Maybe they can be fixed more directly by not using with-local-quit?

Are there bug#NNN where these are discussed?


        Stefan

Re: [PATCH] Really prevent quitting in sensitive contexts

[Daniel Colascione]

On 3/11/20 7:20 PM, Stefan Monnier wrote:
>> with-local-quit is actually documented to allow this sort of
>> foreground quitting,
> 
> AFAIK it's not just documented to allow that, but it's designed
> specifically for that.

Right, but I can imagine it being useful more generally in some code 
that wants to be atomic against quit, but with exceptions. I don't know 
of any such code though.

>> but since the interactive C-g race is unavoidable, we should deprecate
>> use of with-local-quit for this purpose.
> 
> I tend to agree.
> 
>> This change adds a new inhibit-quit-override flag.
> 
> Sounds like an arm's race: then we'll have to add
> a with-local-quit-override-override, etc...

I considered making with-local-quit-override inaccessible from Lisp.

>> +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.
> 
> I'm not really familiar with those accidental cases.
> Maybe they can be fixed more directly by not using with-local-quit?

with-local-quit has been there for ten years. Can we just delete it?

> 
> Are there bug#NNN where these are discussed?

No. The motiviation is Tramp inflooping after a quit because it uses 
with-local-quit inside tramp-accept-process-output, which causes 
tramp-wait-for-regexp to retry infinitely and quickly.

I found *that* behavior trying to figure out why M-x compile over Tramp 
was hanging not only Emacs, but also the SSH connection more generally. 
I *think* it has something to do with compilation-filter calling 
file-truename, which invokes Tramp's handler, which wants to talk over 
the same SSH connection that's spewing compilation messages, deadlocking 
something somewhere.

We probably should make compile not filename lookup directly in the 
process filter.

Re: [PATCH] Really prevent quitting in sensitive contexts

[Óscar Fuentes]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> +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.
>
> I'm not really familiar with those accidental cases.
> Maybe they can be fixed more directly by not using with-local-quit?
>
> Are there bug#NNN where these are discussed?

bug#31692 dealed with missing key events and while-no-input /
with-local-quit were discussed.

Re: [PATCH] Really prevent quitting in sensitive contexts

[Eli Zaretskii]

On March 12, 2020 5:29:33 AM GMT+02:00, "Óscar Fuentes" <ofv@wanadoo.es> wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> 
> > Are there bug#NNN where these are discussed?
> 
> bug#31692 dealed with missing key events and while-no-input /
> with-local-quit were discussed.

AFAICT, that bug discussion is about while-no-input, and thus not really related to the issue at hand here.

Re: [PATCH] Really prevent quitting in sensitive contexts

[Michael Albinus]

[-- Attachment #1: Type: text/plain, Size: 948 bytes --]

Daniel Colascione <dancol@dancol.org> writes:

>> Are there bug#NNN where these are discussed?
>
> No. The motiviation is Tramp inflooping after a quit because it uses
> with-local-quit inside tramp-accept-process-output, which causes
> tramp-wait-for-regexp to retry infinitely and quickly.
>
> I found *that* behavior trying to figure out why M-x compile over
> Tramp was hanging not only Emacs, but also the SSH connection more
> generally. I *think* it has something to do with compilation-filter
> calling file-truename, which invokes Tramp's handler, which wants to
> talk over the same SSH connection that's spewing compilation messages,
> deadlocking something somewhere.

Please write a bug report, it should be solved in Tramp.

with-local-quit is used in tramp-accept-process-output by
intention. There were reports that Tramp hung, and couldn't be quit ...

Would the appended patch help (completely untested)?

Best regards, Michael.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 1342 bytes --]

diff --git a/lisp/tramp.el b/lisp/tramp.el
index 82ac693d..af2bc4b2 100644
--- a/lisp/tramp.el
+++ b/lisp/tramp.el
@@ -4227,18 +4227,20 @@ performed successfully.  Any other value means an error."
 (defun tramp-accept-process-output (proc &optional timeout)
   "Like `accept-process-output' for Tramp processes.
 This is needed in order to hide `last-coding-system-used', which is set
-for process communication also."
+for process communication also.
+If the user quits via `C-g', it is propagated up to `tramp-file-name-handler'."
   (with-current-buffer (process-buffer proc)
     (let ((inhibit-read-only t)
 	  last-coding-system-used
 	  result)
-      ;; JUST-THIS-ONE is set due to Bug#12145.
-      (tramp-message
-       proc 10 "%s %s %s %s\n%s"
-       proc timeout (process-status proc)
-       (with-local-quit
-	 (setq result (accept-process-output proc timeout nil t)))
-       (buffer-string))
+      ;; JUST-THIS-ONE is set due to Bug#12145.  `with-local-quit'
+      ;; returns t in order to report success.
+      (if (with-local-quit
+	    (setq result (accept-process-output proc timeout nil t)) t)
+	  (tramp-message
+	   proc 10 "%s %s %s %s\n%s"
+	   proc timeout (process-status proc) result (buffer-string))
+	(keyboard-quit))
       result)))

 (defun tramp-search-regexp (regexp)

Re: [PATCH] Really prevent quitting in sensitive contexts

[Robert Pluim]

>>>>> On Thu, 12 Mar 2020 11:07:29 +0100, Michael Albinus <michael.albinus@gmx.de> said:

    Michael> Daniel Colascione <dancol@dancol.org> writes:
    >>> Are there bug#NNN where these are discussed?
    >> 
    >> No. The motiviation is Tramp inflooping after a quit because it uses
    >> with-local-quit inside tramp-accept-process-output, which causes
    >> tramp-wait-for-regexp to retry infinitely and quickly.
    >> 
    >> I found *that* behavior trying to figure out why M-x compile over
    >> Tramp was hanging not only Emacs, but also the SSH connection more
    >> generally. I *think* it has something to do with compilation-filter
    >> calling file-truename, which invokes Tramp's handler, which wants to
    >> talk over the same SSH connection that's spewing compilation messages,
    >> deadlocking something somewhere.

    Michael> Please write a bug report, it should be solved in Tramp.

    Michael> with-local-quit is used in tramp-accept-process-output by
    Michael> intention. There were reports that Tramp hung, and couldn't be quit ...

    Michael> Would the appended patch help (completely untested)?

Yes! This solves my 'tramp hangs when visiting a patch' hang (or at
least, I can now C-g out of it).

Robert

Re: [PATCH] Really prevent quitting in sensitive contexts

[Michael Albinus]

Robert Pluim <rpluim@gmail.com> writes:

Hi Robert,

>     Michael> with-local-quit is used in tramp-accept-process-output by
>     Michael> intention. There were reports that Tramp hung, and
>     Michael> couldn't be quit ...
>
>     Michael> Would the appended patch help (completely untested)?
>
> Yes! This solves my 'tramp hangs when visiting a patch' hang (or at
> least, I can now C-g out of it).

Thanks for the feedback. I'm waiting for Daniel, and if he also confirms
an improvement, I'll commit.

> Robert

Best regards, Michael.

Re: [PATCH] Really prevent quitting in sensitive contexts

[Richard Copley]

Stefan, will you please consider configuring your system to send email
to the lists as plain text? Recently your messages have "Content-type:
text/markdown". My user agent presents their body as an attachment,
which is inconvenient.

On Thu, 12 Mar 2020 at 02:21, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

RE: [PATCH] Really prevent quitting in sensitive contexts

[Drew Adams]

> Stefan, will you please consider configuring your system to send email
> to the lists as plain text? Recently your messages have "Content-type:
> text/markdown". My user agent presents their body as an attachment,
> which is inconvenient.

+1.  I was going to say the same thing.

FYI, I think that the use of an attachment started
about a week ago.

Re: [PATCH] Really prevent quitting in sensitive contexts

[Stefan Monnier]

> Stefan, will you please consider configuring your system to send email
> to the lists as plain text? Recently your messages have "Content-type:
> text/markdown".

Hmm... that's odd... I did that a few months ago but then had to revert
it because of the number of broken MUAs out there.
Can you forward me one of the recent ones?

> My user agent presents their body as an attachment, which
> is inconvenient.

I'd urge you to report this as the bug that it is (MIME says very
clearly that any `text/<foo>` that's not specifically supported by the
MUA should be rendered as `text/plain`).


        Stefan

Re: [PATCH] Really prevent quitting in sensitive contexts

[Richard Copley]

Thanks, your latest messages are being displayed properly here.

On Thu, 12 Mar 2020 at 22:06, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> > Stefan, will you please consider configuring your system to send email
> > to the lists as plain text? Recently your messages have "Content-type:
> > text/markdown".
>
> Hmm... that's odd... I did that a few months ago but then had to revert
> it because of the number of broken MUAs out there.
> Can you forward me one of the recent ones?

Sent off-list.

> > My user agent presents their body as an attachment, which
> > is inconvenient.
>
> I'd urge you to report this as the bug that it is (MIME says very
> clearly that any `text/<foo>` that's not specifically supported by the
> MUA should be rendered as `text/plain`).

It's the Google Mail web interface. Unfortunately, as far as I know,
they don't solicit bug reports.

Re: [PATCH] Really prevent quitting in sensitive contexts

[Stefan Monnier]

> It's the Google Mail web interface.  Unfortunately, as far as I know,
> they don't solicit bug reports.

A quick search turned up

    https://www.wikihow.com/Report-a-Bug-in-Gmail


-- Stefan

Re: [PATCH] Really prevent quitting in sensitive contexts

[Richard Copley]

On Thu, 12 Mar 2020 at 23:35, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> > It's the Google Mail web interface.  Unfortunately, as far as I know,
> > they don't solicit bug reports.
>
> A quick search turned up
>
>     https://www.wikihow.com/Report-a-Bug-in-Gmail

Oh thanks, that's good. Feedback submitted.

Re: [PATCH] Really prevent quitting in sensitive contexts

[Michael Albinus]

Michael Albinus <michael.albinus@gmx.de> writes:

Hi,

>>     Michael> with-local-quit is used in tramp-accept-process-output by
>>     Michael> intention. There were reports that Tramp hung, and
>>     Michael> couldn't be quit ...
>>
>>     Michael> Would the appended patch help (completely untested)?
>>
>> Yes! This solves my 'tramp hangs when visiting a patch' hang (or at
>> least, I can now C-g out of it).
>
> Thanks for the feedback. I'm waiting for Daniel, and if he also confirms
> an improvement, I'll commit.

No answer yet, so I have pushed it to the master branch. My internal
tests haven't shown any regression.

>> Robert

Best regards, Michael.

RE: [PATCH] Really prevent quitting in sensitive contexts

[Drew Adams]

[-- Attachment #1: Type: text/plain, Size: 363 bytes --]

Attached.

> > Stefan, will you please consider configuring your system to send email
> > to the lists as plain text? Recently your messages have "Content-type:
> > text/markdown".
> 
> Hmm... that's odd... I did that a few months ago but then had to revert
> it because of the number of broken MUAs out there.
> Can you forward me one of the recent ones?

[-- Attachment #2: Type: message/rfc822, Size: 1620 bytes --]

From: Stefan Monnier <monnier@iro.umontreal.ca>
To: Steve Youngs <steve@sxemacs.org>
Cc: emacs-devel@gnu.org, Richard Stallman <rms@gnu.org>, Dima Kogan <dima@secretsauce.net>
Subject: Re: Can we expand the valid location of "Local Variables" ?
Date: Sat, 14 Mar 2020 00:07:30 -0700 (PDT)
Message-ID: <jwv5zf7pgb5.fsf-monnier+emacs@gnu.org>


[-- Attachment #2.1: Untitled.txt --]
[-- Type: text/x-markdown, Size: 710 bytes --]

From: Stefan Monnier <monnier@iro.umontreal.ca>
To: Steve Youngs <steve@sxemacs.org>
Cc: emacs-devel@gnu.org, Richard Stallman <rms@gnu.org>, Dima Kogan <dima@secretsauce.net>
Subject: Re: Can we expand the valid location of "Local Variables" ?
Date: Sat, 14 Mar 2020 00:07:30 -0700 (PDT)
Message-ID: <jwv5zf7pgb5.fsf-monnier+emacs@gnu.org>

> How about some form of "include" mechanism:
>
>         Local Variables:
>         @include: FILENAME
>         End:

Sounds OK, yes.

> I would suggest implementing that with the same care, considerations,
> and restrictions as has `eval' for Locals

Indeed.  There be dragons.

> Mind you, if your Local Vars sections are getting so out of hand that
> they warrant having a whole extra file to store them, you should probably
> re-think what you're doing. :-)

I think such an @include directive can be useful not just for large
sections but also to avoid duplicating settings (i.e. a kind of
alternative to .dir-locals.el, not tied to the directory hierarchy).


        Stefan

Re: [PATCH] Really prevent quitting in sensitive contexts

[Stefan Monnier]

> Attached.

OK, I found the culprit.


        Stefan

RE: [PATCH] Really prevent quitting in sensitive contexts

[Drew Adams]

> > Attached.
> 
> OK, I found the culprit.

Great!  Thanks for doing that.

Re: [PATCH] Really prevent quitting in sensitive contexts

[Daniel Colascione]

On 3/12/20 3:07 AM, Michael Albinus wrote:
> Daniel Colascione <dancol@dancol.org> writes:
> 
>>> Are there bug#NNN where these are discussed?
>>
>> No. The motiviation is Tramp inflooping after a quit because it uses
>> with-local-quit inside tramp-accept-process-output, which causes
>> tramp-wait-for-regexp to retry infinitely and quickly.
>>
>> I found *that* behavior trying to figure out why M-x compile over
>> Tramp was hanging not only Emacs, but also the SSH connection more
>> generally. I *think* it has something to do with compilation-filter
>> calling file-truename, which invokes Tramp's handler, which wants to
>> talk over the same SSH connection that's spewing compilation messages,
>> deadlocking something somewhere.
> 
> Please write a bug report, it should be solved in Tramp.

No, it shouldn't. This use of with-local-quit can *never* be correct 
because a stray C-g can *always* interfere with a filter. Don't do 
long-running operations in process filters. Period. The fix isn't to 
make quitting these operations possible in some defined manner. The fix 
is not to do them in the first place.

Re: [PATCH] Really prevent quitting in sensitive contexts

[Michael Albinus]

Daniel Colascione <dancol@dancol.org> writes:

Hi Daniel,

> No, it shouldn't. This use of with-local-quit can *never* be correct
> because a stray C-g can *always* interfere with a filter. Don't do
> long-running operations in process filters. Period. The fix isn't to
> make quitting these operations possible in some defined manner. The
> fix is not to do them in the first place.

How do you want to prevent use of such a basic operation like
`file-truename' in a process filter? And yes, in the remote case this
could be a "long-running operation".

Best regards, Michael.