GNU bug report logs -
#75520
Circular code or data can hang Emacs unquittably
Previous Next
To reply to this bug, email your comments to 75520 AT debbugs.gnu.org.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75520; Package
emacs.
(Sun, 12 Jan 2025 17:09:02 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Pip Cet <pipcet <at> protonmail.com>:
New bug report received and forwarded. Copy sent to
bug-gnu-emacs <at> gnu.org.
(Sun, 12 Jan 2025 17:09:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
When evaluated in -Q, this code:
(funcall '(lambda () . #1=(t . #1#)))
causes an unquittable infloop.
IMHO, circular code isn't something we should support in any meaningful
way: it's perfectly okay for it to have unexpected results, and throw
any error message we can think of, or none, until I hit quit. However,
I do think the loops should be quittable.
The easiest way to ensure this is to move the maybe_quit call in
eval_sub to the top of the function. I mentioned this problem to Paul
Eggert and he provided the patch below, which also fixes two other cases
I'd overlooked.
There are similar issues with circular data: if Lisp code turns
process-environment into a circular structure, for example, call_process
will hang Emacs. If this should be fixed, please let me know so I can
create a separate bug report and look for similar issues.
From a2015ba2f81c67b1a77054f0715ba3d643504a98 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Sun, 5 Jan 2025 17:30:18 -0800
Subject: [PATCH] Some maybe_quit fixes in eval.c
* src/eval.c (skip_debugger):
Use FOR_EACH_TAIL to avoid unquittable infloops.
(eval_sub): Call maybe_quit earlier.
(funcall_lambda): Use FOR_EACH_TAIL rather than calling
maybe_quit each time through the loop.
---
src/eval.c | 30 ++++++++++++++++--------------
1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/src/eval.c b/src/eval.c
index 3e899db4436..2fa60af90af 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -2044,35 +2044,36 @@ wants_debugger (Lisp_Object list, Lisp_Object conditions)
static bool
skip_debugger (Lisp_Object conditions, Lisp_Object data)
{
- Lisp_Object tail;
- bool first_string = 1;
+ Lisp_Object tail = Vdebug_ignored_errors;
+ bool first_string = true;
Lisp_Object error_message;
error_message = Qnil;
- for (tail = Vdebug_ignored_errors; CONSP (tail); tail = XCDR (tail))
+
+ FOR_EACH_TAIL (tail)
{
if (STRINGP (XCAR (tail)))
{
if (first_string)
{
error_message = Ferror_message_string (data);
- first_string = 0;
+ first_string = false;
}
if (fast_string_match (XCAR (tail), error_message) >= 0)
- return 1;
+ return true;
}
else
{
- Lisp_Object contail;
+ Lisp_Object contail = conditions;
- for (contail = conditions; CONSP (contail); contail = XCDR (contail))
+ FOR_EACH_TAIL (contail)
if (EQ (XCAR (tail), XCAR (contail)))
- return 1;
+ return true;
}
}
- return 0;
+ return false;
}
/* Say whether SIGNAL is a `quit' error (or inherits from it). */
@@ -2470,6 +2471,11 @@ grow_specpdl_allocation (void)
Lisp_Object
eval_sub (Lisp_Object form)
{
+ /* Call maybe_quit now, rather than just before calling maybe_gc, so
+ that callers like Fand, Fprogn and Fsetq need not call maybe_quit
+ each time through their loops. */
+ maybe_quit ();
+
if (SYMBOLP (form))
{
/* Look up its binding in the lexical environment.
@@ -2483,8 +2489,6 @@ eval_sub (Lisp_Object form)
if (!CONSP (form))
return form;
- maybe_quit ();
-
maybe_gc ();
if (++lisp_eval_depth > max_lisp_eval_depth)
@@ -3262,10 +3266,8 @@ funcall_lambda (Lisp_Object fun, ptrdiff_t nargs, Lisp_Object *arg_vector)
bool optional = false;
bool rest = false;
bool previous_rest = false;
- for (; CONSP (syms_left); syms_left = XCDR (syms_left))
+ FOR_EACH_TAIL (syms_left)
{
- maybe_quit ();
-
Lisp_Object next = maybe_remove_pos_from_symbol (XCAR (syms_left));
if (!BARE_SYMBOL_P (next))
xsignal1 (Qinvalid_function, fun);
--
2.45.2
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75520; Package
emacs.
(Sun, 12 Jan 2025 18:44:02 GMT)
Full text and
rfc822 format available.
Message #8 received at 75520 <at> debbugs.gnu.org (full text, mbox):
> Date: Sun, 12 Jan 2025 17:08:24 +0000
> From: Pip Cet via "Bug reports for GNU Emacs,
> the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
>
> When evaluated in -Q, this code:
>
> (funcall '(lambda () . #1=(t . #1#)))
>
> causes an unquittable infloop.
>
> IMHO, circular code isn't something we should support in any meaningful
> way: it's perfectly okay for it to have unexpected results, and throw
> any error message we can think of, or none, until I hit quit. However,
> I do think the loops should be quittable.
>
> The easiest way to ensure this is to move the maybe_quit call in
> eval_sub to the top of the function. I mentioned this problem to Paul
> Eggert and he provided the patch below, which also fixes two other cases
> I'd overlooked.
>
> There are similar issues with circular data: if Lisp code turns
> process-environment into a circular structure, for example, call_process
> will hang Emacs. If this should be fixed, please let me know so I can
> create a separate bug report and look for similar issues.
Thanks, let's hear what others (CC'ed) think about this.
> >From a2015ba2f81c67b1a77054f0715ba3d643504a98 Mon Sep 17 00:00:00 2001
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> Date: Sun, 5 Jan 2025 17:30:18 -0800
> Subject: [PATCH] Some maybe_quit fixes in eval.c
>
> * src/eval.c (skip_debugger):
> Use FOR_EACH_TAIL to avoid unquittable infloops.
> (eval_sub): Call maybe_quit earlier.
> (funcall_lambda): Use FOR_EACH_TAIL rather than calling
> maybe_quit each time through the loop.
> ---
> src/eval.c | 30 ++++++++++++++++--------------
> 1 file changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/src/eval.c b/src/eval.c
> index 3e899db4436..2fa60af90af 100644
> --- a/src/eval.c
> +++ b/src/eval.c
> @@ -2044,35 +2044,36 @@ wants_debugger (Lisp_Object list, Lisp_Object conditions)
> static bool
> skip_debugger (Lisp_Object conditions, Lisp_Object data)
> {
> - Lisp_Object tail;
> - bool first_string = 1;
> + Lisp_Object tail = Vdebug_ignored_errors;
> + bool first_string = true;
> Lisp_Object error_message;
>
> error_message = Qnil;
> - for (tail = Vdebug_ignored_errors; CONSP (tail); tail = XCDR (tail))
> +
> + FOR_EACH_TAIL (tail)
> {
> if (STRINGP (XCAR (tail)))
> {
> if (first_string)
> {
> error_message = Ferror_message_string (data);
> - first_string = 0;
> + first_string = false;
> }
>
> if (fast_string_match (XCAR (tail), error_message) >= 0)
> - return 1;
> + return true;
> }
> else
> {
> - Lisp_Object contail;
> + Lisp_Object contail = conditions;
>
> - for (contail = conditions; CONSP (contail); contail = XCDR (contail))
> + FOR_EACH_TAIL (contail)
> if (EQ (XCAR (tail), XCAR (contail)))
> - return 1;
> + return true;
> }
> }
>
> - return 0;
> + return false;
> }
>
> /* Say whether SIGNAL is a `quit' error (or inherits from it). */
> @@ -2470,6 +2471,11 @@ grow_specpdl_allocation (void)
> Lisp_Object
> eval_sub (Lisp_Object form)
> {
> + /* Call maybe_quit now, rather than just before calling maybe_gc, so
> + that callers like Fand, Fprogn and Fsetq need not call maybe_quit
> + each time through their loops. */
> + maybe_quit ();
> +
> if (SYMBOLP (form))
> {
> /* Look up its binding in the lexical environment.
> @@ -2483,8 +2489,6 @@ eval_sub (Lisp_Object form)
> if (!CONSP (form))
> return form;
>
> - maybe_quit ();
> -
> maybe_gc ();
>
> if (++lisp_eval_depth > max_lisp_eval_depth)
> @@ -3262,10 +3266,8 @@ funcall_lambda (Lisp_Object fun, ptrdiff_t nargs, Lisp_Object *arg_vector)
> bool optional = false;
> bool rest = false;
> bool previous_rest = false;
> - for (; CONSP (syms_left); syms_left = XCDR (syms_left))
> + FOR_EACH_TAIL (syms_left)
> {
> - maybe_quit ();
> -
> Lisp_Object next = maybe_remove_pos_from_symbol (XCAR (syms_left));
> if (!BARE_SYMBOL_P (next))
> xsignal1 (Qinvalid_function, fun);
> --
> 2.45.2
>
>
>
>
>
>
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75520; Package
emacs.
(Sun, 12 Jan 2025 19:14:01 GMT)
Full text and
rfc822 format available.
Message #11 received at 75520 <at> debbugs.gnu.org (full text, mbox):
On 2025-01-12 10:43, Eli Zaretskii wrote:
>> Date: Sun, 12 Jan 2025 17:08:24 +0000
>> From: Pip Cet via "Bug reports for GNU Emacs,
>> There are similar issues with circular data: if Lisp code turns
>> process-environment into a circular structure, for example, call_process
>> will hang Emacs. If this should be fixed, please let me know so I can
>> create a separate bug report and look for similar issues.
>
> Thanks, let's hear what others (CC'ed) think about this.
I'd like the process-environment bug fixed too. The bug should be easy
to fix, and an unquittable Emacs is a real pain for all concerned.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75520; Package
emacs.
(Sun, 12 Jan 2025 19:30:02 GMT)
Full text and
rfc822 format available.
Message #14 received at 75520 <at> debbugs.gnu.org (full text, mbox):
> Date: Sun, 12 Jan 2025 11:12:59 -0800
> Cc: 75520 <at> debbugs.gnu.org
> From: Paul Eggert <eggert <at> cs.ucla.edu>
>
> On 2025-01-12 10:43, Eli Zaretskii wrote:
> >> Date: Sun, 12 Jan 2025 17:08:24 +0000
> >> From: Pip Cet via "Bug reports for GNU Emacs,
>
> >> There are similar issues with circular data: if Lisp code turns
> >> process-environment into a circular structure, for example, call_process
> >> will hang Emacs. If this should be fixed, please let me know so I can
> >> create a separate bug report and look for similar issues.
> >
> > Thanks, let's hear what others (CC'ed) think about this.
>
> I'd like the process-environment bug fixed too.
Which bug is that? Does it have a number?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75520; Package
emacs.
(Sun, 12 Jan 2025 20:26:02 GMT)
Full text and
rfc822 format available.
Message #17 received at 75520 <at> debbugs.gnu.org (full text, mbox):
>> There are similar issues with circular data: if Lisp code turns
>> process-environment into a circular structure, for example, call_process
>> will hang Emacs. If this should be fixed, please let me know so I can
>> create a separate bug report and look for similar issues.
> Thanks, let's hear what others (CC'ed) think about this.
I'm in favor of fixing all those unquittable inf-loops, except maybe
when there a very strong reason not to, which I fail to see here.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75520; Package
emacs.
(Sun, 12 Jan 2025 21:47:03 GMT)
Full text and
rfc822 format available.
Message #20 received at 75520 <at> debbugs.gnu.org (full text, mbox):
"Paul Eggert" <eggert <at> cs.ucla.edu> writes:
> On 2025-01-12 10:43, Eli Zaretskii wrote:
>>> Date: Sun, 12 Jan 2025 17:08:24 +0000
>>> From: Pip Cet via "Bug reports for GNU Emacs,
>
>>> There are similar issues with circular data: if Lisp code turns
>>> process-environment into a circular structure, for example, call_process
>>> will hang Emacs. If this should be fixed, please let me know so I can
>>> create a separate bug report and look for similar issues.
>>
>> Thanks, let's hear what others (CC'ed) think about this.
>
> I'd like the process-environment bug fixed too. The bug should be easy
> to fix, and an unquittable Emacs is a real pain for all concerned.
This seems to be a common problem. I did a quick git grep
'while.*CONSP':
definitely broken: debug-on-error, debug-ignored-errors,
unread-command-events, timer-list, the argument to Fkeymap_prompt
possibly broken: the 'error-conditions symbol property, image specs
(gtkutil.c checks valid_image_p, then runs Lisp, which might modify the
spec), buffer-undo-list (truncate_undo_list; please ensure ditty is
preserved), other functions in keymap.c.
git grep 'for.*CONSP' also returns many hits, which I haven't looked at
(it's also common to have the "for" keyword on a different line than the
CONSP, of course).
Pip
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75520; Package
emacs.
(Mon, 13 Jan 2025 02:57:01 GMT)
Full text and
rfc822 format available.
Message #23 received at 75520 <at> debbugs.gnu.org (full text, mbox):
On 2025-01-12 11:28, Eli Zaretskii wrote:
>> I'd like the process-environment bug fixed too.
> Which bug is that? Does it have a number?
Yes, it's the bug number 75520 that we're currently discussing. Look for
"process-environment" in the original bug report.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75520; Package
emacs.
(Mon, 13 Jan 2025 05:59:02 GMT)
Full text and
rfc822 format available.
Message #26 received at 75520 <at> debbugs.gnu.org (full text, mbox):
On 2025-01-12 13:46, Pip Cet wrote:
> This seems to be a common problem. I did a quick git grep
> 'while.*CONSP':
Yes, I did something similar in 2017; see, for example, commit
14dd9101ec4838f75addf25bf6b06ef33f8a7e97, which fixes 'length',
'member', 'memq', etc. I don't remember how carefully I audited the code
for the problem way back then. Either I missed several potential issues,
or the code has changed since then, or both.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75520; Package
emacs.
(Tue, 14 Jan 2025 16:34:02 GMT)
Full text and
rfc822 format available.
Message #29 received at 75520 <at> debbugs.gnu.org (full text, mbox):
> Date: Sun, 12 Jan 2025 21:57:55 -0800
> Cc: Eli Zaretskii <eliz <at> gnu.org>, Mattias Engdegård
> <mattiase <at> acm.org>, Stefan Monnier <monnier <at> iro.umontreal.ca>,
> Andrea Corallo <acorallo <at> gnu.org>, 75520 <at> debbugs.gnu.org
> From: Paul Eggert <eggert <at> cs.ucla.edu>
>
> On 2025-01-12 13:46, Pip Cet wrote:
> > This seems to be a common problem. I did a quick git grep
> > 'while.*CONSP':
>
> Yes, I did something similar in 2017; see, for example, commit
> 14dd9101ec4838f75addf25bf6b06ef33f8a7e97, which fixes 'length',
> 'member', 'memq', etc. I don't remember how carefully I audited the code
> for the problem way back then. Either I missed several potential issues,
> or the code has changed since then, or both.
Seems like we have a consensus here that this should be fixed.
But can we please have at least one test for every place where we fix
this kind of bug?
Thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75520; Package
emacs.
(Tue, 14 Jan 2025 18:47:02 GMT)
Full text and
rfc822 format available.
Message #32 received at 75520 <at> debbugs.gnu.org (full text, mbox):
"Eli Zaretskii" <eliz <at> gnu.org> writes:
>> Date: Sun, 12 Jan 2025 21:57:55 -0800
>> Cc: Eli Zaretskii <eliz <at> gnu.org>, Mattias Engdegård
>> <mattiase <at> acm.org>, Stefan Monnier <monnier <at> iro.umontreal.ca>,
>> Andrea Corallo <acorallo <at> gnu.org>, 75520 <at> debbugs.gnu.org
>> From: Paul Eggert <eggert <at> cs.ucla.edu>
>>
>> On 2025-01-12 13:46, Pip Cet wrote:
>> > This seems to be a common problem. I did a quick git grep
>> > 'while.*CONSP':
>>
>> Yes, I did something similar in 2017; see, for example, commit
>> 14dd9101ec4838f75addf25bf6b06ef33f8a7e97, which fixes 'length',
>> 'member', 'memq', etc. I don't remember how carefully I audited the code
>> for the problem way back then. Either I missed several potential issues,
>> or the code has changed since then, or both.
>
> Seems like we have a consensus here that this should be fixed.
I agree, but it's a significant task: My git grep caught ~8 bugs, but
covered only 22% of cases (at best), so I'd naively expect at least 20
bugs, and fixing them is usually simple but occasionally tedious
(because one might forget to include an additional check, for example,
as I did when I accidentally removed a STRINGP (XCAR (tem)) on
scratch/igc).
(I don't know what happened to coccinelle; it used to be good at this
kind of task, but I'm not sure it's still being developed or extended in
the appropriate ways).
My preference would be to use FOR_EACH_TAIL where we can, and to retain
that macro's quitting behavior: while the comment claims there is no
reason to quit in FOR_EACH_TAIL because Emacs doesn't support "very
long" lists, I believe that statement to be inaccurate for the MPS
branch. I can easily create a 100-million-entry list on the master
branch, too; it takes a few seconds to create (and a few more to
collect).
That would mean:
1. find likely infloops
2. prove that the infloop actually is reachable
3. check each loop for extra conditions
4. ensure that the loop is a good place to quit
5. use FOR_EACH_TAIL
6. write a test for each case
I'm volunteering to do 1, 3, 4, 5. I think (2) is a waste of time, to
be honest: if it looks like a bug and there isn't a clear comment
indicating that it's not one, it should be fixed. (6) is very hard, if
we actually want the test to fail (i.e. infloop) reliably for unfixed
Emacs versions. (I also think a blind belief that more tests are better
is unwarranted, but that's a project decision; since we have decided to
test exhaustively, if not as exhaustively as possible, I'll do that).
If (4) isn't possible, we'll have to put an Flength call somewhere
further up the call chain.
As for (2), consider, for example, the Qerror_conditions property of a
symbol. skip_debugger contains an unprotected loop iterating through
this property (this is the inner loop; the outer loop, iterating over
Vdebug_ignored_errors, definitely can cause an infloop); if we ever
reach that code, we'll infloop. I tried setting the Qerror_conditions
property to a circular list, but it didn't infloop. Do I now go and try
to prove conclusively that every code path that reaches skip_debugger is
actually safe? If so, do I add comments to every code paths to ensure
that it will remain safe? I think the effort to do so is
disproportionate: let's just use FOR_EACH_TAIL_SAFE there, even if there
is a possibility that it might not be required for correctness.
> But can we please have at least one test for every place where we fix
> this kind of bug?
Assuming you mean a test case that reliably infloops unfixed Emacs:
probably not. If the requirement is simply to make some variable
circular and call code which might refer to it, on the off chance that
that code would infloop once in a while, maybe.
And what if we can't find such a test? I'm pretty sure
find_font_encoding isn't unused, and it will infloop if
Vfont_encoding_alist is circular and we don't want to return one of its
elements, but how do I trigger it? Even if I managed to trigger it on
my system (no luck so far), would that do anything on Windows, with its
totally different font handling code? Wouldn't it depend on which fonts
are installed?
Pip
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75520; Package
emacs.
(Tue, 14 Jan 2025 19:05:01 GMT)
Full text and
rfc822 format available.
Message #35 received at 75520 <at> debbugs.gnu.org (full text, mbox):
On 2025-01-14 10:45, Pip Cet wrote:
> And what if we can't find such a test?
I'm with you. Any requirement that we implement a test case for every
such bug we fix would immediately turn on a light bulb in my head saying
"don't report or fix the bug". It's OK if someone else wants to do it,
but my volunteer time is too limited to do that kind of busywork.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75520; Package
emacs.
(Tue, 14 Jan 2025 21:14:01 GMT)
Full text and
rfc822 format available.
Message #38 received at 75520 <at> debbugs.gnu.org (full text, mbox):
> Date: Tue, 14 Jan 2025 18:45:51 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: Paul Eggert <eggert <at> cs.ucla.edu>, mattiase <at> acm.org, monnier <at> iro.umontreal.ca, acorallo <at> gnu.org, 75520 <at> debbugs.gnu.org
>
> > Seems like we have a consensus here that this should be fixed.
>
> I agree, but it's a significant task: My git grep caught ~8 bugs, but
> covered only 22% of cases (at best), so I'd naively expect at least 20
> bugs, and fixing them is usually simple but occasionally tedious
> (because one might forget to include an additional check, for example,
> as I did when I accidentally removed a STRINGP (XCAR (tem)) on
> scratch/igc).
We can fix only the most painful ones. We don't need to make sure
none remain, if it isn't easy.
> > But can we please have at least one test for every place where we fix
> > this kind of bug?
>
> Assuming you mean a test case that reliably infloops unfixed Emacs:
> probably not. If the requirement is simply to make some variable
> circular and call code which might refer to it, on the off chance that
> that code would infloop once in a while, maybe.
>
> And what if we can't find such a test? I'm pretty sure
> find_font_encoding isn't unused, and it will infloop if
> Vfont_encoding_alist is circular and we don't want to return one of its
> elements, but how do I trigger it? Even if I managed to trigger it on
> my system (no luck so far), would that do anything on Windows, with its
> totally different font handling code? Wouldn't it depend on which fonts
> are installed?
I thought concocting tests for this would be easy. If it isn't, we
don't have to have such tests.
(And Vfont_encoding_alist works on Windows like it works elsewhere.
Everything in font.c is platform-independent, with a small number of
exceptions explicitly marked by #ifdef's.)
Thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75520; Package
emacs.
(Tue, 14 Jan 2025 22:15:02 GMT)
Full text and
rfc822 format available.
Message #41 received at 75520 <at> debbugs.gnu.org (full text, mbox):
"Eli Zaretskii" <eliz <at> gnu.org> writes:
>> Date: Tue, 14 Jan 2025 18:45:51 +0000
>> From: Pip Cet <pipcet <at> protonmail.com>
>> Cc: Paul Eggert <eggert <at> cs.ucla.edu>, mattiase <at> acm.org, monnier <at> iro.umontreal.ca, acorallo <at> gnu.org, 75520 <at> debbugs.gnu.org
>>
>> > Seems like we have a consensus here that this should be fixed.
>>
>> I agree, but it's a significant task: My git grep caught ~8 bugs, but
>> covered only 22% of cases (at best), so I'd naively expect at least 20
>> bugs, and fixing them is usually simple but occasionally tedious
>> (because one might forget to include an additional check, for example,
>> as I did when I accidentally removed a STRINGP (XCAR (tem)) on
>> scratch/igc).
>
> We can fix only the most painful ones. We don't need to make sure
> none remain, if it isn't easy.
It's not. Maybe coccinelle can help, though (Stefan Kangas just used
it, so it seems to still exist, and coccigrep at least was updated
recently).
However, I don't believe we should go to the trouble of identifying bugs
and then decide not to fix them.
>> > But can we please have at least one test for every place where we fix
>> > this kind of bug?
>>
>> Assuming you mean a test case that reliably infloops unfixed Emacs:
>> probably not. If the requirement is simply to make some variable
>> circular and call code which might refer to it, on the off chance that
>> that code would infloop once in a while, maybe.
>>
>> And what if we can't find such a test? I'm pretty sure
>> find_font_encoding isn't unused, and it will infloop if
>> Vfont_encoding_alist is circular and we don't want to return one of its
>> elements, but how do I trigger it? Even if I managed to trigger it on
>> my system (no luck so far), would that do anything on Windows, with its
>> totally different font handling code? Wouldn't it depend on which fonts
>> are installed?
>
> I thought concocting tests for this would be easy. If it isn't, we
> don't have to have such tests.
Excellent. I'll do the easy tests and report back with a patch, listing
those places I couldn't find a test for.
> (And Vfont_encoding_alist works on Windows like it works elsewhere.
xfont.c calls font_registry_charsets in four places; w32font.c doesn't
call it at all.
> Everything in font.c is platform-independent, with a small number of
> exceptions explicitly marked by #ifdef's.)
A lot of the code isn't reachable in the same way; for example, unless
I'm quite confused, there's no way to reach the code from bug#75521 on
Windows, while it might be possible to reach it on GNU/Linux.
I don't know much about font registries, though (it's a workaround for
the limitation of 65535 characters/"font", IIUC? Is that limitation
still relevant, or are we using new font formats now which aren't
subject to it?).
Pip
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75520; Package
emacs.
(Tue, 14 Jan 2025 23:54:02 GMT)
Full text and
rfc822 format available.
Message #44 received at 75520 <at> debbugs.gnu.org (full text, mbox):
> My preference would be to use FOR_EACH_TAIL where we can, and to retain
> that macro's quitting behavior: while the comment claims there is no
> reason to quit in FOR_EACH_TAIL because Emacs doesn't support "very
> long" lists, I believe that statement to be inaccurate for the MPS
> branch. I can easily create a 100-million-entry list on the master
> branch, too; it takes a few seconds to create (and a few more to
> collect).
I guess the impact depends on the time to perform one iteration of
the loop. If iterating over the 100 million elements also takes just
a few seconds, then I don't see a need to try and `maybe_quit`.
FWIW, I find accidental circular lists happen much more often than
accidental 100-million-entry lists.
[ In any case I think this is mostly irrelevant for the discussion at
hand or at least can be decided separately/later. ]
> That would mean:
>
> 1. find likely infloops
> 2. prove that the infloop actually is reachable
> 3. check each loop for extra conditions
> 4. ensure that the loop is a good place to quit
> 5. use FOR_EACH_TAIL
> 6. write a test for each case
> I'm volunteering to do 1, 3, 4, 5. I think (2) is a waste of time, to
> be honest: if it looks like a bug and there isn't a clear comment
> indicating that it's not one, it should be fixed. (6) is very hard, if
> we actually want the test to fail (i.e. infloop) reliably for unfixed
> Emacs versions.
+1
> If (4) isn't possible, we'll have to put an Flength call somewhere
> further up the call chain.
For (4) the question is not just "quit" but non-local exit in general
since `FOR_EACH_TAIL` can also signal an error. We should likely use
`FOR_EACH_TAIL_SAFE` when non-local exits aren't known to be acceptable.
> As for (2), consider, for example, the Qerror_conditions property of a
> symbol. skip_debugger contains an unprotected loop iterating through
> this property (this is the inner loop; the outer loop, iterating over
> Vdebug_ignored_errors, definitely can cause an infloop); if we ever
> reach that code, we'll infloop. I tried setting the Qerror_conditions
> property to a circular list, but it didn't infloop. Do I now go and try
> to prove conclusively that every code path that reaches skip_debugger is
> actually safe?
Yeah, I just took a look and it's nasty. I think we'll usually catch it
in `find_handler_clause` (because that one uses `Fmemq`) before we reach
that code, but there's probably some way to avoid those `Fmemq`.
> I think the effort to do so is disproportionate: let's just use
> FOR_EACH_TAIL_SAFE there, even if there is a possibility that it might
> not be required for correctness.
Agreed. This specific code could likely use `Fmemq` even.
>> But can we please have at least one test for every place where we fix
>> this kind of bug?
Sounds like a lot of work since many of those places can require a fair
bit of setup before we can trigger them.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75520; Package
emacs.
(Wed, 15 Jan 2025 06:25:01 GMT)
Full text and
rfc822 format available.
Message #47 received at 75520 <at> debbugs.gnu.org (full text, mbox):
Pip Cet via "Bug reports for GNU Emacs, the Swiss army knife of text
editors" <bug-gnu-emacs <at> gnu.org> writes:
> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>
>> We can fix only the most painful ones. We don't need to make sure
>> none remain, if it isn't easy.
>
> It's not. Maybe coccinelle can help, though (Stefan Kangas just used
> it, so it seems to still exist, and coccigrep at least was updated
> recently).
AFAIK, it's actively developed and used. Version 1.3.0 was released
last November.
https://coccinelle.gitlabpages.inria.fr/website/distrib/changes.html
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#75520; Package
emacs.
(Wed, 15 Jan 2025 14:03:02 GMT)
Full text and
rfc822 format available.
Message #50 received at 75520 <at> debbugs.gnu.org (full text, mbox):
"Stefan Kangas" <stefankangas <at> gmail.com> writes:
> Pip Cet via "Bug reports for GNU Emacs, the Swiss army knife of text
> editors" <bug-gnu-emacs <at> gnu.org> writes:
>
>> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>>
>>> We can fix only the most painful ones. We don't need to make sure
>>> none remain, if it isn't easy.
>>
>> It's not. Maybe coccinelle can help, though (Stefan Kangas just used
>> it, so it seems to still exist, and coccigrep at least was updated
>> recently).
>
> AFAIK, it's actively developed and used. Version 1.3.0 was released
> last November.
Thanks!
Here's what I'm going to do (this is long, but I'd appreciate feedback
on how to improve the coccinelle scripts in particular!):
1. DONE: coccinelle script to detect likely loops
2. TODO: improve coccinelle script to un-detect likely loops that call an
infloop-breaking function
3. DONE: additional compile-time reporting of globals used in likely
loops
4. TODO: go through all of them :-)
Coccinelle doesn't seem to handle do { ... } while (...); loops, but we
don't really need those: the only two hits would be timer_check_2 and
validate_plist.
(timer_check is probably a problem: it calls copy-sequence on
Vtimer_list and Vtimer_idle_list, which may throw while input is blocked
and inhibit-quit is t. It would be really nice to mark critical
sections which should not ever throw a signal so we can eassert that
we're not in one, but that's only a runtime check...)
Here are the cocci files I'm using:
// Find loops for bug#75520: Circular code or data can hang Emacs unquittably
// This file cannot be applied to intervals.c, because that contains a
// double loop which matches the pattern in two different ways.
@@
identifier A;
expression R;
expression R2;
@@
+ maybe_circular(A);
while (
(
CONSP (A)
|
R && CONSP (A)
|
CONSP (A) && R
|
R && CONSP (A) && R2
) )
{
<...
A = XCDR (A);
...>
}
// Find loops for bug#75520: Circular code or data can hang Emacs unquittably
@@
identifier A;
expression E;
expression E2;
expression R;
expression R2;
@@
+E;
+maybe_circular (A);
-for (E;
+for (;
(
CONSP (A)
|
CONSP (A) && R
|
R && CONSP (A)
|
R && CONSP (A) && R2
);
E2)
{
<...
A = XCDR(A);
...>
}
(Note that coccinelle is smart enough to find "A = XCDR(A);" even if
that is the third sub-expression "argument" of the for statement. This
makes things a lot easier.)
These add calls to a new macro, maybe_circular, to happen right before
the first iteration of the loop.
It also breaks coding.c, because it matches a for loop definition in a
macro, but fails to add the necessary final backslash to the new line it
produces.
So we could leave it at that, and go over the diff, removing calls to
maybe_circular that are safe.
If we had a list of function calls which we know prevent an infloop
(such as maybe_quit, Flength(A), etc.), we could create another .cocci
file to remove maybe_circular automatically if the loop is likely to be
safe, but that would probably involve a small number of false negatives
if the call to the infloop-preventing function is conditional.
To get a quick list of global variables which are known to appear in
such loops, this works (but, probably, only on ELF systems; definitely
requires optimization, probably doesn't work with clang):
#define is_tainted_global(v, v2, s) ({ \
if (__builtin_constant_p ((v) == (v2))) \
{ \
asm volatile (".pushsection .tainted_globals,\"aw\"\n\t" \
".asciz \"" s "\"\n\t" \
".popsection" : : "i" (&v2)); \
} \
})
#include "tainted.h"
#define maybe_circular(v) ({ \
is_tainted (v); \
1; \
})
where tainted.h is generated by this bash script:
(echo "#define is_tainted(v) ({ \\";
(grep ' Lisp_Object f_V' | sed -e 's/ Lisp_Object f_//g' | sed -e 's/;$//' | while read; do echo "is_tainted_global(v, $REPLY, \"$REPLY\"); \\"; done) < globals.h;
echo "})") > tainted.h
This puts the names of all global variables which are known to appear in
potentially circular loops into an ELF section:
Contents of section .tainted_globals:
67ed60 566f7665 726c6179 5f617272 6f775f76 Voverlay_arrow_v
67ed70 61726961 626c655f 6c697374 00566f76 ariable_list.Vov
67ed80 65726c61 795f6172 726f775f 76617269 erlay_arrow_vari
67ed90 61626c65 5f6c6973 7400566f 7665726c able_list.Voverl
67eda0 61795f61 72726f77 5f766172 6961626c ay_arrow_variabl
67edb0 655f6c69 73740056 6f766572 6c61795f e_list.Voverlay_
67edc0 6172726f 775f7661 72696162 6c655f6c arrow_variable_l
67edd0 69737400 5677696e 646f775f 70657273 ist.Vwindow_pers
67ede0 69737465 6e745f70 6172616d 65746572 istent_parameter
67edf0 73005677 696e646f 775f7065 72736973 s.Vwindow_persis
67ee00 74656e74 5f706172 616d6574 65727300 tent_parameters.
67ee10 5677696e 646f775f 70657273 69737465 Vwindow_persiste
67ee20 6e745f70 6172616d 65746572 73005663 nt_parameters.Vc
67ee30 6f6d706c 6574696f 6e5f7265 67657870 ompletion_regexp
67ee40 5f6c6973 74005663 6f6d706c 6574696f _list.Vcompletio
67ee50 6e5f6967 6e6f7265 645f6578 74656e73 n_ignored_extens
67ee60 696f6e73 0056636f 6d706c65 74696f6e ions.Vcompletion
67ee70 5f69676e 6f726564 5f657874 656e7369 _ignored_extensi
67ee80 6f6e7300 56646562 75675f69 676e6f72 ons.Vdebug_ignor
67ee90 65645f65 72726f72 73005666 6f6e745f ed_errors.Vfont_
67eea0 656e636f 64696e67 5f616c69 73740056 encoding_alist.V
67eeb0 66616365 5f666f6e 745f7265 7363616c face_font_rescal
67eec0 655f616c 69737400 56736372 6970745f e_alist.Vscript_
67eed0 72657072 6573656e 74617469 76655f63 representative_c
67eee0 68617273 00567363 616c6162 6c655f66 hars.Vscalable_f
67eef0 6f6e7473 5f616c6c 6f776564 00 onts_allowed.
This finds both cases that are actually safe
(Vwindow_persistent_parameters has its own cycle detection: no
teleporting there) and cases that seem unsafe
(Vcompletion_ignored_extensions).
Somewhat independently of this, we might want to put static Lisp_Objects
into a separate section: this allows us to make staticpro of such
objects a nop and simply protect the entire section.
Similarly, we can make global Lisp_Object "variables" (actually elements
of struct emacs_globals) contiguous in memory, and protect them all at
once, omitting the staticpro calls for that case as well. This would
require changes to make-docfile.c which should also allow us to
conditionalize DEFVAR_LISP/DEFSYM in a way that detects unreachable
DEFVAR_LISP/DEFSYM "calls" and doesn't create variables for all of them.
(For example, right now, builds without nativecomp still defsym some 65
symbols from comp.c, wasting space in lispsym and for the symbol name).
(Another reason we may want to touch make-docfile.c is that we sometimes
use uninitialized global Lisp variables; if they were contiguous in
memory, we could simply poison them and catch such errors. I know this
approach works because if I poison the entire globals area, it allows me
to catch one such bug before crashing because non-Lisp-Object globals
are also poisoned: Voverriding_plist_environment is used by
init_syntax_once() but only initialized by syms_of_fns())
These changes should help performance, particularly with MPS, and may
allow protecting the static/global areas with memory barriers, which may
or may not help performance further (it may help debugging to put each
Lisp_Object on a separate page). I also think this should allow us to
add runtime checks ensuring that certain static Lisp_Objects do not ever
refer to circular objects.
So that's my approach: coccinelle for static analysis, compile-time
analysis producing an ELF section of likely tainted globals, and
potentially changes to ensure certain statics are non-circular at
runtime.
Of course, the first thing to do for any such changes is to remove
DEFVAR_LISP_NOPRO; see bug#75521 for a very lengthy discussion of this
question; IIUC, the conclusion is that the master branch will retain
DEFVAR_LISP_NOPRO for now, which complicates things significantly.
Pip
This bug report was last modified 84 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.