bug#75322: SAFE_ALLOCA assumed to root Lisp_Objects/SSDATA(string)

[Daniel Colascione <dancol@dancol.org>]

Daniel Colascione <dancol@dancol.org> writes:

> On January 5, 2025 9:11:08 AM EST, "Gerd Möllmann"
> <gerd.moellmann@gmail.com> wrote:
>>Eli Zaretskii <eliz@gnu.org> writes:
>>
>>>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>>>> Cc: pipcet@protonmail.com,  75322@debbugs.gnu.org
>>>> Date: Sat, 04 Jan 2025 11:20:41 +0100
>>>> 
>>>> In callproc.c I found two: call_process and create_temp_file both use
>>>> SAFE_NALLOCA to store Lisp_Objects. I think these should be replaces
>>>> with SAVE_ALLOCA_LISP.
>>>
>>> What are the conditions under which placing Lisp objects into
>>> SAFE_NALLOCA is not safe?
>>>
>>> I understand that the first condition is that SAFE_NALLOCA uses
>>> xmalloc instead of alloca.
>>
>>Right. If it doesn't use xmalloc, the references are on the C stack, and
>>both old and new GC handle that by scanning the C stack.
>>
>>> But what are the other conditions?  Is one of them that GC could
>>> happen while these Lisp objects are in the memory allocated by
>>> SAFE_NALLOCA off the heap?  
>>
>>Yes.
>>
>>> IOW, if no GC happen, is that still unsafe? And if GC _can_ happen,
>>> but we don't use the allocated block again, is that a problem? For
>>> example, in this fragment:
>>>
>>> 	    SAFE_NALLOCA (args2, 1, nargs + 1);
>>> 	    args2[0] = Qcall_process;
>>> 	    for (i = 0; i < nargs; i++) args2[i + 1] = args[i];
>>> 	    coding_systems = Ffind_operation_coding_system (nargs + 1, args2);
>>> 	    val = CONSP (coding_systems) ? XCDR (coding_systems) : Qnil;
>>>
>>> Let's say Ffind_operation_coding_system could trigger GC.  But we
>>> never again use the args2[] array after Ffind_operation_coding_system
>>> returns.  Is the above still unsafe?  If so, could you tell what
>>> could MPS do during GC to make this unsafe?
>>
>>Let me first say why I find this unsafe in the old GC, in principle. If
>>we don't assume anything about the objects referenced from args2, then a
>>reference in args2 may well be the only one to some object. In this
>>case, the old GC would sweep it.
>
> Gerd is right. This pattern was never safe.

Here's a demonstration of the problem.  Run ./emacs -batch -Q --eval
'(acos 0)'.  If you leave demo_crash to true, Emacs will abort quickly
after we detect a use-after-free.  If you set demo_crash to false, Emacs
will run the loop all day.

diff --git a/src/floatfns.c b/src/floatfns.c
index 065ae16e885..a95597beef8 100644
--- a/src/floatfns.c
+++ b/src/floatfns.c
@@ -50,6 +50,8 @@ Copyright (C) 1988, 1993-1994, 1999, 2001-2025 Free Software Foundation,
 
 #include <config.h>
 
+#include <stdlib.h>
+
 #include "lisp.h"
 #include "bignum.h"
 
@@ -86,6 +88,31 @@ DEFUN ("acos", Facos, Sacos, 1, 1, 0,
        doc: /* Return the inverse cosine of ARG.  */)
   (Lisp_Object arg)
 {
+  Lisp_Object* args2;
+  unsigned start = 12345;
+  unsigned counter = start;
+  bool demo_crash = true;
+
+  USE_SAFE_ALLOCA;
+
+  SAFE_NALLOCA (args2, 1, 1 + (demo_crash ? MAX_ALLOCA : 0));
+  args2[0] = Fcons (make_fixnum (counter),
+		    make_fixnum (counter + 1));
+  counter += 2;
+  for (;;)
+    {
+      if (!FIXNUMP (XCAR (args2[0])))
+	emacs_abort ();
+      if (XFIXNUM (XCAR (args2[0])) != 12345)
+	emacs_abort ();
+      Fcons (make_fixnum (counter),
+	     make_fixnum (counter + 1));
+      Fgarbage_collect ();
+      fprintf (stderr, ".");
+      fflush (stderr);
+      counter += 2;
+    }
+
   double d = extract_float (arg);
   d = acos (d);
   return make_float (d);