bug#41321: 27.0.91; Emacs aborts due to invalid pseudovector objects

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

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

On 5/27/20 11:31 PM, Pip Cet wrote:
> I hope you're right, in that compilers will support GC better before
> they move on to clever optimizations that break it :-)

After looking into it, I decided it wasn't worth the hassle of treating pointers
just past the end of a Lisp object as pointing into the object. Although such
pointers can exist, I can't think of a realistic-with-today's-compilers scenario
at the machine level where (1) a pointer like that will exist, (2) no pointers
into the middle or start of the object will exist, and (3) the object might be
accessed later. In contrast we have seen scenarios with pointers into the middle
of Lisp objects.

With that in mind, attached is a proposed patch to master that I hope deals with
some of the more-serious problems mentioned so far in this thread, in particular
the problem with Lisp_Object representations of symbols being split into two
registers in a --with-wide-int build. I haven't tested this as much as I'd like,
but I need to turn my attention to sleep and work and so this is a good place to
broadcast a checkpoint.

This patch doesn't address the LISP_ALIGNMENT issues you mentioned, both in
lisp.h and in the pdumper; I can work on that soon, I think.

PS. Thanks for helping bring this problem to our attention; it's been fun to
look into it.

[-- Attachment #2: 0001-Fix-crashes-due-to-misidentified-pointers.patch --]
[-- Type: text/x-patch, Size: 5798 bytes --]

From 023344217e05d2a23b5e8157da2f9aea16a5df78 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 28 May 2020 00:11:08 -0700
Subject: [PATCH] Fix crashes due to misidentified pointers
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Problem reported by Pip Cet (Bug#41321#74, Bug#41321#80)
A compiler can create temporaries that point somewhere into a Lisp
object but are not GCALIGNED, and these temporaries may be the
only thing that addresses the object.  So, if any value points
within an object, treat the object as being addressed.  However,
do not worry about pointers that point just past the end of an
object, as these do not seem to be a problem in practice and
attempting to worry about them would complicate and slow the code.
* src/alloc.c (live_float_p): Don’t insist that the offset
be aligned properly for a float, since it might be tagged
or offset.
(GC_OBJECT_ALIGNMENT_MINIMUM, maybe_lisp_pointer):
Remove.  All uses removed.
(mark_maybe_pointer): New arg SYMBOL_ONLY.  All callers changed.
Don’t insist on pointers being aligned.
Align pointers before doing pdumper checks on them, and
before giving them to make_lisp_ptr.
(mark_memory): Do not use mark_maybe_object here.
Instead, use mark_maybe_pointer alone; that suffices.
Also look for offsets from lispsym, to mark symbols more
reliably.
---
 src/alloc.c | 65 +++++++++++++++--------------------------------------
 1 file changed, 18 insertions(+), 47 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index e241b9933a..b1d45dbb33 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -4560,7 +4560,6 @@ live_float_p (struct mem_node *m, void *p)
       /* P must point to the start of a Lisp_Float and not be
 	 one of the unused cells in the current float block.  */
       return (offset >= 0
-	      && offset % sizeof b->floats[0] == 0
 	      && offset < (FLOAT_BLOCK_SIZE * sizeof b->floats[0])
 	      && (b != float_block
 		  || offset / sizeof b->floats[0] < float_block_index));
@@ -4687,54 +4686,25 @@ mark_maybe_objects (Lisp_Object const *array, ptrdiff_t nelts)
     mark_maybe_object (*array);
 }
 
-/* A lower bound on the alignment of Lisp objects that need marking.
-   Although 1 is safe, higher values speed up mark_maybe_pointer.
-   If USE_LSB_TAG, this value is typically GCALIGNMENT; otherwise,
-   it's determined by the natural alignment of Lisp structs.
-   All vectorlike objects have alignment at least that of union
-   vectorlike_header and it's unlikely they all have alignment greater,
-   so use the union as a safe and likely-accurate standin for
-   vectorlike objects.  */
-
-enum { GC_OBJECT_ALIGNMENT_MINIMUM
-         = max (GCALIGNMENT,
-		min (alignof (union vectorlike_header),
-		     min (min (alignof (struct Lisp_Cons),
-			       alignof (struct Lisp_Float)),
-			  min (alignof (struct Lisp_String),
-			       alignof (struct Lisp_Symbol))))) };
-
-/* Return true if P might point to Lisp data that can be garbage
-   collected, and false otherwise (i.e., false if it is easy to see
-   that P cannot point to Lisp data that can be garbage collected).
-   Symbols are implemented via offsets not pointers, but the offsets
-   are also multiples of GC_OBJECT_ALIGNMENT_MINIMUM.  */
-
-static bool
-maybe_lisp_pointer (void *p)
-{
-  return (uintptr_t) p % GC_OBJECT_ALIGNMENT_MINIMUM == 0;
-}
-
 /* If P points to Lisp data, mark that as live if it isn't already
-   marked.  */
+   marked.  If SYMBOL_ONLY, mark it only if it is a symbol.  */
 
 static void
-mark_maybe_pointer (void *p)
+mark_maybe_pointer (void *p, bool symbol_only)
 {
+  char *cp = p;
   struct mem_node *m;
 
 #ifdef USE_VALGRIND
   VALGRIND_MAKE_MEM_DEFINED (&p, sizeof (p));
 #endif
 
-  if (!maybe_lisp_pointer (p))
-    return;
-
   if (pdumper_object_p (p))
     {
+      p = cp - (uintptr_t) cp % GCALIGNMENT;
       int type = pdumper_find_object_type (p);
-      if (pdumper_valid_object_type_p (type))
+      if (pdumper_valid_object_type_p (type)
+	  && (type == Lisp_Symbol || !symbol_only))
         mark_object (type == Lisp_Symbol
                      ? make_lisp_symbol (p)
                      : make_lisp_ptr (p, type));
@@ -4755,11 +4725,13 @@ mark_maybe_pointer (void *p)
 	  break;
 
 	case MEM_TYPE_CONS:
-	  obj = live_cons_holding (m, p);
+	  if (!symbol_only)
+	    obj = live_cons_holding (m, p);
 	  break;
 
 	case MEM_TYPE_STRING:
-	  obj = live_string_holding (m, p);
+	  if (!symbol_only)
+	    obj = live_string_holding (m, p);
 	  break;
 
 	case MEM_TYPE_SYMBOL:
@@ -4767,13 +4739,14 @@ mark_maybe_pointer (void *p)
 	  break;
 
 	case MEM_TYPE_FLOAT:
-	  if (live_float_p (m, p))
-	    obj = make_lisp_ptr (p, Lisp_Float);
+	  if (!symbol_only && live_float_p (m, p))
+	    obj = make_lisp_ptr (cp - (uintptr_t) cp % GCALIGNMENT, Lisp_Float);
 	  break;
 
 	case MEM_TYPE_VECTORLIKE:
 	case MEM_TYPE_VECTOR_BLOCK:
-	  obj = live_vector_holding (m, p);
+	  if (!symbol_only)
+	    obj = live_vector_holding (m, p);
 	  break;
 
 	default:
@@ -4830,12 +4803,10 @@ mark_memory (void const *start, void const *end)
 
   for (pp = start; (void const *) pp < end; pp += GC_POINTER_ALIGNMENT)
     {
-      mark_maybe_pointer (*(void *const *) pp);
-
-      verify (alignof (Lisp_Object) % GC_POINTER_ALIGNMENT == 0);
-      if (alignof (Lisp_Object) == GC_POINTER_ALIGNMENT
-	  || (uintptr_t) pp % alignof (Lisp_Object) == 0)
-	mark_maybe_object (*(Lisp_Object const *) pp);
+      char *p = *(char *const *) pp;
+      mark_maybe_pointer (p, false);
+      p += (intptr_t) lispsym;
+      mark_maybe_pointer (p, true);
     }
 }
 
-- 
2.17.1