Subject: bug#75551: Potentially erroneous long loop in lrealloc

From: Paul Eggert <[email-masked]>
Date: Wed, 15 Jan 2025 23:41:44 -0800

On 2025-01-15 16:11, Hong Xu wrote:
> It seems like the code is a bit less ideal to me -- on an imaginary
> oddball host, this memory exhaustion bug is so frequent that I would
> rather consider Emacs unsupported on such an oddball host.
> 
> I agree that it may be better to just remove lrealloc

Good points. Attached is a patch to make that change in a safe way. 
Please take a look at it and give it a try.

From 4d1608653efd5dc2d05126a5852a0881b7daf9a4 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Wed, 15 Jan 2025 23:32:43 -0800
Subject: [PATCH] Simplify alloc by assuming MALLOC_IS_LISP_ALIGNED

Problem reported by Hong Xu <https://bugs.gnu.org/75551#14>.
* src/alloc.c (MALLOC_IS_LISP_ALIGNED): static_assert it,
since it is true on all current Emacs platforms.
All uses simplified to assume it is true.
(xmalloc, xzalloc, xrealloc, lisp_malloc): Just use
malloc/calloc/realloc.  Since we are using the malloc-gnu
and realloc-posix modules, we need not worry about whether
these functions return a null pointer for zero-size requests.
(xrealloc): Stop worrying about no-longer-existing platforms
where realloc (nullptr, ...) did not work.
(laligned, lmalloc, lrealloc): Remove.  All uses removed.
---
 src/alloc.c | 128 +++++++++++-----------------------------------------
 1 file changed, 26 insertions(+), 102 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index 8307c74c106..e7290c55f88 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -704,21 +704,24 @@ #define COMMON_MULTIPLE(a, b) \
   ((a) % (b) == 0 ? (a) : (b) % (a) == 0 ? (b) : (a) * (b))
 
 /* 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.  */
+   and that contain Lisp objects.  */
 enum { LISP_ALIGNMENT = alignof (union { union emacs_align_type x;
 					 GCALIGNED_UNION_MEMBER }) };
 static_assert (LISP_ALIGNMENT % GCALIGNMENT == 0);
 
-/* True if malloc (N) is known to return storage suitably aligned for
-   Lisp objects whenever N is a multiple of LISP_ALIGNMENT.  In
-   practice this is true whenever alignof (max_align_t) is also a
+/* Verify Emacs's assumption that malloc (N) returns storage suitably
+   aligned for Lisp objects whenever N is a multiple of LISP_ALIGNMENT.
+   This assumption holds for current Emacs porting targets;
+   if the assumption fails on a new platform, this check should
+   cause compilation to fail and some porting work will need to be done.
+
+   In practice the assumption holds when alignof (max_align_t) is also a
    multiple of LISP_ALIGNMENT.  This works even for buggy platforms
    like MinGW circa 2020, where alignof (max_align_t) is 16 even though
    the malloc alignment is only 8, and where Emacs still works because
    it never does anything that requires an alignment of 16.  */
 enum { MALLOC_IS_LISP_ALIGNED = alignof (max_align_t) % LISP_ALIGNMENT == 0 };
+static_assert (MALLOC_IS_LISP_ALIGNED);
 
 /* If compiled with XMALLOC_BLOCK_INPUT_CHECK, define a symbol
    BLOCK_INPUT_IN_MEMORY_ALLOCATORS that is visible to the debugger.
@@ -759,9 +762,6 @@ #define MALLOC_PROBE(size)			\
       malloc_probe (size);			\
   } while (0)
 
-static void *lmalloc (size_t, bool) ATTRIBUTE_MALLOC_SIZE ((1));
-static void *lrealloc (void *, size_t);
-
 /* Like malloc but check for no memory and block interrupt input.  */
 
 void *
@@ -770,7 +770,7 @@ xmalloc (size_t size)
   void *val;
 
   MALLOC_BLOCK_INPUT;
-  val = lmalloc (size, false);
+  val = malloc (size);
   MALLOC_UNBLOCK_INPUT;
 
   if (!val)
@@ -787,7 +787,7 @@ xzalloc (size_t size)
   void *val;
 
   MALLOC_BLOCK_INPUT;
-  val = lmalloc (size, true);
+  val = calloc (1, size);
   MALLOC_UNBLOCK_INPUT;
 
   if (!val)
@@ -804,12 +804,7 @@ xrealloc (void *block, size_t size)
   void *val;
 
   MALLOC_BLOCK_INPUT;
-  /* Call lmalloc when BLOCK is null, for the benefit of long-obsolete
-     platforms lacking support for realloc (NULL, size).  */
-  if (! block)
-    val = lmalloc (size, false);
-  else
-    val = lrealloc (block, size);
+  val = realloc (block, size);
   MALLOC_UNBLOCK_INPUT;
 
   if (!val)
@@ -994,15 +989,23 @@ record_xmalloc (size_t size)
 }

-/* Like malloc but used for allocating Lisp data.  NBYTES is the
-   number of bytes to allocate, TYPE describes the intended use of the
-   allocated memory block (for strings, for conses, ...).  */
-
 #if ! USE_LSB_TAG
 extern void *lisp_malloc_loser;
 void *lisp_malloc_loser EXTERNALLY_VISIBLE;
 #endif
 
+/* Allocate memory for Lisp data.
+   NBYTES is the number of bytes to allocate; it must be Lisp-aligned.
+   If CLEARIT, arrange for the allocated memory to be cleared
+   by using calloc, which can be faster than malloc+memset.
+   TYPE describes the intended use of the allocated memory block
+   (for strings, for conses, ...).
+   Return a null pointer if and only if allocation failed.
+
+   Code allocating heap memory for Lisp should use this function to get
+   a pointer P; that way, if T is an enum Lisp_Type value and
+   L == make_lisp_ptr (P, T), then XPNTR (L) == P and XTYPE (L) == T.  */
+
 static void *
 lisp_malloc (size_t nbytes, bool clearit, enum mem_type type)
 {
@@ -1014,7 +1017,7 @@ lisp_malloc (size_t nbytes, bool clearit, enum mem_type type)
   allocated_mem_type = type;
 #endif
 
-  val = lmalloc (nbytes, clearit);
+  val = clearit ? calloc (1, nbytes) : malloc (nbytes);
 
 #if ! USE_LSB_TAG
   /* If the memory just allocated cannot be addressed thru a Lisp
@@ -1098,11 +1101,7 @@ aligned_alloc (size_t alignment, size_t size)
      Verify this for all arguments this function is given.  */
   static_assert (BLOCK_ALIGN % sizeof (void *) == 0
 		 && POWER_OF_2 (BLOCK_ALIGN / sizeof (void *)));
-  static_assert (MALLOC_IS_LISP_ALIGNED
-		 || (LISP_ALIGNMENT % sizeof (void *) == 0
-		     && POWER_OF_2 (LISP_ALIGNMENT / sizeof (void *))));
-  eassert (alignment == BLOCK_ALIGN
-	   || (!MALLOC_IS_LISP_ALIGNED && alignment == LISP_ALIGNMENT));
+  eassert (alignment == BLOCK_ALIGN);
 
   void *p;
   return posix_memalign (&p, alignment, size) == 0 ? p : 0;
@@ -1350,81 +1349,6 @@ lisp_align_free (void *block)
   MALLOC_UNBLOCK_INPUT;
 }
 
-/* True if a malloc-returned pointer P is suitably aligned for SIZE,
-   where Lisp object alignment may be needed if SIZE is a multiple of
-   LISP_ALIGNMENT.  */
-
-static bool
-laligned (void *p, size_t size)
-{
-  return (MALLOC_IS_LISP_ALIGNED || (intptr_t) p % LISP_ALIGNMENT == 0
-	  || size % LISP_ALIGNMENT != 0);
-}
-
-/* Like malloc and realloc except return null only on failure,
-   the result is Lisp-aligned if SIZE is, and lrealloc's pointer
-   argument must be nonnull.  Code allocating C heap memory
-   for a Lisp object should use one of these functions to obtain a
-   pointer P; that way, if T is an enum Lisp_Type value and L ==
-   make_lisp_ptr (P, T), then XPNTR (L) == P and XTYPE (L) == T.
-
-   If CLEARIT, arrange for the allocated memory to be cleared.
-   This might use calloc, as calloc can be faster than malloc+memset.
-
-   On typical modern platforms these functions' loops do not iterate.
-   On now-rare (and perhaps nonexistent) platforms, the code can loop,
-   reallocating (typically with larger and larger sizes) until the
-   allocator returns a Lisp-aligned pointer.  This loop in
-   theory could repeat forever.  If an infinite loop is possible on a
-   platform, a build would surely loop and the builder can then send
-   us a bug report.  Adding a counter to try to detect any such loop
-   would complicate the code (and possibly introduce bugs, in code
-   that's never really exercised) for little benefit.  */
-
-static void *
-lmalloc (size_t size, bool clearit)
-{
-#ifdef USE_ALIGNED_ALLOC
-  if (! MALLOC_IS_LISP_ALIGNED && size % LISP_ALIGNMENT == 0)
-    {
-      void *p = aligned_alloc (LISP_ALIGNMENT, size);
-      if (p)
-	{
-	  if (clearit)
-	    memclear (p, size);
-	}
-      else if (! (MALLOC_0_IS_NONNULL || size))
-	return aligned_alloc (LISP_ALIGNMENT, LISP_ALIGNMENT);
-      return p;
-    }
-#endif
-
-  while (true)
-    {
-      void *p = clearit ? calloc (1, size) : malloc (size);
-      if (laligned (p, size) && (MALLOC_0_IS_NONNULL || size || p))
-	return p;
-      free (p);
-      size_t bigger;
-      if (!ckd_add (&bigger, size, LISP_ALIGNMENT))
-	size = bigger;
-    }
-}
-
-static void *
-lrealloc (void *p, size_t size)
-{
-  while (true)
-    {
-      p = realloc (p, size);
-      if (laligned (p, size) && (size || p))
-	return p;
-      size_t bigger;
-      if (!ckd_add (&bigger, size, LISP_ALIGNMENT))
-	size = bigger;
-    }
-}
-
 
 /***********************************************************************
 			 Interval Allocation
-- 
2.45.2