bug#49261: Segfault during loadup

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

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

On 7/7/21 5:09 PM, Lars Ingebrigtsen wrote:
> There seems to be a corruption in the hash tables somewhere.  It's
> totally reproducible with the recipe

Thanks for the recipe; it let me reproduce the problem on Fedora 34 x86-64.

The problem comes from the fact that mark_maybe_pointer works 
differently on pdumper objects than it works on ordinary objects. On 
ordinary objects, roots can point anywhere into an object (because this 
sort of thing has happened on real machines), whereas on pdumper 
objects, roots had to point to the start of the object.

I worked around this particular problem changing mark_maybe_pointer so 
that pdumper roots can also be tagged (see first attached patch). 
However, I suspect this is not a complete fix, as it doesn't cover the 
case where a root points to some part of a pdumper object that is not at 
the object's start. I added a FIXME about this. Perhaps Daniel can take 
a look at it sometime. I think the remaining bug will be hit only rarely 
(if ever).

The second attached patch is in the same area, but is not part of the 
fix. It causes the GC to be a bit more accurate (i.e., less 
conservative) for roots, which can help avoid some leaks.

[-- Attachment #2: 0001-Fix-pdumper-related-GC-bug.patch --]
[-- Type: text/x-patch, Size: 1623 bytes --]

From 2f7afef5ffe023a7a12520201ab70643f826abfd Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 11 Jul 2021 00:27:43 -0700
Subject: [PATCH 1/2] Fix pdumper-related GC bug
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/alloc.c (mark_maybe_pointer): Also mark pointers
to pdumper objects, even when the pointers are tagged.
Add a FIXME saying why this isn’t enough.
---
 src/alloc.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/src/alloc.c b/src/alloc.c
index 76d8c7ddd1..752eaec135 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -4755,6 +4755,17 @@ mark_maybe_pointer (void *p)
      definitely _don't_ have an object.  */
   if (pdumper_object_p (p))
     {
+      /* FIXME: This code assumes that every reachable pdumper object
+	 is addressed either by a pointer to the object start, or by
+	 the same pointer with an LSB-style tag.  This assumption
+	 fails if a pdumper object is reachable only via machine
+	 addresses of non-initial object components.  Although such
+	 addressing is rare in machine code generated by C compilers
+	 from Emacs source code, it can occur in some cases.  To fix
+	 this problem, the pdumper code should grok non-initial
+	 addresses, as the non-pdumper code does.  */
+      uintptr_t mask = VALMASK;
+      p = (void *) ((uintptr_t) p & mask);
       /* Don't use pdumper_object_p_precise here! It doesn't check the
          tag bits. OBJ here might be complete garbage, so we need to
          verify both the pointer and the tag.  */
-- 
2.31.1


[-- Attachment #3: 0002-Make-pdumper-marking-pickier.patch --]
[-- Type: text/x-patch, Size: 4037 bytes --]

From f6472cc8e2fdcfd7365240783f34e101fe44142b Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 11 Jul 2021 00:54:32 -0700
Subject: [PATCH 2/2] Make pdumper-marking pickier
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Prevent some false-positives in conservative GC marking.
This doesn’t fix any correctness bugs; it’s merely to
reclaim some memory instead of keeping it unnecessarily.
* src/alloc.c (mark_maybe_pointer): New arg SYMBOL_ONLY.
All callers changed.  Check that the pointer’s tag, if any,
matches the pdumper-reported type.
---
 src/alloc.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index 752eaec135..b3668d2131 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -4740,7 +4740,7 @@ live_small_vector_p (struct mem_node *m, void *p)
    marked.  */
 
 static void
-mark_maybe_pointer (void *p)
+mark_maybe_pointer (void *p, bool symbol_only)
 {
   struct mem_node *m;
 
@@ -4765,15 +4765,21 @@ mark_maybe_pointer (void *p)
 	 this problem, the pdumper code should grok non-initial
 	 addresses, as the non-pdumper code does.  */
       uintptr_t mask = VALMASK;
-      p = (void *) ((uintptr_t) p & mask);
+      void *po = (void *) ((uintptr_t) p & mask);
+      char *cp = p;
+      char *cpo = po;
       /* Don't use pdumper_object_p_precise here! It doesn't check the
          tag bits. OBJ here might be complete garbage, so we need to
          verify both the pointer and the tag.  */
-      int type = pdumper_find_object_type (p);
-      if (pdumper_valid_object_type_p (type))
-        mark_object (type == Lisp_Symbol
-                     ? make_lisp_symbol (p)
-                     : make_lisp_ptr (p, type));
+      int type = pdumper_find_object_type (po);
+      if (pdumper_valid_object_type_p (type)
+	  && (!USE_LSB_TAG || p == po || cp - cpo == type))
+	{
+	  if (type == Lisp_Symbol)
+	    mark_object (make_lisp_symbol (po));
+	  else if (!symbol_only)
+	    mark_object (make_lisp_ptr (po, type));
+	}
       return;
     }
 
@@ -4791,6 +4797,8 @@ mark_maybe_pointer (void *p)
 
 	case MEM_TYPE_CONS:
 	  {
+	    if (symbol_only)
+	      return;
 	    struct Lisp_Cons *h = live_cons_holding (m, p);
 	    if (!h)
 	      return;
@@ -4800,6 +4808,8 @@ mark_maybe_pointer (void *p)
 
 	case MEM_TYPE_STRING:
 	  {
+	    if (symbol_only)
+	      return;
 	    struct Lisp_String *h = live_string_holding (m, p);
 	    if (!h)
 	      return;
@@ -4818,6 +4828,8 @@ mark_maybe_pointer (void *p)
 
 	case MEM_TYPE_FLOAT:
 	  {
+	    if (symbol_only)
+	      return;
 	    struct Lisp_Float *h = live_float_holding (m, p);
 	    if (!h)
 	      return;
@@ -4827,6 +4839,8 @@ mark_maybe_pointer (void *p)
 
 	case MEM_TYPE_VECTORLIKE:
 	  {
+	    if (symbol_only)
+	      return;
 	    struct Lisp_Vector *h = live_large_vector_holding (m, p);
 	    if (!h)
 	      return;
@@ -4836,6 +4850,8 @@ mark_maybe_pointer (void *p)
 
 	case MEM_TYPE_VECTOR_BLOCK:
 	  {
+	    if (symbol_only)
+	      return;
 	    struct Lisp_Vector *h = live_small_vector_holding (m, p);
 	    if (!h)
 	      return;
@@ -4897,7 +4913,7 @@ mark_memory (void const *start, void const *end)
   for (pp = start; (void const *) pp < end; pp += GC_POINTER_ALIGNMENT)
     {
       void *p = *(void *const *) pp;
-      mark_maybe_pointer (p);
+      mark_maybe_pointer (p, false);
 
       /* Unmask any struct Lisp_Symbol pointer that make_lisp_symbol
 	 previously disguised by adding the address of 'lispsym'.
@@ -4906,7 +4922,7 @@ mark_memory (void const *start, void const *end)
 	 non-adjacent words and P might be the low-order word's value.  */
       intptr_t ip;
       INT_ADD_WRAPV ((intptr_t) p, (intptr_t) lispsym, &ip);
-      mark_maybe_pointer ((void *) ip);
+      mark_maybe_pointer ((void *) ip, true);
     }
 }
 
-- 
2.31.1