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

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

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

On 5/28/20 11:19 PM, Eli Zaretskii wrote:
>> -  return (uintptr_t) p % LISP_ALIGNMENT == 0;
>> +  return (uintptr_t) p % GCALIGNMENT == 0;
>>  }
> ...replacing LISP_ALIGNMENT with GCALIGNMENT just here doesn't sound
> right to me: by keeping the current value of LISP_ALIGNMENT, we
> basically declare that Lisp objects shall be aligned on that boundary,
> whereas that isn't really the case.  Why not change the value of
> LISP_ALIGNMENT instead?

There are really two bugs here.

1. The idea of taking the address modulo LISP_ALIGNMENT is wrong, as a pointer
can point into the middle of (say) a pseudovector and not be
LISP_ALIGNMENT-aligned. Replacing LISP_ALIGNMENT with GCALIGNMENT does not fix
this bug in general, because such a pointer might not be GCALIGNMENT-aligned
either. This bug can cause crashes because it causes GC to think an object is
garbage when it's not garbage.

2. LISP_ALIGNMENT is too large on MinGW and some other platforms.

The patch I sent earlier attempted to be the simplest patch that would fix the
bug you observed on MinGW, which is a special case of (1). It does not attempt
to fix all plausible cases of (1), nor does it address (2).

We can fix these two bugs separately, by installing the attached patches into
emacs-27. The first patch fixes (1) and thus fixes the crash along with other
plausible crashes. The second one fixes (2), and this fixes the MinGW crash in a
different way but does not fix the crash on other plausible platforms. (1)
probably has better performance than (2), though I doubt whether users will notice.

[-- Attachment #2: 0001-Remove-maybe_lisp_pointer.patch --]
[-- Type: text/x-patch, Size: 1669 bytes --]

From 2c0bac868a7aefe7dafd2362cce42a7d3738319f Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 29 May 2020 12:56:16 -0700
Subject: [PATCH 1/2] =?UTF-8?q?Remove=20=E2=80=98maybe=5Flisp=5Fpointer?=
 =?UTF-8?q?=E2=80=99?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

It’s an invalid optimization, since pointers can address the
middle of Lisp_Object data.
* src/alloc.c (maybe_lisp_pointer): Remove.  Only use removed.
Do not merge to master, as we’ll put in a better fix there.
---
 src/alloc.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index 1c6b664b22..b8382aca5b 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -4585,18 +4585,6 @@ mark_maybe_objects (Lisp_Object const *array, ptrdiff_t nelts)
     mark_maybe_object (*array);
 }
 
-/* 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 LISP_ALIGNMENT.  */
-
-static bool
-maybe_lisp_pointer (void *p)
-{
-  return (uintptr_t) p % LISP_ALIGNMENT == 0;
-}
-
 /* If P points to Lisp data, mark that as live if it isn't already
    marked.  */
 
@@ -4609,9 +4597,6 @@ mark_maybe_pointer (void *p)
   VALGRIND_MAKE_MEM_DEFINED (&p, sizeof (p));
 #endif
 
-  if (!maybe_lisp_pointer (p))
-    return;
-
   if (pdumper_object_p (p))
     {
       int type = pdumper_find_object_type (p);
-- 
2.17.1


[-- Attachment #3: 0002-Don-t-overalign-Lisp-objects.patch --]
[-- Type: text/x-patch, Size: 3666 bytes --]

From f620b5b802bf2afad033c7cc7856a71fd28b2c13 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 29 May 2020 13:02:32 -0700
Subject: [PATCH 2/2] =?UTF-8?q?Don=E2=80=99t=20overalign=20Lisp=20objects?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Backport from master.
* src/alloc.c (union emacs_align_type):
New type, used for LISP_ALIGNMENT.
(LISP_ALIGNMENT): Use it instead of max_align_t.
---
 src/alloc.c | 55 +++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 45 insertions(+), 10 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index b8382aca5b..48e96863db 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -104,6 +104,46 @@ Copyright (C) 1985-1986, 1988, 1993-1995, 1997-2020 Free Software
 #include "w32heap.h"	/* for sbrk */
 #endif
 
+/* A type with alignment at least as large as any object that Emacs
+   allocates.  This is not max_align_t because some platforms (e.g.,
+   mingw) have buggy malloc implementations that do not align for
+   max_align_t.  This union contains types of all GCALIGNED_STRUCT
+   components visible here.  */
+union emacs_align_type
+{
+  struct frame frame;
+  struct Lisp_Bignum Lisp_Bignum;
+  struct Lisp_Bool_Vector Lisp_Bool_Vector;
+  struct Lisp_Char_Table Lisp_Char_Table;
+  struct Lisp_CondVar Lisp_CondVar;
+  struct Lisp_Finalizer Lisp_Finalizer;
+  struct Lisp_Float Lisp_Float;
+  struct Lisp_Hash_Table Lisp_Hash_Table;
+  struct Lisp_Marker Lisp_Marker;
+  struct Lisp_Misc_Ptr Lisp_Misc_Ptr;
+  struct Lisp_Mutex Lisp_Mutex;
+  struct Lisp_Overlay Lisp_Overlay;
+  struct Lisp_Sub_Char_Table Lisp_Sub_Char_Table;
+  struct Lisp_Subr Lisp_Subr;
+  struct Lisp_User_Ptr Lisp_User_Ptr;
+  struct Lisp_Vector Lisp_Vector;
+  struct terminal terminal;
+  struct thread_state thread_state;
+  struct window window;
+
+  /* Omit the following since they would require including process.h
+     etc.  In practice their alignments never exceed that of the
+     structs already listed.  */
+#if 0
+  struct Lisp_Module_Function Lisp_Module_Function;
+  struct Lisp_Process Lisp_Process;
+  struct save_window_data save_window_data;
+  struct scroll_bar scroll_bar;
+  struct xwidget_view xwidget_view;
+  struct xwidget xwidget;
+#endif
+};
+
 #ifdef DOUG_LEA_MALLOC
 
 /* Specify maximum number of areas to mmap.  It would be nice to use a
@@ -636,16 +676,11 @@ buffer_memory_full (ptrdiff_t nbytes)
 #define COMMON_MULTIPLE(a, b) \
   ((a) % (b) == 0 ? (a) : (b) % (a) == 0 ? (b) : (a) * (b))
 
-/* LISP_ALIGNMENT is the alignment of Lisp objects.  It must be at
-   least GCALIGNMENT so that pointers can be tagged.  It also must be
-   at least as strict as the alignment of all the C types used to
-   implement Lisp objects; since pseudovectors can contain any C type,
-   this is max_align_t.  On recent GNU/Linux x86 and x86-64 this can
-   often waste up to 8 bytes, since alignof (max_align_t) is 16 but
-   typical vectors need only an alignment of 8.  Although shrinking
-   the alignment to 8 would save memory, it cost a 20% hit to Emacs
-   CPU performance on Fedora 28 x86-64 when compiled with gcc -m32.  */
-enum { LISP_ALIGNMENT = alignof (union { max_align_t x;
+/* Alignment needed for memory blocks that are allocated via malloc
+   and that contain Lisp objects.  On typical hosts malloc already
+   aligns sufficiently, but extra work is needed on oddball hosts
+   where Emacs would crash if malloc returned a non-GCALIGNED pointer.  */
+enum { LISP_ALIGNMENT = alignof (union { union emacs_align_type x;
 					 GCALIGNED_UNION_MEMBER }) };
 verify (LISP_ALIGNMENT % GCALIGNMENT == 0);
 
-- 
2.17.1