bug#63253: 29.0.90; with-delayed-message fails in combination with inhibit-message

bug#63253: 29.0.90; with-delayed-message fails in combination with inhibit-message

[Daniel Mendler]

There seems to exist an interaction issue of `with-delayed-message' with
`inhibit-message'. In order to reproduce the issue, start emacs -Q and
evaluate the following function:

(defun delayed-message-bug ()
  (interactive)
  (setq inhibit-message-regexps (list "\\`something"))
  (add-to-list 'set-message-functions #'inhibit-message)
  (while t
    (with-delayed-message (0.5 "Fontifying...")
      (font-lock-flush (point-min) (point-max))
      (font-lock-ensure (point-min) (point-max)))
    (message "Done")))

Then open a large file like emacs/src/xterm.c and run `M-x
delayed-message-bug'. The error may not occur on the first try since it
is timing dependent. Retry a few times if no error is observed. I
observe the internal error

    Error in syntax_table logic for to-the-end intervals

which is produced by `update_syntax_table' from syntax.c.

In GNU Emacs 29.0.90 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.16.0, Xaw scroll bars) of 2023-04-27
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Debian GNU/Linux 11 (bullseye)

Configured using:
 'configure --prefix=/home/user/emacs/install --with-json
 --with-native-compilation --with-x-toolkit=athena --without-dbus
 --without-selinux --without-threads --without-gsettings --with-cairo'

Configured features:
CAIRO FREETYPE GIF GLIB GMP GNUTLS HARFBUZZ JPEG JSON LIBXML2 MODULES
NATIVE_COMP NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3 TIFF
TOOLKIT_SCROLL_BARS WEBP X11 XDBE XIM XPM LUCID ZLIB

bug#63253: 29.0.90; with-delayed-message fails in combination with inhibit-message

[Eli Zaretskii]

> From: Daniel Mendler <mail@daniel-mendler.de>
> Date: Wed, 03 May 2023 21:54:47 +0200
> 
> There seems to exist an interaction issue of `with-delayed-message' with
> `inhibit-message'. In order to reproduce the issue, start emacs -Q and
> evaluate the following function:
> 
> (defun delayed-message-bug ()
>   (interactive)
>   (setq inhibit-message-regexps (list "\\`something"))
>   (add-to-list 'set-message-functions #'inhibit-message)
>   (while t
>     (with-delayed-message (0.5 "Fontifying...")
>       (font-lock-flush (point-min) (point-max))
>       (font-lock-ensure (point-min) (point-max)))
>     (message "Done")))
> 
> Then open a large file like emacs/src/xterm.c and run `M-x
> delayed-message-bug'. The error may not occur on the first try since it
> is timing dependent. Retry a few times if no error is observed. I
> observe the internal error
> 
>     Error in syntax_table logic for to-the-end intervals
> 
> which is produced by `update_syntax_table' from syntax.c.

Stefan, and ideas/suggestions/hints?

bug#63253: 29.0.90; with-delayed-message fails in combination with inhibit-message

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

> There seems to exist an interaction issue of `with-delayed-message' with
> `inhibit-message'. In order to reproduce the issue, start emacs -Q and
> evaluate the following function:
>
> (defun delayed-message-bug ()
>   (interactive)
>   (setq inhibit-message-regexps (list "\\`something"))
>   (add-to-list 'set-message-functions #'inhibit-message)
>   (while t
>     (with-delayed-message (0.5 "Fontifying...")
>       (font-lock-flush (point-min) (point-max))
>       (font-lock-ensure (point-min) (point-max)))
>     (message "Done")))
>
> Then open a large file like emacs/src/xterm.c and run `M-x
> delayed-message-bug'. The error may not occur on the first try since it
> is timing dependent. Retry a few times if no error is observed. I
> observe the internal error

After several unsuccessful attempts, I just managed to reproduce the
problem here.  Not sure yet what was the magic incantation, but at least
I should now be able to look into the source of the problem.


        Stefan

bug#63253: 29.0.90; with-delayed-message fails in combination with inhibit-message

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

> There seems to exist an interaction issue of `with-delayed-message' with
> `inhibit-message'.

Very much so, indeed.  The issue is fundamentally linked to
`set-message-function`:

`with-delayed-message` calls `message3` (via
`with_delayed_message_display`) from an "atimer", i.e. a thing that can
be run from `process_pending_signals`, `unblock_input`, `maybe_quit`, ...

AFAIK these are places where it's safe to run some C code, but not
places where it's safe to run arbitrary ELisp code.

So `set-message-function` is "dangerous" because it runs ELisp code from
`message3` => `message3_nolog` => `set_message`.

I think this is related to bug#58042.

Here the problem is that the regexp engine is stateful, so calling
a regexp function in the middle of another regexp match causes serious
problems.  When we need to run ELisp from a regexp match for
on-the-fly syntax-propertization (in `parse_sexp_propertize`), we're
careful to try and detect if the ELisp code may have modified the
current buffer and we re-set the `gl_state` with a call to
`SETUP_SYNTAX_TABLE`.

But when `with-delayed-message' performs a regexp operation from within
another regexp match, it doesn't re-set the `gl_state`, which then leads
to this "Error in syntax_table logic for to-the-end intervals" error.

I'm surprised we haven't yet seen the same problem when the ELisp code is
run via things like -[EmacsView layoutSublayersOfLayer:] which can be
called from `maybe_quit` and call `redisplay` which in turn can run
arbitrary ELisp code (via `mode-line-format`, jit-lock, you name it).


        Stefan

bug#63253: 29.0.90; with-delayed-message fails in combination with inhibit-message

[Eli Zaretskii]

> Cc: 63253@debbugs.gnu.org
> Date: Mon, 08 May 2023 09:54:35 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> > There seems to exist an interaction issue of `with-delayed-message' with
> > `inhibit-message'.
> 
> Very much so, indeed.  The issue is fundamentally linked to
> `set-message-function`:
> 
> `with-delayed-message` calls `message3` (via
> `with_delayed_message_display`) from an "atimer", i.e. a thing that can
> be run from `process_pending_signals`, `unblock_input`, `maybe_quit`, ...
> 
> AFAIK these are places where it's safe to run some C code, but not
> places where it's safe to run arbitrary ELisp code.
> 
> So `set-message-function` is "dangerous" because it runs ELisp code from
> `message3` => `message3_nolog` => `set_message`.

So I guess we need to have run_timers bind some variable, and then in
set_message, if that variable is non-nil, we should avoid calling
functions from set-message-function?

bug#63253: 29.0.90; with-delayed-message fails in combination with inhibit-message

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

>> So `set-message-function` is "dangerous" because it runs ELisp code from
>> `message3` => `message3_nolog` => `set_message`.
>
> So I guess we need to have run_timers bind some variable, and then in
> set_message, if that variable is non-nil, we should avoid calling
> functions from set-message-function?

That's probably the best short-term workaround, yes.

The quick hack below seems to avoid the problem for me (at the cost of
ignoring `inhibit-message` for the `with-delayed-message`s).


        Stefan


diff --git a/src/alloc.c b/src/alloc.c
index 05a19f0b7e9..7ff2cd3b100 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -367,7 +367,7 @@ #define PUREBEG (char *) pure
 
 /* If positive, garbage collection is inhibited.  Otherwise, zero.  */
 
-static intptr_t garbage_collection_inhibited;
+intptr_t garbage_collection_inhibited;
 
 /* The GC threshold in bytes, the last time it was calculated
    from gc-cons-threshold and gc-cons-percentage.  */
diff --git a/src/xdisp.c b/src/xdisp.c
index 43847544396..a5d7d256b6d 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -12810,6 +12810,8 @@ truncate_message_1 (void *a1, Lisp_Object a2)
   return false;
 }
 
+extern intptr_t garbage_collection_inhibited;
+
 /* Set the current message to STRING.  */
 
 static void
@@ -12819,7 +12821,11 @@ set_message (Lisp_Object string)
 
   eassert (STRINGP (string));
 
-  if (FUNCTIONP (Vset_message_function))
+  if (FUNCTIONP (Vset_message_function)
+      /* FIXME: (bug#63253) We should really make the regexp engine re-entrant,
+         but in the mean time, let's ignore `set-message-function` when
+         called from `probably_quit`.  */
+      && !garbage_collection_inhibited)
     {
       specpdl_ref count = SPECPDL_INDEX ();
       specbind (Qinhibit_quit, Qt);

bug#63253: 29.0.90; with-delayed-message fails in combination with inhibit-message

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: mail@daniel-mendler.de,  63253@debbugs.gnu.org
> Date: Mon, 08 May 2023 11:13:37 -0400
> 
> >> So `set-message-function` is "dangerous" because it runs ELisp code from
> >> `message3` => `message3_nolog` => `set_message`.
> >
> > So I guess we need to have run_timers bind some variable, and then in
> > set_message, if that variable is non-nil, we should avoid calling
> > functions from set-message-function?
> 
> That's probably the best short-term workaround, yes.
> 
> The quick hack below seems to avoid the problem for me (at the cost of
> ignoring `inhibit-message` for the `with-delayed-message`s).

Thanks, I think you should install this on the emacs-29 branch.  And I
think a similar change is needed in clear_message, so it doesn't call
clear-message-function.

But what does this mean for with-delayed-message and
funcall-with-delayed-message?  These were introduced in Emacs 29, but
if we say that atimers cannot be used to safely run arbitrary Lisp,
then we should somehow make them safer, perhaps by having in
with_delayed_message_display a subset of code message3, without the
calls to Lisp.  WDYT?

bug#63253: 29.0.90; with-delayed-message fails in combination with inhibit-message

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

> Thanks, I think you should install this on the emacs-29 branch.  And I
> think a similar change is needed in clear_message, so it doesn't call
> clear-message-function.

OK, done.

> But what does this mean for with-delayed-message and
> funcall-with-delayed-message?  These were introduced in Emacs 29, but
> if we say that atimers cannot be used to safely run arbitrary Lisp,
> then we should somehow make them safer, perhaps by having in
> with_delayed_message_display a subset of code message3, without the
> calls to Lisp.  WDYT?

The code already falls back to a default behavior of displaying the
message in the echo area when calls to ELisp can't be used.


        Stefan

bug#63253: 29.0.90; with-delayed-message fails in combination with inhibit-message

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>,  mail@daniel-mendler.de,
>   63253@debbugs.gnu.org
> Date: Tue, 09 May 2023 22:31:09 -0400
> 
> > Thanks, I think you should install this on the emacs-29 branch.  And I
> > think a similar change is needed in clear_message, so it doesn't call
> > clear-message-function.
> 
> OK, done.

Thanks.

> > But what does this mean for with-delayed-message and
> > funcall-with-delayed-message?  These were introduced in Emacs 29, but
> > if we say that atimers cannot be used to safely run arbitrary Lisp,
> > then we should somehow make them safer, perhaps by having in
> > with_delayed_message_display a subset of code message3, without the
> > calls to Lisp.  WDYT?
> 
> The code already falls back to a default behavior of displaying the
> message in the echo area when calls to ELisp can't be used.

You mean, the changes that you just installed?  If so, that is AFAIU
conditioned on garbage_collection_inhibited, which doesn't necessarily
disable Lisp calls from delayed messages in all situations.  Plus, we
might at some point decide that garbage_collection_inhibited is not
necessary in probably_quit.

What I meant was to completely disable customizing
set-message-function for delayed messages.  We don't have to support
that, and the fact that we call 'message' is just an implementation
detail.  If nothing else, this will make the effect on delayed
messages consistent no matter when and how they are actually
displayed.

bug#63253: 29.0.90; with-delayed-message fails in combination with inhibit-message

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

>> The code already falls back to a default behavior of displaying the
>> message in the echo area when calls to ELisp can't be used.
>
> You mean, the changes that you just installed?  If so, that is AFAIU
> conditioned on garbage_collection_inhibited, which doesn't necessarily
> disable Lisp calls from delayed messages in all situations.  Plus, we
> might at some point decide that garbage_collection_inhibited is not
> necessary in probably_quit.
>
> What I meant was to completely disable customizing
> set-message-function for delayed messages.  We don't have to support
> that, and the fact that we call 'message' is just an implementation
> detail.  If nothing else, this will make the effect on delayed
> messages consistent no matter when and how they are actually
> displayed.

Hmm... I think "customizing `set-message-function` for delayed messages"
is actually desirable (which is why I left a FIXME in my patch).
So I regard the new behavior as a bug, just one that is hopefully less
severe than the problem it intends to avoid.


        Stefan

bug#63253: 29.0.90; with-delayed-message fails in combination with inhibit-message

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: larsi@gnus.org,  mail@daniel-mendler.de,  63253@debbugs.gnu.org
> Date: Wed, 10 May 2023 11:15:54 -0400
> 
> >> The code already falls back to a default behavior of displaying the
> >> message in the echo area when calls to ELisp can't be used.
> >
> > You mean, the changes that you just installed?  If so, that is AFAIU
> > conditioned on garbage_collection_inhibited, which doesn't necessarily
> > disable Lisp calls from delayed messages in all situations.  Plus, we
> > might at some point decide that garbage_collection_inhibited is not
> > necessary in probably_quit.
> >
> > What I meant was to completely disable customizing
> > set-message-function for delayed messages.  We don't have to support
> > that, and the fact that we call 'message' is just an implementation
> > detail.  If nothing else, this will make the effect on delayed
> > messages consistent no matter when and how they are actually
> > displayed.
> 
> Hmm... I think "customizing `set-message-function` for delayed messages"
> is actually desirable

Why? we use that for a single facility, which has a well-defined
purpose: show an echo-area message if BODY takes longer than some
time.  Why do we have to allow customization of the message displayed
by this facility?

> (which is why I left a FIXME in my patch).
> So I regard the new behavior as a bug, just one that is hopefully less
> severe than the problem it intends to avoid.

How you envision that FIXME to be fixed, if atimers cannot safely run
any Lisp?  What kind of customizations will we be able to offer, and
how?  maybe_quit can be called from any kind of context, and that is a
feature that we cannot give up.

bug#63253: 29.0.90; with-delayed-message fails in combination with inhibit-message

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

>> Hmm... I think "customizing `set-message-function` for delayed messages"
>> is actually desirable
>
> Why? we use that for a single facility, which has a well-defined
> purpose: show an echo-area message if BODY takes longer than some
> time.  Why do we have to allow customization of the message displayed
> by this facility?

We still want that message to be moved to the end of the minibuffer
when that minibuffer is active, and users may still want to be able
to silence some of those messages.

>> (which is why I left a FIXME in my patch).
>> So I regard the new behavior as a bug, just one that is hopefully less
>> severe than the problem it intends to avoid.
> How you envision that FIXME to be fixed, if atimers cannot safely run
> any Lisp?

Apparently, we do run ELisp code from `maybe_quit` via the GUI's event
handling (according to Po Lu, not just under macOS but also under X11),
so maybe we should strive to make it "safe" to run ELisp from `maybe_quit`
(and hence atimers).

It's inherently dangerous since it amounts to preemptive concurrency, so
by "safe" I mean that we should strive to make it safe with some
side-conditions about the risks of concurrency bugs.


        Stefan

bug#63253: 29.0.90; with-delayed-message fails in combination with inhibit-message

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: larsi@gnus.org,  mail@daniel-mendler.de,  63253@debbugs.gnu.org
> Date: Thu, 11 May 2023 11:00:28 -0400
> 
> >> Hmm... I think "customizing `set-message-function` for delayed messages"
> >> is actually desirable
> >
> > Why? we use that for a single facility, which has a well-defined
> > purpose: show an echo-area message if BODY takes longer than some
> > time.  Why do we have to allow customization of the message displayed
> > by this facility?
> 
> We still want that message to be moved to the end of the minibuffer
> when that minibuffer is active, and users may still want to be able
> to silence some of those messages.

It's highly unlikely that maybe_quit will be called when we are in the
minibuffer in recursive-edit, because maybe_quit is called from
potentially prolonged processing.  As for silencing, I see no reason
why these messages should be silenced, since a Lisp program that calls
with-delayed-message explicitly wants the message to appear, so as to
provide important feedback about some long processing.

> > How you envision that FIXME to be fixed, if atimers cannot safely run
> > any Lisp?
> 
> Apparently, we do run ELisp code from `maybe_quit` via the GUI's event
> handling (according to Po Lu, not just under macOS but also under X11),
> so maybe we should strive to make it "safe" to run ELisp from `maybe_quit`
> (and hence atimers).
> 
> It's inherently dangerous since it amounts to preemptive concurrency, so
> by "safe" I mean that we should strive to make it safe with some
> side-conditions about the risks of concurrency bugs.

AFAIU, the danger is because some places that call maybe_quit are
stateful.  So to make that safe, we'd need to save and restore the
state around the call, something that could be expensive if at all
possible.  And having to do that is a misfeature in itself: we just
got rid of doing so due to problems with ralloc and stuff, so why
would we want to get into another such trouble?

bug#63253: 29.0.90; with-delayed-message fails in combination with inhibit-message

[Eli Zaretskii]

> Cc: mail@daniel-mendler.de, 63253@debbugs.gnu.org
> Date: Mon, 08 May 2023 17:55:08 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > So `set-message-function` is "dangerous" because it runs ELisp code from
> > `message3` => `message3_nolog` => `set_message`.
> 
> So I guess we need to have run_timers bind some variable, and then in
> set_message, if that variable is non-nil, we should avoid calling
> functions from set-message-function?

Or maybe (or in addition) regex-emacs.c should save and restore its
state around calls to maybe_quit?  Is that possible?

bug#63253: 29.0.90; with-delayed-message fails in combination with inhibit-message

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

> Or maybe (or in addition) regex-emacs.c should save and restore its
> state around calls to maybe_quit?  Is that possible?

That would be very costly, I think.
But maybe `gl_state` can be turned into an array (stack) of states, or
can be stack-allocated, so that when we can do nested regexp
matches safely.

There's still the issue that `set-message-functions` could theoretically
do nasty things like insert/delete text in the current buffer, leading
to `regexp.c` containing stale pointers to the old text.


        Stefan