Package: emacs;
Reported by: Pip Cet <pipcet <at> protonmail.com>
Date: Sat, 18 Jan 2025 12:20:02 UTC
Severity: normal
View this message in rfc822 format
From: Eli Zaretskii <eliz <at> gnu.org> To: Pip Cet <pipcet <at> protonmail.com>, Michael Albinus <michael.albinus <at> gmx.de>, Stefan Monnier <monnier <at> iro.umontreal.ca>, Stefan Kangas <stefankangas <at> gmail.com>, Andrea Corallo <acorallo <at> gnu.org> Cc: 75648 <at> debbugs.gnu.org Subject: bug#75648: Minor safety improvements to fns.c/eval.c Date: Sat, 18 Jan 2025 17:29:58 +0200
> Date: Sat, 18 Jan 2025 14:20:39 +0000
> From: Pip Cet via "Bug reports for GNU Emacs,
> the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
>
> "Pip Cet via \"Bug reports for GNU Emacs, the Swiss army knife of text editors\"" <bug-gnu-emacs <at> gnu.org> writes:
>
> > 1. Give such tests a :crash tag
> > 2. Introduce a should-not-crash macro which succeeds if the form it
> > evaluates returns in any way, whether by error or not.
> > 3. Modify ert.el to print when a :crash test is about to start running.
> > This allows us to identify the crashing test.
> > 4. Deviate from the current logical test order and put crash tests last.
> > It's conceivable that if a once-fixed crash reoccurs, the reason is a
> > simple bug that may show up in regular tests, too. If two tests fail
> > and one of them crashes the test run, it's better for the first failure
> > to have been reported first.
> >
> > I'll send a patch once this has a bug number.
>
> Patch 1 adds :crash tests to ert.el.
Hmm... maybe this should be discussed separately, as it's a much
broader issue. Can you provide the motivation, both for the new macro
(which is basically a one-liner), and for the new tag?
I've added some people to the discussion, because of this particular
part (maybe we should take this part to a separate thread).
> Patch 2 adds new tests which crash if the bug is present.
> Patch 3 fixes the actual bug.
Thanks.
> + (cl-destructuring-bind (stats test ) event-args
> + (and (not ert-quiet)
> + (memq :crash (ert-test-tags test))
> + (let* ((max (prin1-to-string (length (ert--stats-tests stats))))
> + (format-string (concat "%9s %"
> + (prin1-to-string (length max))
> + "s/" max " %S")))
> + (message format-string
> + "started"
> + (1+ (ert--stats-test-pos stats test))
> + (ert-test-name test))))))
Is it guaranteed that this text will be output before Emacs dies?
Maybe we should introduce flush-standard-error, or change the code to
print to stdout and use flush-standard-output?
> Fix unlikely crashes for self-modifying plists (bug#75684)
>
> * src/fns.c (Fplist_get): Verify 'tail' is always a cons cell.
> (Fplist_put): Verify 'tail' is always a cons cell. Use 'prev_cdr' to
> refer to the cons cell which has the value to be modified as its car.
> (plist_put): Analogous changes, to improve readability.
>
> diff --git a/src/fns.c b/src/fns.c
> index 07df2a5e90e..c07891771bb 100644
> --- a/src/fns.c
> +++ b/src/fns.c
> @@ -2602,11 +2602,12 @@ DEFUN ("plist-get", Fplist_get, Splist_get, 2, 3, 0,
> Lisp_Object tail = plist;
> FOR_EACH_TAIL_SAFE (tail)
> {
> - if (! CONSP (XCDR (tail)))
> + Lisp_Object tail_cdr = XCDR (tail);
> + if (! CONSP (tail_cdr))
> break;
> if (!NILP (call2 (predicate, XCAR (tail), prop)))
> - return XCAR (XCDR (tail));
> - tail = XCDR (tail);
> + return XCAR (tail_cdr);
> + tail = tail_cdr;
> }
>
> return Qnil;
> @@ -2657,27 +2658,27 @@ DEFUN ("plist-put", Fplist_put, Splist_put, 3, 4, 0,
> {
> if (NILP (predicate))
> return plist_put (plist, prop, val);
> - Lisp_Object prev = Qnil, tail = plist;
> + Lisp_Object prev_cdr = Qnil;
> + Lisp_Object tail = plist;
> FOR_EACH_TAIL (tail)
> {
> - if (! CONSP (XCDR (tail)))
> + Lisp_Object tail_cdr = XCDR (tail);
> + if (! CONSP (tail_cdr))
> break;
> -
> if (!NILP (call2 (predicate, XCAR (tail), prop)))
> {
> - Fsetcar (XCDR (tail), val);
> + Fsetcar (tail_cdr, val);
> return plist;
> }
>
> - prev = tail;
> - tail = XCDR (tail);
> + prev_cdr = tail = tail_cdr;
> }
> CHECK_TYPE (NILP (tail), Qplistp, plist);
> Lisp_Object newcell
> - = Fcons (prop, Fcons (val, NILP (prev) ? plist : XCDR (XCDR (prev))));
> - if (NILP (prev))
> + = Fcons (prop, Fcons (val, NILP (prev_cdr) ? plist : XCDR (prev_cdr)));
> + if (NILP (prev_cdr))
> return newcell;
> - Fsetcdr (XCDR (prev), newcell);
> + Fsetcdr (prev_cdr, newcell);
> return plist;
> }
>
> @@ -2685,10 +2686,12 @@ DEFUN ("plist-put", Fplist_put, Splist_put, 3, 4, 0,
> Lisp_Object
> plist_put (Lisp_Object plist, Lisp_Object prop, Lisp_Object val)
> {
> - Lisp_Object prev = Qnil, tail = plist;
> + Lisp_Object prev_cdr = Qnil;
> + Lisp_Object tail = plist;
> FOR_EACH_TAIL (tail)
> {
> - if (! CONSP (XCDR (tail)))
> + Lisp_Object tail_cdr = XCDR (tail);
> + if (! CONSP (tail_cdr))
> break;
>
> if (EQ (XCAR (tail), prop))
> @@ -2697,15 +2700,14 @@ plist_put (Lisp_Object plist, Lisp_Object prop, Lisp_Object val)
> return plist;
> }
>
> - prev = tail;
> - tail = XCDR (tail);
> + prev_cdr = tail = tail_cdr;;
> }
> CHECK_TYPE (NILP (tail), Qplistp, plist);
> Lisp_Object newcell
> - = Fcons (prop, Fcons (val, NILP (prev) ? plist : XCDR (XCDR (prev))));
> - if (NILP (prev))
> + = Fcons (prop, Fcons (val, NILP (prev_cdr) ? plist : XCDR (prev_cdr)));
> + if (NILP (prev_cdr))
> return newcell;
> - Fsetcdr (XCDR (prev), newcell);
> + Fsetcdr (prev_cdr, newcell);
> return plist;
> }
This is fine by me, but I wonder whether we should replace the NILP
tests with !CONSP, when we are about to take XCAR or XCDR?
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.