Re: Changes in GC and in pure space (was: [Emacs-diffs] master 5d4dd55: Fix lifetime error in previous patch)

[Paul Eggert <eggert@cs.ucla.edu>]

[-- Attachment #1: Type: text/plain, Size: 216 bytes --]

Pip Cet wrote:
> Try reading #s(hash-table data #0=(#0# . #0#))

Thanks for mentioning that. I installed the attached to fix the bug, along with 
some similar bugs I noticed. Are there more instances of the problem?

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Don-t-hard-loop-on-cycles-in-read-etc.patch --]
[-- Type: text/x-patch; name="0001-Don-t-hard-loop-on-cycles-in-read-etc.patch", Size: 7611 bytes --]

From 951ea375d52891f79b89794fbb9dca86ab8cd5a8 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 21 Aug 2019 17:18:33 -0700
Subject: [PATCH] =?UTF-8?q?Don=E2=80=99t=20hard-loop=20on=20cycles=20in=20?=
 =?UTF-8?q?=E2=80=98read=E2=80=99=20etc.?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Problem for ‘read’ reported by Pip Cet in:
https://lists.gnu.org/r/emacs-devel/2019-08/msg00316.html
* src/fns.c (Frequire): Protect against circular current-load-list.
* src/lread.c (Fget_load_suffixes):
Protect against circular load-suffixes or load-file-rep-suffixes.
(Fload): Protect against circular loads-in-progress.
(openp): Protect against circular PATH and SUFFIXES.
(build_load_history): Protect against circular load-history or
current-load-list.
(readevalloop_eager_expand_eval): Protect against circular SUBFORMS.
(read1): Protect against circular data.
* test/src/lread-tests.el (lread-circular-hash): New test.
---
 src/fns.c               |  9 ++++--
 src/lread.c             | 68 ++++++++++++++++++++---------------------
 test/src/lread-tests.el |  3 ++
 3 files changed, 43 insertions(+), 37 deletions(-)

diff --git a/src/fns.c b/src/fns.c
index 6c47b3e5b1..b606d6299c 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -2950,9 +2950,12 @@ DEFUN ("require", Frequire, Srequire, 1, 3, 0,
      But not more than once in any file,
      and not when we aren't loading or reading from a file.  */
   if (!from_file)
-    for (tem = Vcurrent_load_list; CONSP (tem); tem = XCDR (tem))
-      if (NILP (XCDR (tem)) && STRINGP (XCAR (tem)))
-	from_file = 1;
+    {
+      Lisp_Object tail = Vcurrent_load_list;
+      FOR_EACH_TAIL_SAFE (tail)
+	if (NILP (XCDR (tail)) && STRINGP (XCAR (tail)))
+	  from_file = true;
+    }
 
   if (from_file)
     {
diff --git a/src/lread.c b/src/lread.c
index 1bfbf5aa86..e444830c99 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -1064,18 +1064,13 @@ DEFUN ("get-load-suffixes", Fget_load_suffixes, Sget_load_suffixes, 0, 0, 0,
 This uses the variables `load-suffixes' and `load-file-rep-suffixes'.  */)
   (void)
 {
-  Lisp_Object lst = Qnil, suffixes = Vload_suffixes, suffix, ext;
-  while (CONSP (suffixes))
+  Lisp_Object lst = Qnil, suffixes = Vload_suffixes;
+  FOR_EACH_TAIL (suffixes)
     {
       Lisp_Object exts = Vload_file_rep_suffixes;
-      suffix = XCAR (suffixes);
-      suffixes = XCDR (suffixes);
-      while (CONSP (exts))
-	{
-	  ext = XCAR (exts);
-	  exts = XCDR (exts);
-	  lst = Fcons (concat2 (suffix, ext), lst);
-	}
+      Lisp_Object suffix = XCAR (suffixes);
+      FOR_EACH_TAIL (exts)
+	lst = Fcons (concat2 (suffix, XCAR (exts)), lst);
     }
   return Fnreverse (lst);
 }
@@ -1290,8 +1285,8 @@ DEFUN ("load", Fload, Sload, 1, 5, 0,
      the general case; the second load may do something different.  */
   {
     int load_count = 0;
-    Lisp_Object tem;
-    for (tem = Vloads_in_progress; CONSP (tem); tem = XCDR (tem))
+    Lisp_Object tem = Vloads_in_progress;
+    FOR_EACH_TAIL_SAFE (tem)
       if (!NILP (Fequal (found, XCAR (tem))) && (++load_count > 3))
 	signal_error ("Recursive load", Fcons (found, Vloads_in_progress));
     record_unwind_protect (record_load_unwind, Vloads_in_progress);
@@ -1611,7 +1606,8 @@ openp (Lisp_Object path, Lisp_Object str, Lisp_Object suffixes,
 
   CHECK_STRING (str);
 
-  for (tail = suffixes; CONSP (tail); tail = XCDR (tail))
+  tail = suffixes;
+  FOR_EACH_TAIL_SAFE (tail)
     {
       CHECK_STRING_CAR (tail);
       max_suffix_len = max (max_suffix_len,
@@ -1625,12 +1621,17 @@ openp (Lisp_Object path, Lisp_Object str, Lisp_Object suffixes,
 
   absolute = complete_filename_p (str);
 
+  AUTO_LIST1 (just_use_str, Qnil);
+  if (NILP (path))
+    path = just_use_str;
+
   /* Go through all entries in the path and see whether we find the
      executable. */
-  do {
+  FOR_EACH_TAIL_SAFE (path)
+   {
     ptrdiff_t baselen, prefixlen;
 
-    if (NILP (path))
+    if (EQ (path, just_use_str))
       filename = str;
     else
       filename = Fexpand_file_name (str, XCAR (path));
@@ -1663,8 +1664,9 @@ openp (Lisp_Object path, Lisp_Object str, Lisp_Object suffixes,
     memcpy (fn, SDATA (filename) + prefixlen, baselen);
 
     /* Loop over suffixes.  */
-    for (tail = NILP (suffixes) ? list1 (empty_unibyte_string) : suffixes;
-	 CONSP (tail); tail = XCDR (tail))
+    AUTO_LIST1 (empty_string_only, empty_unibyte_string);
+    tail = NILP (suffixes) ? empty_string_only : suffixes;
+    FOR_EACH_TAIL_SAFE (tail)
       {
 	Lisp_Object suffix = XCAR (tail);
 	ptrdiff_t fnlen, lsuffix = SBYTES (suffix);
@@ -1808,10 +1810,9 @@ openp (Lisp_Object path, Lisp_Object str, Lisp_Object suffixes,
 	      }
 	  }
       }
-    if (absolute || NILP (path))
+    if (absolute)
       break;
-    path = XCDR (path);
-  } while (CONSP (path));
+   }
 
   SAFE_FREE ();
   errno = last_errno;
@@ -1838,7 +1839,7 @@ build_load_history (Lisp_Object filename, bool entire)
   tail = Vload_history;
   prev = Qnil;
 
-  while (CONSP (tail))
+  FOR_EACH_TAIL (tail)
     {
       tem = XCAR (tail);
 
@@ -1861,22 +1862,19 @@ build_load_history (Lisp_Object filename, bool entire)
 	    {
 	      tem2 = Vcurrent_load_list;
 
-	      while (CONSP (tem2))
+	      FOR_EACH_TAIL (tem2)
 		{
 		  newelt = XCAR (tem2);
 
 		  if (NILP (Fmember (newelt, tem)))
 		    Fsetcar (tail, Fcons (XCAR (tem),
 		     			  Fcons (newelt, XCDR (tem))));
-
-		  tem2 = XCDR (tem2);
 		  maybe_quit ();
 		}
 	    }
 	}
       else
 	prev = tail;
-      tail = XCDR (tail);
       maybe_quit ();
     }
 
@@ -1918,10 +1916,9 @@ readevalloop_eager_expand_eval (Lisp_Object val, Lisp_Object macroexpand)
   if (EQ (CAR_SAFE (val), Qprogn))
     {
       Lisp_Object subforms = XCDR (val);
-
-      for (val = Qnil; CONSP (subforms); subforms = XCDR (subforms))
-          val = readevalloop_eager_expand_eval (XCAR (subforms),
-                                                macroexpand);
+      val = Qnil;
+      FOR_EACH_TAIL (subforms)
+	val = readevalloop_eager_expand_eval (XCAR (subforms), macroexpand);
     }
   else
       val = eval_sub (call2 (macroexpand, val, Qt));
@@ -2861,16 +2858,19 @@ read1 (Lisp_Object readcharfun, int *pch, bool first_in_list)
 	      /* Now use params to make a new hash table and fill it.  */
 	      ht = Fmake_hash_table (param_count, params);
 
-	      while (CONSP (data))
-	      	{
+	      Lisp_Object last = data;
+	      FOR_EACH_TAIL_SAFE (data)
+		{
 	      	  key = XCAR (data);
 	      	  data = XCDR (data);
 	      	  if (!CONSP (data))
-		    error ("Odd number of elements in hash table data");
+		    break;
 	      	  val = XCAR (data);
-	      	  data = XCDR (data);
+		  last = XCDR (data);
 	      	  Fputhash (key, val, ht);
-	      	}
+		}
+	      if (!NILP (last))
+		error ("Hash table data is not a list of even length");
 
 	      return ht;
 	    }
diff --git a/test/src/lread-tests.el b/test/src/lread-tests.el
index 82b75b195c..ba5bfe0145 100644
--- a/test/src/lread-tests.el
+++ b/test/src/lread-tests.el
@@ -220,4 +220,7 @@ lread-string-to-number-trailing-dot
                    (* most-positive-fixnum most-positive-fixnum)))
     (should (= n (string-to-number (format "%d." n))))))
 
+(ert-deftest lread-circular-hash ()
+  (should-error (read "#s(hash-table data #0=(#0# . #0#))")))
+
 ;;; lread-tests.el ends here
-- 
2.17.1