Subject: Re: bug#75648: Minor safety improvements to fns.c/eval.c

From: Eli Zaretskii <[email-masked]>
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" <[email-masked]>
> 
> "Pip Cet via \"Bug reports for GNU Emacs, the Swiss army knife of text editors\"" <[email-masked]> 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?