Subject: bug#24118: closed (Re: 25.1; [PATCH] Fix a possible crash caused by mapcar1)

From: [email-masked] (GNU bug Tracking System)
Date: Wed, 03 Aug 2016 01:17:02 +0000

Your bug report

#24118: 25.1; [PATCH] Fix a possible crash caused by mapcar1

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
24118: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=24118
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: 25.1; [PATCH] Fix a possible crash caused by mapcar1
From: Paul Eggert <[email-masked]>
To: Chris Feng <[email-masked]>
Cc: Andreas Schwab <[email-masked]>, [email-masked]
Date: Tue, 2 Aug 2016 18:15:53 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

Thanks for the bug report. I installed the attached more-adventurous patch, 
which truncates the result rather than extending it with nils. This seems a bit 
more appropriate anyway.

Although it no longer matters for this patch, memclear is specified to store nil 
values regardless of how nil is represented. Of course memclear's current 
implementation assumes Qnil is zero, and memclear can't be portably and easily 
implemented if we merely change Qnil to be nonzero, but that's a bridge we don't 
have to cross unless we change Qnil to be nonzero.
From ae77c07b399a194bdb7095823595086f8385f2a4 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Tue, 2 Aug 2016 21:04:28 -0400
Subject: [PATCH] =?UTF-8?q?Fix=20(mapcar=20F=20S)=20crash=20when=20F=20alt?=
 =?UTF-8?q?ers=20S=E2=80=99s=20length?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/fns.c (mapcar1): Return number of elements computed,
which can be less than LENI if the function alters the list.
All callers changed.  (Bug#24118)
---
 src/fns.c | 77 +++++++++++++++++++++++++--------------------------------------
 1 file changed, 30 insertions(+), 47 deletions(-)

diff --git a/src/fns.c b/src/fns.c
index c318608..3895ada 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -2517,11 +2517,13 @@ usage: (nconc &rest LISTS)  */)
 }
 
 /* This is the guts of all mapping functions.
- Apply FN to each element of SEQ, one by one,
- storing the results into elements of VALS, a C vector of Lisp_Objects.
- LENI is the length of VALS, which should also be the length of SEQ.  */
+   Apply FN to each element of SEQ, one by one, storing the results
+   into elements of VALS, a C vector of Lisp_Objects.  LENI is the
+   length of VALS, which should also be the length of SEQ.  Return the
+   number of results; although this is normally LENI, it can be less
+   if SEQ is made shorter as a side effect of FN.  */
 
-static void
+static EMACS_INT
 mapcar1 (EMACS_INT leni, Lisp_Object *vals, Lisp_Object fn, Lisp_Object seq)
 {
   Lisp_Object tail, dummy;
@@ -2564,14 +2566,18 @@ mapcar1 (EMACS_INT leni, Lisp_Object *vals, Lisp_Object fn, Lisp_Object seq)
   else   /* Must be a list, since Flength did not get an error */
     {
       tail = seq;
-      for (i = 0; i < leni && CONSP (tail); i++)
+      for (i = 0; i < leni; i++)
 	{
+	  if (! CONSP (tail))
+	    return i;
 	  dummy = call1 (fn, XCAR (tail));
 	  if (vals)
 	    vals[i] = dummy;
 	  tail = XCDR (tail);
 	}
     }
+
+  return leni;
 }
 
 DEFUN ("mapconcat", Fmapconcat, Smapconcat, 3, 3, 0,
@@ -2581,34 +2587,26 @@ SEPARATOR results in spaces between the values returned by FUNCTION.
 SEQUENCE may be a list, a vector, a bool-vector, or a string.  */)
   (Lisp_Object function, Lisp_Object sequence, Lisp_Object separator)
 {
-  Lisp_Object len;
-  EMACS_INT leni;
-  EMACS_INT nargs;
-  ptrdiff_t i;
-  Lisp_Object *args;
-  Lisp_Object ret;
   USE_SAFE_ALLOCA;
-
-  len = Flength (sequence);
+  EMACS_INT leni = XFASTINT (Flength (sequence));
   if (CHAR_TABLE_P (sequence))
     wrong_type_argument (Qlistp, sequence);
-  leni = XINT (len);
-  nargs = leni + leni - 1;
-  if (nargs < 0) return empty_unibyte_string;
-
-  SAFE_ALLOCA_LISP (args, nargs);
-
-  mapcar1 (leni, args, function, sequence);
+  EMACS_INT args_alloc = 2 * leni - 1;
+  if (args_alloc < 0)
+    return empty_unibyte_string;
+  Lisp_Object *args;
+  SAFE_ALLOCA_LISP (args, args_alloc);
+  ptrdiff_t nmapped = mapcar1 (leni, args, function, sequence);
+  ptrdiff_t nargs = 2 * nmapped - 1;
 
-  for (i = leni - 1; i > 0; i--)
+  for (ptrdiff_t i = nmapped - 1; i > 0; i--)
     args[i + i] = args[i];
 
-  for (i = 1; i < nargs; i += 2)
+  for (ptrdiff_t i = 1; i < nargs; i += 2)
     args[i] = separator;
 
-  ret = Fconcat (nargs, args);
+  Lisp_Object ret = Fconcat (nargs, args);
   SAFE_FREE ();
-
   return ret;
 }
 
@@ -2618,24 +2616,15 @@ The result is a list just as long as SEQUENCE.
 SEQUENCE may be a list, a vector, a bool-vector, or a string.  */)
   (Lisp_Object function, Lisp_Object sequence)
 {
-  register Lisp_Object len;
-  register EMACS_INT leni;
-  register Lisp_Object *args;
-  Lisp_Object ret;
   USE_SAFE_ALLOCA;
-
-  len = Flength (sequence);
+  EMACS_INT leni = XFASTINT (Flength (sequence));
   if (CHAR_TABLE_P (sequence))
     wrong_type_argument (Qlistp, sequence);
-  leni = XFASTINT (len);
-
+  Lisp_Object *args;
   SAFE_ALLOCA_LISP (args, leni);
-
-  mapcar1 (leni, args, function, sequence);
-
-  ret = Flist (leni, args);
+  ptrdiff_t nmapped = mapcar1 (leni, args, function, sequence);
+  Lisp_Object ret = Flist (nmapped, args);
   SAFE_FREE ();
-
   return ret;
 }
 
@@ -2661,21 +2650,15 @@ the results by altering them (using `nconc').
 SEQUENCE may be a list, a vector, a bool-vector, or a string. */)
      (Lisp_Object function, Lisp_Object sequence)
 {
-  register EMACS_INT leni;
-  register Lisp_Object *args;
-  Lisp_Object ret;
   USE_SAFE_ALLOCA;
-
+  EMACS_INT leni = XFASTINT (Flength (sequence));
   if (CHAR_TABLE_P (sequence))
     wrong_type_argument (Qlistp, sequence);
-
-  leni = XFASTINT (Flength (sequence));
+  Lisp_Object *args;
   SAFE_ALLOCA_LISP (args, leni);
-  mapcar1 (leni, args, function, sequence);
-  ret = Fnconc (leni, args);
-
+  ptrdiff_t nmapped = mapcar1 (leni, args, function, sequence);
+  Lisp_Object ret = Fnconc (nmapped, args);
   SAFE_FREE ();
-
   return ret;
 }
 
-- 
2.5.5

Subject: 25.1; [PATCH] Fix a possible crash caused by mapcar1
From: Chris Feng <[email-masked]>
To: [email-masked]
Date: Sun, 31 Jul 2016 20:46:50 +0800
List-Id: <debbugs-submit.debbugs.gnu.org>

Processing a list with `mapcar' or `mapconcat' can be terminated early
when the list is tampered (as shown in the following example), and as a
result we'll be dealing with uninitialized memory which will likely
trigger a crash.

  (setq a (make-list 10 0))
  (mapcar (lambda (_)
            (setcdr a nil))
          a)

Chris

---

* src/fns.c (mapcar1): Check and reset uninitialized list elements.
---
 src/fns.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/fns.c b/src/fns.c
index d5a1f74..1804bce 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -2524,6 +2524,10 @@ mapcar1 (EMACS_INT leni, Lisp_Object *vals, Lisp_Object fn, Lisp_Object seq)
 	    vals[i] = dummy;
 	  tail = XCDR (tail);
 	}
+
+      /* In case the list was tampered and the loop terminated early. */
+      if (i < leni)
+        memclear (vals + i, (leni - i) * word_size);
     }
 }
 
-- 
2.8.1