GNU bug report logs - #75845
mapconcat crashes for unreasonable self-modifications

Previous Next

Full log

View this message in rfc822 format

From: Pip Cet <pipcet <at> protonmail.com>
To: 75845 <at> debbugs.gnu.org
Subject: bug#75845: mapconcat crashes for unreasonable self-modifications
Date: Sun, 26 Jan 2025 02:37:37 +0000

Pip Cet <pipcet <at> protonmail.com> writes:

> If the mapping function supplied to mapconcat modifies the sequence
> being mapped over, Emacs can crash.
>
> This seems easily fixable, but I wanted to file a bug to make sure we
> get it right: our current highly-optimized implementation of mapconcat
> will fail in many cases in which a naive implementation of mapconcat (in
> Lisp, say) would succeed.  Some of those limitations are documented (the
> argument must be a sequence when the function is called), some aren't
> (the sequence must not become shorter or longer).
>
> My suggestion is to make the ordinary no-modification case work as fast
> as is possible under the constraint that extraordinary
> self-modifications should not result in crashes.  They can and will
> result in unpredictable behavior, though.
>
> These crash:
>
> (let ((l (make-list 1000 nil))) (mapconcat (lambda (x) (ntake 1 l)) l))
>
> (let ((s (string ?a 1000))) (mapconcat (lambda (x) (aset s 0 1000) (string x)) s))
>
> Patch will follow once this has a bug number.

This patch does the minimum work necessary; while the previous code
attempted to return the "correct" length of a list which was modified,
crashing in an eassert in mapconcat, the new code errors instead.  As
error is marked with ATTRIBUTE_COLD, this should result in better code.

The string case is slightly trickier: we don't keep around an SDATA
pointer, which is good, but we do remember byte and charpos index
values, and those might get out of sync.  So verify we're looking at a
CHAR_HEAD when we need to, which is for multibyte strings.

mapconcat itself is also fixed not to assert but to use whatever mapcar1
returns.

From cf861bc042650c968f2702d842ffa4a36f0105e9 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet <at> protonmail.com>
Subject: [PATCH] Fix crashes in mapconcat etc. (bug#75845)

This doesn't attempt to catch all cases in which the mapping function
modifies the sequence, only those which would cause crashes.

* src/fns.c (mapcar1): Error if we detect modifications by the mapping
function.
(mapconcat): Don't eassert that 'mapcar1' never returns a
partially-filled vector, even though that is currently correct.
---
 src/fns.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/fns.c b/src/fns.c
index 081ed2b9f51..8e8b9584509 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -3385,7 +3385,7 @@ mapcar1 (EMACS_INT leni, Lisp_Object *vals, Lisp_Object fn, Lisp_Object seq)
       for (ptrdiff_t i = 0; i < leni; i++)
 	{
 	  if (! CONSP (tail))
-	    return i;
+	    error ("list modified by mapping function");
 	  Lisp_Object dummy = calln (fn, XCAR (tail));
 	  if (vals)
 	    vals[i] = dummy;
@@ -3403,16 +3403,21 @@ mapcar1 (EMACS_INT leni, Lisp_Object *vals, Lisp_Object fn, Lisp_Object seq)
     }
   else if (STRINGP (seq))
     {
+      ptrdiff_t i = 0;
       ptrdiff_t i_byte = 0;
 
-      for (ptrdiff_t i = 0; i < leni;)
+      while (i_byte < SBYTES (seq))
 	{
 	  ptrdiff_t i_before = i;
+	  if (STRING_MULTIBYTE (seq) && !CHAR_HEAD_P (SREF (seq, i_byte)))
+	    error ("string modified by mapping function");
 	  int c = fetch_string_char_advance (seq, &i, &i_byte);
 	  Lisp_Object dummy = calln (fn, make_fixnum (c));
 	  if (vals)
 	    vals[i_before] = dummy;
 	}
+      if (i < leni)
+	error ("string modified by mapping function");
     }
   else
     {
@@ -3473,8 +3478,7 @@ DEFUN ("mapconcat", Fmapconcat, Smapconcat, 2, 3, 0,
 	  goto concat;
 	}
     }
-  ptrdiff_t nmapped = mapcar1 (leni, args, function, sequence);
-  eassert (nmapped == leni);
+  leni = mapcar1 (leni, args, function, sequence);
 
  concat: ;
   ptrdiff_t nargs = args_alloc;
-- 
2.47.1

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.