GNU bug report logs - #75648
Minor safety improvements to fns.c/eval.c

Previous Next

To reply to this bug, email your comments to 75648 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: Minor safety improvements to fns.c/eval.c
Date: Sat, 18 Jan 2025 12:18:27 +0000

This is a spin-off from bug#75584, originally reported in

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=75584#27

fns.c contains some minor bugs which can cause crashes for buggy Elisp.
While it's conceivable that one of them could happen by accident, it
doesn't affect the pdumper build, so it is still very unlikely.

They are:

1. plist-get and plist-put assume that the cdr of a cons cell is also a
cons cell.  They check this, then call out to Lisp, then rely on the
fact.  However, the Lisp code can call setcdr and turn the cdr into a
non-cons cell, which causes a crash.

2. Fntake, Fsort, and Fwidget_put modify user-supplied cons cells with
XSETCAR/XSETCDR.  They do not check that the cons cell isn't "pure", in
which case writing to it may cause crashes.  In the case of Fsort, this
is likely to happen when a user accidentally attempts to sort a
(partially) pure list in-place.

3. Fsetq does the same thing to the lexical environment, which may
include a pure cons cell establishing a binding.

As (2) and (3) will become non-bugs once purespace is removed, I would
like to propose not fixing them for now.

(1), however, needs a fix.  Also, just as importantly, it needs tests.

Unfortunately, ert is not really set up very well for tests that may
crash.

My proposal will be to:

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.

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: bug-gnu-emacs <at> gnu.org, 75648 <at> debbugs.gnu.org
Subject: Re: bug#75648: Minor safety improvements to fns.c/eval.c
Date: Sat, 18 Jan 2025 14:20:39 +0000

"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.
Patch 2 adds new tests which crash if the bug is present.
Patch 3 fixes the actual bug.

Remarks:
* should-not-crash needs to be written carefully to avoid byte compiler
warnings and, in the general case, ignore errors.
* printing the test name before the crash makes a lot of sense to me,
and it's easier to do in ert-run-tests-batch rather than in
should-not-crash itself.
* plist_put wasn't buggy, but I applied analogous changes to keep it
synchronized  with Fplist_put.

commit a4bf048d1f36680d37ffbd4c54c67ea13840117c
Author: Pip Cet <pipcet <at> protonmail.com>
Date:   Sat Jan 18 12:26:36 2025 +0000

    Handle tests known to cause crashes
    
    * lisp/emacs-lisp/ert.el (should-not-crash): New macro.
    (ert-run-tests-batch): When running a test with a :crash tag, print
    a message first.

diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
index f25ba8a529c..cd25fe6a108 100644
--- a/lisp/emacs-lisp/ert.el
+++ b/lisp/emacs-lisp/ert.el
@@ -457,6 +457,14 @@ should-error
                        (list
                         :fail-reason "did not signal an error")))))))))
 
+(cl-defmacro should-not-crash (form)
+  "Evaluate FORM and ignore its value.
+
+If Emacs doesn't crash, count it as a success.
+
+Returns nil."
+  `(should-not (and (ignore-errors ,form) nil)))
+
 (cl-defmacro ert--skip-when (form)
   "Evaluate FORM.  If it returns t, skip the current test.
 Errors during evaluation are caught and handled like t."
@@ -1439,7 +1447,20 @@ ert-run-tests-batch
               (message "%s" ""))
             (when (getenv "EMACS_TEST_JUNIT_REPORT")
               (ert-write-junit-test-report stats)))))
-       (test-started)
+       (test-started
+        ;; for tests which might crash, the last line of output should
+        ;; indicate which test we attempted to run.
+        (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))))))
        (test-ended
         (cl-destructuring-bind (stats test result) event-args
           (unless (ert-test-result-expected-p test result)

commit f891757a7b71980fc81afff0e818911ad6e1f344
Author: Pip Cet <pipcet <at> protonmail.com>
Date:   Sat Jan 18 12:27:58 2025 +0000

    Add :crash tests for bug#75648
    
    * test/src/fns-tests.el (test-self-modifying-plist-get):
    (test-self-modifying-plist-put): New tests.

diff --git a/test/src/fns-tests.el b/test/src/fns-tests.el
index eebc92bfd41..edb919badcd 100644
--- a/test/src/fns-tests.el
+++ b/test/src/fns-tests.el
@@ -1817,4 +1817,16 @@ fns-value<-bool-vector
                     ;; Undo the flip.
                     (aset b i val)))))))))))
 
+(ert-deftest test-self-modifying-plist-get ()
+  :tags '(:crash)
+  (let ((l (list :key 'value)))
+    (should-not-crash (plist-get l :otherkey
+                                 #'(lambda (&rest _) (setcdr l nil))))))
+
+(ert-deftest test-self-modifying-plist-put ()
+  :tags '(:crash)
+  (let ((l (list :key 'value)))
+    (should-not-crash (plist-put l :otherkey 'dummy
+                                 #'(lambda (&rest _) (setcdr l nil))))))
+
 ;;; fns-tests.el ends here

commit d70c8d145a79f91b4cdd5f050a7b0f6cca425596
Author: Pip Cet <pipcet <at> protonmail.com>
Date:   Sat Jan 18 13:41:31 2025 +0000

    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;
 }
 

Message #14 received at 75648 <at> debbugs.gnu.org (full text, mbox):

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: Re: 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?

Message #17 received at 75648 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Pip Cet <pipcet <at> protonmail.com>, Andrea Corallo <acorallo <at> gnu.org>,
 75648 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Stefan Kangas <stefankangas <at> gmail.com>
Subject: Re: bug#75648: Minor safety improvements to fns.c/eval.c
Date: Sat, 18 Jan 2025 17:25:18 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

Hi,

>> 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?

This patch adds the :crash tag to ert.el. This is unfortune. Until now,
there are no predefined tags in ert.el. Do we want to change this?

If we need such a property for ert tests, we should use another
mechanism. For example, another slot in the defstruct ert-test, like
(should-not-crash nil). And your new macro should-not-crash sets this
slot to non-nil in its scope. With this, you can even declare more
precisely, which parts of your ert deftest shall be protected.

Furthermore, IIUC, your new macro succedds always if the form doesn't
crash. So we have no information, whether the form returns nil, non-nil,
or an error, in case it doesn't crash. I would still like to see the
test result from should-not-crash in this case.

Best regards, Michael.

Message #20 received at 75648 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Andrea Corallo <acorallo <at> gnu.org>, Michael Albinus <michael.albinus <at> gmx.de>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, 75648 <at> debbugs.gnu.org,
 Stefan Kangas <stefankangas <at> gmail.com>
Subject: Re: bug#75648: Minor safety improvements to fns.c/eval.c
Date: Sat, 18 Jan 2025 16:40:21 +0000

"Eli Zaretskii" <eliz <at> gnu.org> writes:

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

Responding separately to these two issues.  If we need a new bug number,
please let me know.  This is the ERT response.

Summary: Some bugs cause Emacs to crash.  We'd like to introduce
regression tests so the crashes, once fixed, won't reappear.  However,
if they do, ERT currently makes it hard to find out which test failed.
Also, these cases are usually such strange things for Elisp to do that
that it's perfectly okay for behavior to be undefined or change: we
don't want to test a return value using (should) or (should-not), and we
want to permit errors but not require them.

>> +        (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?

It reaches errputc, which has this code:

  fputc_unlocked (c, errstream ());

#ifdef WINDOWSNT
  /* Flush stderr after outputting a newline since stderr is fully
     buffered when redirected to a pipe, contrary to POSIX.  */
  if (c == '\n')
    fflush_unlocked (stderr);
#endif

So I think it does (but it's different from vmessage, which calls fflush
in other cases).

> Maybe we should introduce flush-standard-error, or change the code to
> print to stdout and use flush-standard-output?

I don't think that's necessary.

Pip

Message #23 received at 75648 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 75648 <at> debbugs.gnu.org
Subject: Re: bug#75648: Minor safety improvements to fns.c/eval.c
Date: Sat, 18 Jan 2025 16:44:12 +0000

"Eli Zaretskii" <eliz <at> gnu.org> writes:

>> @@ -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?

(Response to the plist bug fix only, thus I reduced the CC list).

We should!  I missed that one.  Is it okay if I just use CONSP and
reverse the arguments? "CONSP (x) ? XCDR (x) : y" makes it a little more
obvious that the code is safe than "!CONSP (x) ? y : XCDR (x)", IMHO.

Pip

Message #26 received at 75648 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: Eli Zaretskii <eliz <at> gnu.org>, Andrea Corallo <acorallo <at> gnu.org>,
 75648 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Stefan Kangas <stefankangas <at> gmail.com>
Subject: Re: bug#75648: Minor safety improvements to fns.c/eval.c
Date: Sat, 18 Jan 2025 17:10:02 +0000

"Michael Albinus" <michael.albinus <at> gmx.de> writes:

> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> Hi,
>
>>> 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?
>
> This patch adds the :crash tag to ert.el. This is unfortune. Until now,
> there are no predefined tags in ert.el. Do we want to change this?

It's pretty clear you don't, so let's see what else we can do:
should-not-crash can print a message, but needs to somehow be able to
access the ert test for that message to make sense.  My naive idea is to
use a dynamic let-binding for a function to (fun)call to print a
message, would that work?  We can't do what ert-fail does (throws a
signal), IIUC, but we can make it look like an ert-fail call.

> If we need such a property for ert tests, we should use another
> mechanism. For example, another slot in the defstruct ert-test, like
> (should-not-crash nil). And your new macro should-not-crash sets this
> slot to non-nil in its scope. With this, you can even declare more
> precisely, which parts of your ert deftest shall be protected.

Hmm.  I don't see how that would work.  I'll have to study the code some
more.

> Furthermore, IIUC, your new macro succedds always if the form doesn't
> crash. So we have no information, whether the form returns nil, non-nil,
> or an error, in case it doesn't crash. I would still like to see the
> test result from should-not-crash in this case.

In this case, that's intentional: it's perfectly okay to do anything but
crash or infloop, and we really don't want to have to update the tests
for every behavior change.

I haven't thought much about the case where we want to assert both that
a form doesn't crash and which value it has.  My intuition is that it's
best to have separate tests.  Admittedly that's a little easier if
should-not-crash returns the value of the form.

Pip

Message #29 received at 75648 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> protonmail.com>
Cc: 75648 <at> debbugs.gnu.org
Subject: Re: bug#75648: Minor safety improvements to fns.c/eval.c
Date: Sat, 18 Jan 2025 20:43:01 +0200

> Date: Sat, 18 Jan 2025 16:44:12 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: 75648 <at> debbugs.gnu.org
> 
> "Eli Zaretskii" <eliz <at> gnu.org> writes:
> 
> > 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?
> 
> (Response to the plist bug fix only, thus I reduced the CC list).
> 
> We should!  I missed that one.  Is it okay if I just use CONSP and
> reverse the arguments? "CONSP (x) ? XCDR (x) : y" makes it a little more
> obvious that the code is safe than "!CONSP (x) ? y : XCDR (x)", IMHO.

Sure, that's okay.

Message #32 received at 75648 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Pip Cet <pipcet <at> protonmail.com>, Eli Zaretskii <eliz <at> gnu.org>
Cc: Andrea Corallo <acorallo <at> gnu.org>, Michael Albinus <michael.albinus <at> gmx.de>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, 75648 <at> debbugs.gnu.org
Subject: Re: bug#75648: Minor safety improvements to fns.c/eval.c
Date: Sat, 18 Jan 2025 17:30:54 -0600

Pip Cet <pipcet <at> protonmail.com> writes:

> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>
>>> 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).
>
> Responding separately to these two issues.  If we need a new bug number,
> please let me know.  This is the ERT response.
>
> Summary: Some bugs cause Emacs to crash.  We'd like to introduce
> regression tests so the crashes, once fixed, won't reappear.  However,
> if they do, ERT currently makes it hard to find out which test failed.
> Also, these cases are usually such strange things for Elisp to do that
> that it's perfectly okay for behavior to be undefined or change: we
> don't want to test a return value using (should) or (should-not), and we
> want to permit errors but not require them.

I assume that the justification for the new :crash property is improved
display of the test result.  Is that right?  If so, I think it would be
useful to see an example excerpt of the output in case a test crashes
Emacs before and after this patch.

The patch that fixes the bug LGTM, but I see that Eli had a suggestion
for how to improve things even further.

Thanks for working on this.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.